1335305 - stylo: nsAttrValue::ToString mutates |this| in the eCSSDeclaration case

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

Discovered by the static analysis in bug 1294915. This isn't threadsafe:

http://searchfox.org/mozilla-central/rev/f31f538622d0dfb2d869d4babc4951f6001e9be2/dom/base/nsAttrValue.cpp#650

Comment 1

[Bobby Holley (:bholley)]

Looks like this optimization was added in bug 582228, so presumably this caching is pretty important. However, I _think_ it's mostly important in the main-thread case when we're doing getAttribute("style"). It's technically possible to reach this path during selector matching for attribute selectors, but I doubt the perf is critical there.

I'll attach a patch.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Don't cache stringifications for CSS declaration blocks during parallel traversal. v1

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8831939 [details] [diff] [review]
Don't cache stringifications for CSS declaration blocks during parallel traversal. v1

r=me, though this is only OK because really no one ever matches on "style" attributes... and there is no way to wildcard-match on attribute name.

Comment 4

[Bobby Holley (:bholley)]

Actually, I think it's worth introducing an explicit flag that we can just set a global flag when we kick over to the parallel servo traversal. That way we can check for this case as needed without worrying about the TLS hit.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Actually, isn't the first selector-match always done with exclusive access to the element? Subsequent ones during the first restyle would only be in the same thread for the style sharing cache, and we don't bother caching elements with style attribute anyway.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Actually, isn't the first selector-match always done with exclusive access
> to the element? Subsequent ones during the first restyle would only be in
> the same thread for the style sharing cache, and we don't bother caching
> elements with style attribute anyway.

Yes, but we have no way of enforcing this invariant, so I don't want to depend on it in Gecko.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3)
> Comment on attachment 8831939 [details] [diff] [review]
> Don't cache stringifications for CSS declaration blocks during parallel
> traversal. v1
> 
> r=me, though this is only OK because really no one ever matches on "style"
> attributes... and there is no way to wildcard-match on attribute name.

I'm updating this to use the flag in bug 1335319, which should eliminate the TLS overhead here.

Comment 8

[Bobby Holley (:bholley)]

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

Comment 9

[Bobby Holley (:bholley)]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2869274f6532
Don't cache stringifications for CSS declaration blocks during parallel traversal. r=bz

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2869274f6532