Skip to content

script: Avoid data race in ServoLayoutElement::apply_selector_flags()#42963

Merged
Loirooriol merged 1 commit into
servo:mainfrom
Loirooriol:data-race-apply_selector_flags
Mar 3, 2026
Merged

script: Avoid data race in ServoLayoutElement::apply_selector_flags()#42963
Loirooriol merged 1 commit into
servo:mainfrom
Loirooriol:data-race-apply_selector_flags

Conversation

@Loirooriol
Copy link
Copy Markdown
Contributor

@Loirooriol Loirooriol commented Mar 2, 2026 •
edited
Loading

This changes Element::selector_flags from Cell<ElementSelectorFlags> to AtomicUsize, thus avoiding the risk of data races.

Testing: Tested manually. We don't run tests using TSAN builds.
Fixes: #37495

@mrobinson
Copy link
Copy Markdown
Member

mrobinson commented Mar 2, 2026

Hrm. Does Gecko also use locking in this case? I assume this code is very performance sensitive, so I want to be very sure this is the right approach before landing this.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 2, 2026

❌ 4 Tests Failed:

Tests completed Failed Passed Skipped
873 4 869 0
View the top 3 failed test(s) by shortest run time 
script_tests::size_of::size_element
Stack Traces | 0.009s run time 
thread 'size_of::size_element' (37210) panicked at .../unit/script/size_of.rs:34:1:
Your changes have increased the stack size of commonly used DOM struct Element from 344 to 360. These structs are present in large quantities in the DOM, and increasing the size may dramatically affect our memory footprint. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update to the new size in .../unit/script/size_of.rs.
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: script_tests::size_of::size_element
   3: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
script_tests::size_of::size_span
Stack Traces | 0.009s run time 
thread 'size_of::size_span' (37226) panicked at .../unit/script/size_of.rs:37:1:
Your changes have increased the stack size of commonly used DOM struct HTMLSpanElement from 360 to 376. These structs are present in large quantities in the DOM, and increasing the size may dramatically affect our memory footprint. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update to the new size in .../unit/script/size_of.rs.
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: script_tests::size_of::size_span
   3: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
script_tests::size_of::size_htmlelement
Stack Traces | 0.01s run time 
thread 'size_of::size_htmlelement' (37222) panicked at .../unit/script/size_of.rs:35:1:
Your changes have increased the stack size of commonly used DOM struct HTMLElement from 360 to 376. These structs are present in large quantities in the DOM, and increasing the size may dramatically affect our memory footprint. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update to the new size in .../unit/script/size_of.rs.
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: script_tests::size_of::size_htmlelement
   3: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
script_tests::size_of::size_div
Stack Traces | 0.011s run time 
thread 'size_of::size_div' (37206) panicked at .../unit/script/size_of.rs:36:1:
Your changes have increased the stack size of commonly used DOM struct HTMLDivElement from 360 to 376. These structs are present in large quantities in the DOM, and increasing the size may dramatically affect our memory footprint. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update to the new size in .../unit/script/size_of.rs.
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: script_tests::size_of::size_div
   3: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@Loirooriol
Copy link
Copy Markdown
Contributor Author

Loirooriol commented Mar 2, 2026

Gecko uses AtomicU32, I guess we can do the same. https://searchfox.org/firefox-main/rev/ad106dcbb4483fb34c1b144fa74eb32e885171ce/servo/components/style/gecko/wrapper.rs#312-325

@Loirooriol
script: Avoid data race in ServoLayoutElement::apply_selector_flags()
be1f236 
This changes `Element::selector_flags` from `Cell<ElementSelectorFlags>`
to `AtomicUsize`, thus avoiding the risk of data races.

Signed-off-by: Oriol Brufau <[email protected]>
@Loirooriol Loirooriol force-pushed the data-race-apply_selector_flags branch from f3669c8 to be1f236 Compare March 2, 2026 23:20
@Loirooriol Loirooriol marked this pull request as ready for review March 2, 2026 23:27
@Loirooriol Loirooriol requested a review from gterzian as a code owner March 2, 2026 23:27
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 2, 2026
@jdm jdm mentioned this pull request Mar 3, 2026
Merged
@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Mar 3, 2026
@Loirooriol Loirooriol added this pull request to the merge queue Mar 3, 2026
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 3, 2026
Merged via the queue into servo:main with commit 36c6594 Mar 3, 2026
34 checks passed
@Loirooriol Loirooriol deleted the data-race-apply_selector_flags branch March 3, 2026 12:11
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 3, 2026
offline-ant pushed a commit to offline-ant/havi that referenced this pull request Jun 4, 2026
@Loirooriol
script: Avoid data race in `ServoLayoutElement::apply_selector_flags(…
53b2453 
…)` (servo#42963)

This changes `Element::selector_flags` from `Cell<ElementSelectorFlags>`
to `AtomicUsize`, thus avoiding the risk of data races.

Testing: Tested manually. We don't run tests using TSAN builds.
Fixes: servo#37495

Signed-off-by: Oriol Brufau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrobinson mrobinson mrobinson approved these changes

@gterzian gterzian Awaiting requested review from gterzian gterzian is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TSAN: Reported datarace in ServoLayoutElement::apply_selector_flags

4 participants

@Loirooriol @mrobinson @codecov-commenter @servo-highfive