Conversation
orium
approved these changes
Feb 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It turns out that
bad_selector => unreachable!()is sometimes reachable, and takes down the whole process due toextern "C"not allowing unwinding. We have a problem with the selector compiler, but we also have a bigger problem with being unable to guarantee that we won't suddenly abort the whole process.We're not really gaining anything by trying to panic through the C API, because it either aborts immediately (when built with
panic=abort), or aborts on the nearestextern "C"boundary ("C-unwind" would let it unwind, but C++ wouldn't be able to make sense of it anyway). Aborting prevents the caller from reporting the panic via runtime mechanisms like Sentry.I've reviewed the the library for potential lurking panics and fixed what I could along the way.
I've added
catch_unwindaround the biggest FFI functions.We have a few cases of "this should never happen" in code. If they did happen, then we still wouldn't want the chaos of panicking in FFI, so I've softened them to a
debug_assert!+return/fallback. This has a nice side effect of making code leaner (2.5% reduction of compiled code size).I've noticed that
OnceCelland thread-locals generate code with hidden potential edge-case panics, and replaced them with code that definitely can't panic (at least for now, percargo asmoutput).BTW I've replaced the table of 40 references to encodings documented as "don't take references to this" with ones that allow it.
What I didn't/couldn't change:
end()has been called. I think that's excessive, but dialing that down to a regularResult::Errwould be technically a breaking change. These panics are still caught at the FFI boundary.HtmlRewriter::newreturnsSelf, notResult<Self>, so unfortunately it can't gracefully report issues duringnew. The unsupported selectors sneaking past validation are a systemic problem for us, so instead of letting them blow up the whole rewriter, I've made them compiled to never-matching (CSS drops unknown selectors too, so this shouldn't be outrageous).lol-html, not the callers. Many getters don't have a way to explicitly report an error, and I didn't want to make the API too sloppy and quietly return empty strings, etc.