stylo: nsIDocument::GetRootElement is not thread-safe

RESOLVED FIXED in Firefox 54

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

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.
Blocks: 1294915
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
Created attachment 8833070 [details] [diff] [review]
Prime the root element cache before the servo traversal. v1
Attachment #8833070 - Flags: review?(emilio+bugs)
(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+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=98924c761ac4cda411cc07c992fbddb37081ed6d
And an m-c try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eceeed6f236ca1cf2b34c4dfc7e07ae4552bab4a

Comment 9

9 months ago
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

Comment 10

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ed7112dfd385
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.