Bug 2018924 - Make ElementData thread-safety checks optional, debug-only. r=#style
ClosedPublic
Actions

Authored by emilio on Feb 24 2026, 12:06 PM.

Details

Reviewers

dshin

Group Reviewers

Restricted Project

Commits

rFIREFOXAUTOLAND469e2d980ab6: Bug 2018924 - Make ElementData thread-safety checks optional, debug-only.

Bugzilla Bug ID

2018924

Summary

Right now we unconditionally use AtomicRefCell<ElementData>. I'm
experimenting with moving the primary frame pointer from nsINode to
inside ElementData (for elements at least, of course).

In order for it not to take extra memory, we need to not go over the
jemalloc bucket limit (16 / 32 bytes, etc).

That's generally the case (ElementData is 24 bytes currently), except
the wrapping AtomicRefCell adds another 8.

Make the checks optional / debug-only. We're already skipping the checks
in a bunch of cases for performance reasons, clear_data()'s soundness
check is already debug-only, and we haven't found these checks
particularly useful in the wild, so seems worth it...

Diff Detail

Repository: rFIREFOXAUTOLAND firefox-autoland

Event Timeline

emilio created this revision.Feb 24 2026, 12:06 PM

Herald added a project: secure-revision. · View Herald TranscriptFeb 24 2026, 12:06 PM

phab-bot published this revision for review.Feb 24 2026, 12:06 PM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

Herald added a subscriber: zrhoffman. · View Herald TranscriptFeb 24 2026, 12:06 PM

Harbormaster completed remote builds in B918207: Diff 1210872.Feb 24 2026, 12:22 PM

From bug descriptions, and from previous behaviour as you mentioned, this seems like it would be... What we need (Certainly not ideal).
Is the only line of defense left against thread safety now "make sure to run debug tests?" Is it "hard" by API design to mistakenly cause non-thread-safe access? Personal anec-data is the style/main thread boundary isn't that easy to cross by accident, at least.

servo/components/style/data.rs
287	Ok, this is pretty cool, haha

Fix opt build

Harbormaster completed remote builds in B918333: Diff 1211018.Feb 24 2026, 3:25 PM

emilio marked an inline comment as done.Feb 24 2026, 3:59 PM

emilio added inline comments.

servo/components/style/data.rs
287	Ok, this is pretty cool, haha Yeah I went a bit back and forth on how to less painfully implement it in a way that was sound (didn't require to create a reference to `ElementData` before doing the safety check), and this was the best one I could think of, but alternatives welcome :)

Ah, right, didn't mean to block the review on approach, but did want to figure out if we needed follow-up for making up for (admittedly slightly) less safety - be that be additional doc/change somewhere else. Would be unfortunate if this became a tripping hazard.

This revision is now accepted and ready to land.Feb 24 2026, 4:30 PM

In D284632#9874681, @dshin wrote:

Ah, right, didn't mean to block the review on approach, but did want to figure out if we needed follow-up for making up for (admittedly slightly) less safety - be that be additional doc/change somewhere else. Would be unfortunate if this became a tripping hazard.

Generally we run WPT with forced parallelism even on debug builds, so I think we should exercise the debug checks reasonably well. We could also enable them in diagnostic builds or something but that affects nightly which hurts performance measurements.

In D284632#9874788, @emilio wrote:

In D284632#9874681, @dshin wrote:

Ah, right, didn't mean to block the review on approach, but did want to figure out if we needed follow-up for making up for (admittedly slightly) less safety - be that be additional doc/change somewhere else. Would be unfortunate if this became a tripping hazard.

Generally we run WPT with forced parallelism even on debug builds, so I think we should exercise the debug checks reasonably well. We could also enable them in diagnostic builds or something but that affects nightly which hurts performance measurements.