In particular, using parserAppendChild with doctype., breaks an invariant of that function which is the parent and child should already be owned by the same document. Note that using appendChild brings webkit closer to DOM4 by not bypassing the standard work of http://dom.spec.whatwg.org/#concept-node-append.
Created attachment 177551 [details] Patch
Note that this patch also removes treeScopeAdoption from parserAppendChild, which I think has to be safe if the ASSERT is valid (and I'm assuming it is).
(In reply to comment #2) > Note that this patch also removes treeScopeAdoption from parserAppendChild, which I think has to be safe if the ASSERT is valid (and I'm assuming it is). Your assertion is about the document, not the treeScope, so they don't really cover the same things. In particular in a shadow document() != treeScope() so doing document() == newNode->document() can pass but treeScope() == newNode->treeScope() might fail. That said we *also* assert ASSERT(!newChild->parentNode()) which means the only way for newNode->treeScope() to not be newNode->document() would be if newNode was itself a ShadowRoot which never happens (and calling adoptNode wouldn't fix it anyway).
Created attachment 177616 [details] Patch
Actually, it looks like this can happen. If assign markup to innerHTML of a node in the shadow, then the parser will have set the appropriate ownerDocument for the new node, but not the appropriate treeScope. This seems like an expensive way to pull nodes into the treeScope during innerHTML, but I think changing the parser to create the node in the appropriate treeScope may also be overkill because it adds another pointer compare to every node created during parsing. I've put the treeScope adoption back. I'd create a test, but I'm not sure how to detect that nodes created from innerHTML in the shadow have the wrong treeScope. Perhaps one of the component folks can do that? =-)
(In reply to comment #5) > Actually, it looks like this can happen. > > If assign markup to innerHTML of a node in the shadow, then the parser will have set the appropriate ownerDocument for the new node, but not the appropriate treeScope. > > This seems like an expensive way to pull nodes into the treeScope during innerHTML, but I think changing the parser to create the node in the appropriate treeScope may also be overkill because it adds another pointer compare to every node created during parsing. > > I've put the treeScope adoption back. I'd create a test, but I'm not sure how to detect that nodes created from innerHTML in the shadow have the wrong treeScope. Perhaps one of the component folks can do that? =-) That's not how innerHTML works, ShadowRoot::setInnerHTML creates a document fragment and then goes through replaceChildrenWithFragment which does: containerNode->removeChildren(); containerNode->appendChild(fragment, ec);
Right. Forgot that part. In any case, Elliot wants to deal with that call in a follow-on patch, so I'll leave the call alone for now.
Created attachment 177783 [details] Patch for landing
Comment on attachment 177783 [details] Patch for landing Clearing flags on attachment: 177783 Committed r136717: <http://trac.webkit.org/changeset/136717>
All reviewed patches have been landed. Closing bug.
> Your assertion is about the document, not the treeScope, so they don't really cover the same things. In particular in a shadow document() != treeScope() so doing document() == newNode->document() can pass but treeScope() == newNode->treeScope() might fail. As the first version of the patch had a bug that was not caught by EWS, it would be very desirable to add a regression test for that.
Comment on attachment 177783 [details] Patch for landing I'm surprised this is not observable. Do these cause JS execution (for synchronous mutation events?)