1333183 - stylo: nsIDocument::GetRootElement is not thread-safe

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Note that the HasPendingRestyles() call already checks for the root element, so we're probably pretty safe here already.

Comment 2

[Bobby Holley (:bholley)]

(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.

Comment 3

[Bobby Holley (:bholley)]

Manish added the Servo_TraverseSubtree choke point, so this is unblocked now. I'll write a patch.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Prime the root element cache before the servo traversal. v1

Comment 5

[Bobby Holley (:bholley)]

(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 6

[Emilio Cobos Álvarez (:emilio)]

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 << ?

Comment 7

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98924c761ac4cda411cc07c992fbddb37081ed6d

Comment 8

[Bobby Holley (:bholley)]

And an m-c try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eceeed6f236ca1cf2b34c4dfc7e07ae4552bab4a

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed7112dfd385
Prime the root element cache before the servo traversal. r=emilio

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed7112dfd385