1419322 - Track WPT failure at custom-elements/attribute-changed-callback.html

Status: RESOLVED DUPLICATE of bug 1428246

(Core :: DOM: Core & HTML, defect, P2)

Description

[John Dai[:jdai]]

There is a test failure at attribute-changed-callback.html[1][2][3].

[1] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/testing/web-platform/tests/custom-elements/attribute-changed-callback.html
[2] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/testing/web-platform/meta/custom-elements/attribute-changed-callback.html.ini
[3] http://w3c-test.org/custom-elements/attribute-changed-callback.html

Comment 1

[John Dai[:jdai]]

Somehow we put attribute changed callback into BackupQueue when we have style attribute changed.

Comment 2

[Edgar Chen [:edgar]]

(In reply to John Dai[:jdai] from comment #1)
> Somehow we put attribute changed callback into BackupQueue when we have
> style attribute changed.

Yeah, it looks like we are missing some [CEReactions] in CSSStyleDeclaration, https://drafts.csswg.org/cssom/#the-cssstyledeclaration-interface.

Comment 3

[John Dai[:jdai]]

It seems like we auto-generate style attribute from GenerateCSS2PropertiesWebIDL.py[1]

[1] https://searchfox.org/mozilla-central/source/dom/bindings/GenerateCSS2PropertiesWebIDL.py

Comment 4

[John Dai[:jdai]]

After adding [CEReactions] in GenerateCSS2PropertiesWebIDL.py, I got two attribute changed callbacks if there is only a inline style attribute change, however, it is wrong. It is because when we do nsDOMCSSDeclaration::ModifyDeclaration, it will run GetCSSDeclaration[1] and SetCSSDeclaration[2], both will call SetInlineStyleDeclaration[3] which will call SetAttrAndNotify to enqueue attribute change. 


[1] https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/layout/style/nsDOMCSSDeclaration.cpp#304
[2] https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/layout/style/nsDOMCSSDeclaration.cpp#340
[3] https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/layout/style/nsDOMCSSAttrDeclaration.cpp#80,156

Comment 5

[John Dai[:jdai]]

Attachment: Bug 1419322 - Part1: Add CEReactions in CSSStyleDeclaration.

This patch adds [CEReactions] in CSSStyleDeclaration and fixed comment 4 issue, so it can file correct number of attribute change callback. However, the newValue in the attribute change callback is wrong. Ex:

var instance = document.createElement('element-with-style-attribute-observation');
instance.style.fontSize = '10px';
assert_attribute_log_entry(calls[0], {name: 'style', oldValue: null, newValue: 'font-size: 10px;', namespace: null});

newValue is empty string in this case.

Comment 6

[Edgar Chen [:edgar]]

Comment on attachment 8934431 [details] [diff] [review]
Bug 1419322 - Part1: Add CEReactions in CSSStyleDeclaration.

Review of attachment 8934431 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Element.cpp
@@ +2777,5 @@
>        binding->AttributeChanged(aName, aNamespaceID, false, aNotify);
>      }
>    }
>  
> +  if (aNotify && CustomElementRegistry::IsCustomElementEnabled()) {

This will also filter out the attribute changes triggered from parser, which looks wrong to me.

Comment 7

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #6)
> Comment on attachment 8934431 [details] [diff] [review]
> Bug 1419322 - Part1: Add CEReactions in CSSStyleDeclaration.
> 
> Review of attachment 8934431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/Element.cpp
> @@ +2777,5 @@
> >        binding->AttributeChanged(aName, aNamespaceID, false, aNotify);
> >      }
> >    }
> >  
> > +  if (aNotify && CustomElementRegistry::IsCustomElementEnabled()) {
> 
> This will also filter out the attribute changes triggered from parser, which
> looks wrong to me.

Looks like there is a side-effect. Thanks to notice this! It seems like we have to modify code which changes calling SetInlineStyleDeclaration in one place(not in GetCSSDeclaration and SetCSSDeclaration).

(In reply to John Dai[:jdai] from comment #5)
> Created attachment 8934431 [details] [diff] [review]
> Bug 1419322 - Part1: Add CEReactions in CSSStyleDeclaration.
> 
> This patch adds [CEReactions] in CSSStyleDeclaration and fixed comment 4
> issue, so it can file correct number of attribute change callback. However,
> the newValue in the attribute change callback is wrong.
> 
It looks like we use kDontCallAfterSetAttr[1], so newValue can not be updated via aParsedValue[2]. Change to kCallAfterSetAttr can fix this issue. Need to do more test to make sure there is no side-effect.

[1] https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/dom/base/nsStyledElement.cpp#115
[2] https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/dom/base/Element.cpp#2725-2728,2794

Comment 8

[Edgar Chen [:edgar]]

(In reply to John Dai[:jdai] from comment #7)
> Looks like there is a side-effect. Thanks to notice this!
 
It might be worth to add a wpt to test the attributeChangedCallback which is triggered from parser.

Comment 9

[Edgar Chen [:edgar]]

(In reply to John Dai[:jdai] from comment #7)
> Change to kCallAfterSetAttr can fix this issue.

Changing to kCallAfterSetAttr will cause AfterSetAttr be called for inline style change, it seems not necessary; subclasses don't have anything need to deal with the inline style attribute change, right?

Comment 10

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #8)
> (In reply to John Dai[:jdai] from comment #7)
> > Looks like there is a side-effect. Thanks to notice this!
>  
> It might be worth to add a wpt to test the attributeChangedCallback which is
> triggered from parser.

Sure. Will do. Thank you.

(In reply to Edgar Chen [:edgar] from comment #9)
> (In reply to John Dai[:jdai] from comment #7)
> > Change to kCallAfterSetAttr can fix this issue.
> 
> Changing to kCallAfterSetAttr will cause AfterSetAttr be called for inline
> style change, it seems not necessary; subclasses don't have anything need to
> deal with the inline style attribute change, right?

Oops, I'll come out with another solution without changing kDontCallAfterSetAttr. Thanks for your information.

Comment 11

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #7)
> (In reply to Edgar Chen [:edgar] from comment #6)
> > Comment on attachment 8934431 [details] [diff] [review]
> > Bug 1419322 - Part1: Add CEReactions in CSSStyleDeclaration.
> > 
> > Review of attachment 8934431 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/base/Element.cpp
> > @@ +2777,5 @@
> > >        binding->AttributeChanged(aName, aNamespaceID, false, aNotify);
> > >      }
> > >    }
> > >  
> > > +  if (aNotify && CustomElementRegistry::IsCustomElementEnabled()) {
> > 
> > This will also filter out the attribute changes triggered from parser, which
> > looks wrong to me.
> 
> Looks like there is a side-effect. Thanks to notice this! It seems like we
> have to modify code which changes calling SetInlineStyleDeclaration in one
> place(not in GetCSSDeclaration and SetCSSDeclaration).
> 
It will get crash if we don't do SetInlineStyleDeclaration in GetCSSDeclaration, the crash point at olddecl->EnsureMutable when cloning a DeclarationBlock[2]. Seems like we can't skip mAttrsAndChildren.SetAndSwapAttr[3]. I'm not familiar with this code, still digging to it.


[1] https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/layout/style/nsDOMCSSDeclaration.cpp#315
[2] https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/layout/style/DeclarationBlockInlines.h#59
[3] https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/dom/base/Element.cpp#2744

Comment 12

[John Dai[:jdai]]

I am trying to avoid nsDOMCSSAttributeDeclaration::GetCSSDeclaration call SetInlineStyleDeclaration, because it'll trigger an attribute change. So, if I mark mAttrsAndChildren.SetAndSwapAttr[1] which is called by SetInlineStyleDeclaration and inside Element::SetAttrAndNotify, I got a crash.

[1] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/base/Element.cpp#2744

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x00000001043693dc XUL`nsAttrValue::BaseType(this=0x0000000000000000) const at nsAttrValue.h:510
    frame #1: 0x0000000104352d59 XUL`nsAttrValue::Type(this=0x0000000000000000) const at nsAttrValueInlines.h:242
    frame #2: 0x0000000104352f6d XUL`nsAttrValue::GetAtomValue(this=0x0000000000000000) const at nsAttrValueInlines.h:261
    frame #3: 0x000000010448e2b9 XUL`nsIContent::DoGetID(this=0x0000000125cb1280) const at Element.cpp:204
  * frame #4: 0x0000000104496aa5 XUL`mozilla::dom::Element::BindToTree(this=0x0000000125cb1280, aDocument=0x0000000125c7b800, aParent=0x0000000125cb1160, aBindingParent=0x0000000000000000, aCompileEventHandlers=true) at Element.cpp:1850
    frame #5: 0x0000000104691da6 XUL`nsINode::doInsertChildAt(this=0x0000000125cb1160, aKid=0x0000000125cb1280, aIndex=1, aNotify=false, aChildArray=0x0000000125cb11c8) at nsINode.cpp:1627
    frame #6: 0x00000001044b8bc8 XUL`mozilla::dom::FragmentOrElement::InsertChildAt(this=0x0000000125cb1160, aKid=0x0000000125cb1280, aIndex=1, aNotify=false) at FragmentOrElement.cpp:1347
    frame #7: 0x000000010393c72f XUL`nsINode::AppendChildTo(this=0x0000000125cb1160, aKid=0x0000000125cb1280, aNotify=false) at nsINode.h:744
    frame #8: 0x0000000106bb6eba XUL`nsXMLContentSink::HandleStartElement(this=0x00000001269ac000, aName=u"http://www.mozilla.org/2006/addons-blocklist\U0000ffffemItem", aAtts=0x00000001266a2400, aAttsCount=2, aLineNumber=4, aInterruptable=true) at nsXMLContentSink.cpp:990
    frame #9: 0x0000000106bb6849 XUL`nsXMLContentSink::HandleStartElement(this=0x00000001269ac000, aName=u"http://www.mozilla.org/2006/addons-blocklist\U0000ffffemItem", aAtts=0x00000001266a2400, aAttsCount=4, aLineNumber=4) at nsXMLContentSink.cpp:926
    frame #10: 0x0000000106bb7285 XUL`non-virtual thunk to nsXMLContentSink::HandleStartElement(this=0x00000001269ac000, aName=u"http://www.mozilla.org/2006/addons-blocklist\U0000ffffemItem", aAtts=0x00000001266a2400, aAttsCount=4, aLineNumber=4) at nsXMLContentSink.cpp:0
    frame #11: 0x00000001038ad1c4 XUL`nsExpatDriver::HandleStartElement(this=0x0000000123d0cfc0, aValue=u"http://www.mozilla.org/2006/addons-blocklist\U0000ffffemItem", aAtts=0x00000001266a2400) at nsExpatDriver.cpp:324
    frame #12: 0x00000001038b23ad XUL`Driver_HandleStartElement(aUserData=0x0000000123d0cfc0, aName=u"http://www.mozilla.org/2006/addons-blocklist\U0000ffffemItem", aAtts=0x00000001266a2400) at nsExpatDriver.cpp:64
    frame #13: 0x00000001082c3d7c XUL`doContent(parser=0x0000000125c59000, startTagLevel=0, enc=0x00000001111e90a8, s="<", end="����, nextPtr=0x00007fff5fbc3ae8, haveMore='\x01') at xmlparse.c:2442
    frame #14: 0x00000001082c0ef1 XUL`contentProcessor(parser=0x0000000125c59000, start="<", end="����, endPtr=0x00007fff5fbc3ae8) at xmlparse.c:2098
    frame #15: 0x00000001082bdb98 XUL`doProlog(parser=0x0000000125c59000, enc=0x00000001111e90a8, s="<", end="����, tok=29, next="<", nextPtr=0x00007fff5fbc3ae8, haveMore='\x01') at xmlparse.c:4078
    frame #16: 0x00000001082bced3 XUL`prologProcessor(parser=0x0000000125c59000, s="<", end="����, nextPtr=0x00007fff5fbc3ae8) at xmlparse.c:3812
    frame #17: 0x00000001082bcc40 XUL`prologInitProcessor(parser=0x0000000125c59000, s="<", end="����, nextPtr=0x00007fff5fbc3ae8) at xmlparse.c:3629
    frame #18: 0x00000001082bb782 XUL`MOZ_XML_Parse(parser=0x0000000125c59000, s="<", len=554536, isFinal=0) at xmlparse.c:1530
    frame #19: 0x00000001038b0611 XUL`nsExpatDriver::ParseBuffer(this=0x0000000123d0cfc0, aBuffer=u"<?xml version='1.0' encoding='UTF-8'?>\n<blocklist lastupdate=\"1500496563565\" xmlns=\"http://www.mozilla.org/2006/addons-blocklist\">\n  <emItems>\n    <emItem blockID=\"i988\" id=\"{b12785f5-d8d0-4530-a3ea-5c4263b85bef}\">\n      <prefs/>\n      <versionRange minVersion=\"0\" maxVersion=\"*\" severity=\"1\"/>\n    </emItem>\n    <emItem blockID=\"i398\" id=\"{377e5d4d-77e5-476a-8716-7e70a9272da0}\">\n      <prefs/>\n      <versionRange minVersion=\"0\" maxVersion=\"*\" severity=\"1\"/>\n    </emItem>\n    <emItem blockID=\"i698\" id=\"{6b2a75c8-6e2e-4267-b955-43e25b54e575}\">\n      <prefs/>\n      <versionRange minVersion=\"0\" maxVersion=\"*\" severity=\"1\"/>\n    </emItem>\n    <emItem blockID=\"i1231\" id=\"[e-mail]\">\n      <prefs/>\n      <versionRange minVersion=\"0\" maxVersion=\"*\" severity=\"3\"/>\n    </emItem>\n    <emItem blockID=\"i1263\" id=\"[e-mail]\">\n      <prefs/>\n      <versionRange minVersion=\"0\" maxVersion=\"1.1.1\" severity=\"3\"/>\n    </emItem>\n    <emItem blockID=\"i874\" id=\"/^toolbar[0-9]*@findwide\\.com$/\">\n      <prefs", aLength=277268, aIsFinal=false, aConsumed=0x00007fff5fbc3d04) at nsExpatDriver.cpp:887
    frame #20: 0x00000001038b0db4 XUL`nsExpatDriver::ConsumeToken(this=0x0000000123d0cfc0, aScanner=0x00000001266d4450, aFlushTokens=0x00007fff5fbc3fd3) at nsExpatDriver.cpp:985
    frame #21: 0x00000001038b1c94 XUL`non-virtual thunk to nsExpatDriver::ConsumeToken(this=0x0000000123d0cfc0, aScanner=0x00000001266d4450, aFlushTokens=0x00007fff5fbc3fd3) at nsExpatDriver.cpp:0
    frame #22: 0x00000001038b74cb XUL`nsParser::Tokenize(this=0x0000000100636c40, aIsFinalChunk=false) at nsParser.cpp:1539
    frame #23: 0x00000001038b5d53 XUL`nsParser::ResumeParse(this=0x0000000100636c40, allowIteration=true, aIsFinalChunk=false, aCanInterrupt=true) at nsParser.cpp:1056
    frame #24: 0x00000001038b7e2e XUL`nsParser::OnDataAvailable(this=0x0000000100636c40, request=0x0000000100616820, aContext=0x0000000000000000, pIStream=0x00000001006b0f60, sourceOffset=0, aLength=277268) at nsParser.cpp:1437
    frame #25: 0x00000001038b825c XUL`non-virtual thunk to nsParser::OnDataAvailable(this=0x0000000100636c40, request=0x0000000100616820, aContext=0x0000000000000000, pIStream=0x00000001006b0f60, sourceOffset=0, aLength=277268) at nsParser.cpp:0
    frame #26: 0x000000010448159f XUL`mozilla::dom::DOMParser::ParseFromStream(this=0x00000001266d2190, aStream=0x00000001006b0f60, aCharset="UTF-8", aContentLength=277268, aContentType="text/xml", aResult=0x00007fff5fbc4558) at DOMParser.cpp:287
    frame #27: 0x0000000104480716 XUL`mozilla::dom::DOMParser::ParseFromString(this=0x00000001266d2190, str=0x00007fff5fbc4638, contentType="text/xml", aResult=0x00007fff5fbc4558) at DOMParser.cpp:123
    frame #28: 0x0000000104480247 XUL`mozilla::dom::DOMParser::ParseFromString(this=0x00000001266d2190, aStr=0x00007fff5fbc4638, aType=Text_xml, rv=0x00007fff5fbc45e8) at DOMParser.cpp:63
    frame #29: 0x000000010543139e XUL`mozilla::dom::DOMParserBinding::parseFromString(cx=0x0000000123e27000, obj=Handle<JSObject *> @ 0x00007fff5fbc46e0, self=0x00000001266d2190, args=0x00007fff5fbc4748) at DOMParserBinding.cpp:75
    frame #30: 0x000000010581a612 XUL`mozilla::dom::GenericBindingMethod(cx=0x0000000123e27000, argc=2, vp=0x0000000125b1ddf0) at BindingUtils.cpp:3042
    frame #31: 0x000000010a657674 XUL`js::CallJSNative(cx=0x0000000123e27000, native=(XUL`mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) at BindingUtils.cpp:3015), args=0x00007fff5fbc6b48)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) at jscntxtinlines.h:291
    frame #32: 0x000000010a6573ac XUL`js::InternalCallOrConstruct(cx=0x0000000123e27000, args=0x00007fff5fbc6b48, construct=NO_CONSTRUCT) at Interpreter.cpp:473
    frame #33: 0x000000010a657b4d XUL`InternalCall(cx=0x0000000123e27000, args=0x00007fff5fbc6b48) at Interpreter.cpp:522
    frame #34: 0x000000010a65794d XUL`js::CallFromStack(cx=0x0000000123e27000, args=0x00007fff5fbc6b48) at Interpreter.cpp:528
    frame #35: 0x000000010a64c18c XUL`Interpret(cx=0x0000000123e27000, state=0x00007fff5fbc7c20) at Interpreter.cpp:3096
    frame #36: 0x000000010a6410b7 XUL`js::RunScript(cx=0x0000000123e27000, state=0x00007fff5fbc7c20) at Interpreter.cpp:423
    frame #37: 0x000000010a65750a XUL`js::InternalCallOrConstruct(cx=0x0000000123e27000, args=0x00007fff5fbc7ff0, construct=NO_CONSTRUCT) at Interpreter.cpp:495
    frame #38: 0x000000010a657b4d XUL`InternalCall(cx=0x0000000123e27000, args=0x00007fff5fbc7ff0) at Interpreter.cpp:522
    frame #39: 0x000000010a65794d XUL`js::CallFromStack(cx=0x0000000123e27000, args=0x00007fff5fbc7ff0) at Interpreter.cpp:528
    frame #40: 0x000000010a78d0ca XUL`js::jit::DoCallFallback(cx=0x0000000123e27000, frame=0x00007fff5fbc83e8, stub_=0x0000000126a9ef80, argc=1, vp=0x00007fff5fbc8360, res=JS::MutableHandleValue @ 0x00007fff5fbc80b0) at BaselineIC.cpp:2559
    frame #41: 0x000012b8b36e20e6

Comment 13

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #8) 
> It might be worth to add a wpt to test the attributeChangedCallback which is
> triggered from parser.

Filed bug 1425079.

Comment 14

[Edgar Chen [:edgar]]

We have three issues on attributeChangedCallback for style attribute,
1). We are missing [CEReactions] annotation on the attributes of CSSStyleDeclaration [1].
2). The attributeChangedCallback for style attribute is always fired with null as oldValue and empty string as newValue [2].
3). There are two attributeChangedCallback fired for the *first* style attribute change [2]. 

[1] https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-camel-cased-attribute 
[2] https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5663

The fixes for 1) and 2) are trivial.
3) is a hard one, as mention in comment #4, the additional attributeChangedCallback is from GetCSSDeclaration() call with eOperation_Modify mode. In eOperation_Modify mode, if we don't have existing InlineStyleDeclaration, it will create a new CSSDeclaration and transfer its ownership to nsAttrValue, then call SetAttrAndNotify() which trigger an attributeChangedCallback. Hunting down this additional attributeChangedCallback will probably require changing all these stuff.

On a second thought: After we fix 1 and 2, the additional attributeChangedCallback seems not cause much harm given that
- The additional callback only shows on *first* style attribute change.
- We eventually fire attributeChangedCallback with correct value.

So I am going to,
- Fire a separated bug to fix 1) and 2).
- Fire a separated bug to track 3).
- Leave this bug to track the wpt failure which will require all 1)~3) fixes.

Comment 15

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #14)
> - Fire a separated bug to fix 1) and 2).
Filed bug 1428244.

> - Fire a separated bug to track 3).
Filed bug 1428246.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

What is left here is a dup of bug 1428246