1340341 - stylo: side-effect-y SetImmutable called when accessing attributes during servo traversal

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

Static analysis found this in bug 1294915 comment 43. It's almost a benign race, except there's no such thing as a benign race. :-)

> Error: Field write mozilla::DeclarationBlock.mImmutable
> Location: void mozilla::DeclarationBlock::SetImmutable() const @
> ff-dbg/dist/include/mozilla/DeclarationBlock.h:66
> Stack Trace:
> void mozilla::css::Declaration::ToString(nsAString_internal*) const @
> layout/style/Declaration.cpp:1706 ### SafeArguments: arg0
> void mozilla::DeclarationBlock::ToString(nsAString_internal*) const @
> ff-dbg/dist/include/mozilla/DeclarationBlockInlines.h:58 ### SafeArguments:
> arg0
> void nsAttrValue::ToString(nsAString_internal*) const @
> dom/base/nsAttrValue.cpp:649 ### SafeArguments: arg0
> uint8 nsAttrValue::Equals(nsIAtom*, uint32) const @
> dom/base/nsAttrValue.cpp:1135
> ServoBindings.cpp:uint8 AttrEquals(nsAttrValue*)::<lambda(const
> nsAttrValue*)> [with Implementor = const mozilla::dom::Element] @
> layout/style/ServoBindings.cpp:451 ### SafeArguments: this
> ServoBindings.cpp:uint8 DoMatch(mozilla::dom::Element*, nsIAtom*, nsIAtom*,
> struct AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with
> Implementor = const mozilla::dom::Element]::<lambda(const class
> nsAttrValue*)>) [with Implementor = const mozilla::dom::Element; MatchFn =
> AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with
> Implementor = const mozilla::dom::Element]::<lambda(const nsAttrValue*)>] @
> layout/style/ServoBindings.cpp:430
> ServoBindings.cpp:uint8 AttrEquals(mozilla::dom::Element*, nsIAtom*,
> nsIAtom*, nsIAtom*, uint8) [with Implementor = const mozilla::dom::Element]
> @ layout/style/ServoBindings.cpp:453
> Gecko_AttrEquals @ layout/style/ServoBindings.cpp:635

Comment 1

[Bobby Holley (:bholley)]

Looks like the SetImmutable call was added in bug 338679. I read through the bug but don't fully understand why we need it. Xidorn, do you understand why? If not, we can ask bz.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

So the reason is that, the change wants to ensure that when the style data is touched by CSSOM, DOMAttrModified event still have the correct prevValue reported. The idea is that, by setting the declaration immutable, CSSOM access would need to create a clone of the old declaration, and thus when CSSOM code eventually calls into nsStyledElement::SetInlineStyleDeclaration, the latter still have a chance to serialize the previous declaration.

But that fix is basically broken. It relies on that the declaration has already been set immutable before the first dispatch of DOMAttrModified event. There are still ways to bypass that and have a wrong prevValue returned.

I think the correct solution would be setting immutable inside nsDOMCSSAttributeDeclaration::GetCSSDeclaration, and it probably doesn't make much sense to set immutable in Declaration::ToString.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1340341 - Set immutablity in GetCSSDeclaration instead of ToString.

Review commit: https://reviewboard.mozilla.org/r/113360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113360/

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

If dbaron is too busy... bz, could you review this patch? It seems you reviewed the patch in bug 338679.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838424 [details]
Bug 1340341 - Set immutablity in GetCSSDeclaration instead of ToString.

https://reviewboard.mozilla.org/r/113360/#review116636

I think the "XXXz" comment about the "old rule" in nsStyledElement::SetInlineStyleDeclaration can go away.

r=me.

::: layout/style/nsDOMCSSAttrDeclaration.cpp:125
(Diff revision 1)
>                                       nullptr);
>    }
>  
>    if (declaration) {
> +    if (aOperation != eOperation_Read &&
> +        nsContentUtils::HasMutationListeners(

I really wish we could do this just once instead of twice (here and in nsStyledElement::SetInlineStyleDeclaration).  But I'm not quite sure how to manage that.  :(

::: layout/style/test/test_style_attr_listener.html:14
(Diff revision 1)
> +<div style=""></div>
> +<script>
> +let div = document.querySelector('div');
> +let expectation;
> +let count = 0;
> +div.style.color = "red";

Hmm.  So the existing test missed this case because it always had the decl immutable from either style matching or the previous mutation event?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #5)
> Comment on attachment 8838424 [details]
> Hmm.  So the existing test missed this case because it always had the decl
> immutable from either style matching or the previous mutation event?

Inline style from the HTML property goes to the cache of nsHTMLCSSStyleSheet directly during parsing, where it is set immutable. See https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/dom/base/nsAttrValue.cpp#1743

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6ca148fc2a
Set immutablity in GetCSSDeclaration instead of ToString. r=bz

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b6ca148fc2a