Skip to content

Comments

Don't panic in FFI#301

Merged
orium merged 13 commits intomainfrom
depanic
Feb 21, 2026
Merged

Don't panic in FFI#301
orium merged 13 commits intomainfrom
depanic

Conversation

@kornelski
Copy link
Contributor

It turns out that bad_selector => unreachable!() is sometimes reachable, and takes down the whole process due to extern "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 nearest extern "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_unwind around 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 OnceCell and thread-locals generate code with hidden potential edge-case panics, and replaced them with code that definitely can't panic (at least for now, per cargo asm output).

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:

  • some methods in lol-html's Rust API are documented as panicking if used after end() has been called. I think that's excessive, but dialing that down to a regular Result::Err would be technically a breaking change. These panics are still caught at the FFI boundary.
  • HtmlRewriter::new returns Self, not Result<Self>, so unfortunately it can't gracefully report issues during new. 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).
  • assertions for non-NULL arguments in FFI. I focused on defusing problems caused by 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.

@kornelski kornelski requested review from a team, Noah-Kennedy, jasnell and orium as code owners February 21, 2026 00:00
@orium orium merged commit 7869a38 into main Feb 21, 2026
6 checks passed
@orium orium deleted the depanic branch February 21, 2026 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants