Bug 104040 – DOMImplementation::createDocument should call appendChild rather than parserAppendChild to add docType and documentElement

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Rafael Weinstein]

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.

[Rafael Weinstein]

Created attachment 177551 [details]
Patch

[Rafael Weinstein]

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).

[Elliott Sprehn]

(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).

[Rafael Weinstein]

Created attachment 177616 [details]
Patch

[Rafael Weinstein]

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? =-)

[Elliott Sprehn]

(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);

[Rafael Weinstein]

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.

[Rafael Weinstein]

Created attachment 177783 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177783 [details]
Patch for landing

Clearing flags on attachment: 177783

Committed r136717: <http://trac.webkit.org/changeset/136717>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> 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.

[Eric Seidel (no email)]

Comment on attachment 177783 [details]
Patch for landing

I'm surprised this is not observable.  Do these cause JS execution (for synchronous mutation events?)