Bug 105066 – [HTMLTemplateElement] Disallow cycles within template content

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

[HTMLTemplateElement] Disallow cycles within template content

[Adam Klein]

Created attachment 179551 [details]
Patch

[Adam Klein]

Created attachment 179554 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 179554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179554&action=review

> Source/WebCore/ChangeLog:15
> +        In order to disallow cycles, the HTMLTemplateElement.content
> +        DocumentFragment needs a pointer to its host. The approach here
> +        creates a new subclass with a host pointer and a new virtual method
> +        to DocumentFragment to identify the subclass.

So overall this patch looks OK.  Could you speak more about the decision to use a subclass instead of just augmenting DocumentFragment?

[Elliott Sprehn]

(In reply to comment #3)
> (From update of attachment 179554 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179554&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        In order to disallow cycles, the HTMLTemplateElement.content
> > +        DocumentFragment needs a pointer to its host. The approach here
> > +        creates a new subclass with a host pointer and a new virtual method
> > +        to DocumentFragment to identify the subclass.
> 
> So overall this patch looks OK.  Could you speak more about the decision to use a subclass instead of just augmenting DocumentFragment?

I think we should just add the pointer to DocumentFragment and not have the "virtual avoidance" problem. isTemplateContents()  can then be m_host != 0.

[Adam Klein]

Comment on attachment 179554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179554&action=review

>>> Source/WebCore/ChangeLog:15
>>> +        to DocumentFragment to identify the subclass.
>> 
>> So overall this patch looks OK.  Could you speak more about the decision to use a subclass instead of just augmenting DocumentFragment?
> 
> I think we should just add the pointer to DocumentFragment and not have the "virtual avoidance" problem. isTemplateContents()  can then be m_host != 0.

Just trying to avoid adding memory footprint where it's not needed. But the obvious counter-argument is that there aren't that many DocumentFragments in the world.

[Adam Klein]

Comment on attachment 179554 [details]
Patch

After more reading of the existing code and chatting with esprehn on IRC I'm inclined to try to combine this with ShadowRoot's similar need, hopefully in a way that doesn't further confuse what a "host" is.

[Adam Klein]

Created attachment 181193 [details]
Tackle shadow dom too

[Adam Klein]

+dominicc, who's with me that a subclass is useful here.

I've also updated the patch to tackle shadow dom at the same time. I'd be fine with delaying that shadow dom related change until a subsequent patch, but it seems small and is corner-case-y enough that I'm not worried about it breaking anything (especially since my shadow dom test case currently crashes the browser).

[Build Bot]

Comment on attachment 181193 [details]
Tackle shadow dom too

Attachment 181193 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15627958

[Ojan Vafai]

Comment on attachment 181193 [details]
Tackle shadow dom too

View in context: https://bugs.webkit.org/attachment.cgi?id=181193&action=review

> Source/WebCore/dom/ContainerNode.cpp:169
>          if (newChild->contains(newParent))
>              return HIERARCHY_REQUEST_ERR;
> +        if (needsHostElementContainmentCheck(newParent) && newChild->containsIncludingHostElements(newParent))

Can we only call the appropriate contains call instead of walking up the tree twice?

> Source/WebCore/dom/Node.cpp:1078
> +            node = node->parentOrHostNode();

Seems like we need to rename this to parentOrShadowHostNode. Fine to do so as a separate patch, but a FIXME as part of this one would be nice.

[Adam Klein]

Comment on attachment 181193 [details]
Tackle shadow dom too

View in context: https://bugs.webkit.org/attachment.cgi?id=181193&action=review

>> Source/WebCore/dom/ContainerNode.cpp:169
>> +        if (needsHostElementContainmentCheck(newParent) && newChild->containsIncludingHostElements(newParent))
> 
> Can we only call the appropriate contains call instead of walking up the tree twice?

Done, moved into another helper function.

>> Source/WebCore/dom/Node.cpp:1078
>> +            node = node->parentOrHostNode();
> 
> Seems like we need to rename this to parentOrShadowHostNode. Fine to do so as a separate patch, but a FIXME as part of this one would be nice.

Added a FIXME to Node.h.

[Adam Klein]

Created attachment 181202 [details]
Patch

[Ojan Vafai]

Comment on attachment 181202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181202&action=review

> Source/WebCore/dom/ContainerNode.cpp:155
> +static inline bool needsHostElementContainmentCheck(const Node* node)
> +{
> +    return node->isInShadowTree() || isInTemplateContent(node);
> +}

Nit: we don't need this anymore.

[Adam Klein]

Created attachment 181203 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 181203 [details]
Patch for landing

Clearing flags on attachment: 181203

Committed r138730: <http://trac.webkit.org/changeset/138730>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This broke Mac builds:

http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20%28Build%29/builds/6662/steps/compile-webkit/logs/stdio

/Volumes/Data/slave/mountainlion-debug/build/Source/WebCore/dom/ContainerNode.cpp:147:5: error: use of undeclared identifier 'UNUSED'
    UNUSED(node);
    ^

[Adam Klein]

(In reply to comment #17)
> This broke Mac builds:
> 
> http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20%28Build%29/builds/6662/steps/compile-webkit/logs/stdio
> 
> /Volumes/Data/slave/mountainlion-debug/build/Source/WebCore/dom/ContainerNode.cpp:147:5: error: use of undeclared identifier 'UNUSED'
>     UNUSED(node);
>     ^

Sorry for the breakage, already fixed in http://trac.webkit.org/changeset/138731

[Dominic Cooney]

Comment on attachment 181193 [details]
Tackle shadow dom too

View in context: https://bugs.webkit.org/attachment.cgi?id=181193&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:5904
> +		C65046A9167BFB5500CC2A4D /* TemplateContentDocumentFragment.h in Headers */ = {isa = PBXBuildFile; fileRef = C65046A8167BFB5500CC2A4D /* TemplateContentDocumentFragment.h */; };

Sort xcode proj?

> Source/WebCore/dom/Document.h:1195
> +    const Document* templateContentsOwnerDocument() const;

Why is this const Document*?

> Source/WebCore/dom/Node.cpp:1062
> +    for (; node; node = node->parentOrHostNode()) {

Neat.

> LayoutTests/fast/dom/HTMLTemplateElement/cycles-expected.txt:6
> +PASS template.content.appendChild(template) threw exception Error: HierarchyRequestError: DOM Exception 3.

I assume appendChild, etc. that does not throw HierarchyRequestError is covered by other tests?

[Shinya Kawanaka]

*** Bug 105474 has been marked as a duplicate of this bug. ***

[Adam Klein]

Comment on attachment 181193 [details]
Tackle shadow dom too

View in context: https://bugs.webkit.org/attachment.cgi?id=181193&action=review

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:5904
>> +		C65046A9167BFB5500CC2A4D /* TemplateContentDocumentFragment.h in Headers */ = {isa = PBXBuildFile; fileRef = C65046A8167BFB5500CC2A4D /* TemplateContentDocumentFragment.h */; };
> 
> Sort xcode proj?

This part of the file is sorted by the hash thingy, which I think is working as intended.

>> Source/WebCore/dom/Document.h:1195
>> +    const Document* templateContentsOwnerDocument() const;
> 
> Why is this const Document*?

Because sometimes it returns |this|, and since it's a const method, |this| is a const Document*. Could const_cast that away, but it didn't seem necessary.

>> LayoutTests/fast/dom/HTMLTemplateElement/cycles-expected.txt:6
>> +PASS template.content.appendChild(template) threw exception Error: HierarchyRequestError: DOM Exception 3.
> 
> I assume appendChild, etc. that does not throw HierarchyRequestError is covered by other tests?

Huh, looks like we could use more tests of that sort, indeed. Gave myself a todo item.

[Hajime Morrita]

*** Bug 103214 has been marked as a duplicate of this bug. ***