Closed Bug 1340341 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bholley, Assigned: xidorn)

References

Details

Attachments

(1 file)

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
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.
Flags: needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)
Assignee: nobody → xidorn+moz
If dbaron is too busy... bz, could you review this patch? It seems you reviewed the patch in bug 338679.
Flags: needinfo?(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?
Attachment #8838424 - Flags: review+
Attachment #8838424 - Flags: review?(dbaron)
(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
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6ca148fc2a
Set immutablity in GetCSSDeclaration instead of ToString. r=bz
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/6b6ca148fc2a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.