Closed Bug 1333183 Opened 3 years ago Closed 3 years ago

stylo: nsIDocument::GetRootElement is not thread-safe

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file)

There's a trivial little cache (mCachedRootElement), which bhackett found in his static analysis in bug 1294915.

I think the best thing to do is just to always call GetRootElement right before Servo_TraverseSubtree, and then we can assert that stylo always gets a cache hit, since the DOM is never mutated during the Servo traversal.

Should be a trivial patch, but will collide with the code in bug 1331294, so I'll block on that to avoid bitrotting heycam.
Note that the HasPendingRestyles() call already checks for the root element, so we're probably pretty safe here already.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> Note that the HasPendingRestyles() call already checks for the root element,
> so we're probably pretty safe here already.

Yeah, I just want to make it explicit, right before the traversal. The call should be pretty much free if there's a cache hit.
Manish added the Servo_TraverseSubtree choke point, so this is unblocked now. I'll write a patch.
Assignee: nobody → bobbyholley
No longer depends on: 1331294
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> Note that the HasPendingRestyles() call already checks for the root element,
> so we're probably pretty safe here already.

(Also, there's still the StyleNewChildren calls that we do during frame construction when adding NAC).
Comment on attachment 8833070 [details] [diff] [review]
Prime the root element cache before the servo traversal. v1

Review of attachment 8833070 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSet.cpp
@@ +194,5 @@
>  
> +  // Get the Document's root element to ensure that the cache is valid before
> +  // calling into the (potentially-parallel) Servo traversal, where a cache hit
> +  // is necessary to avoid a data race when updating the cache.
> +  (void) aRoot->OwnerDoc()->GetRootElement();

mozilla::Unused << ?
Attachment #8833070 - Flags: review?(emilio+bugs) → review+
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed7112dfd385
Prime the root element cache before the servo traversal. r=emilio
https://hg.mozilla.org/mozilla-central/rev/ed7112dfd385
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1415021
You need to log in before you can comment on or make changes to this bug.