Bug 105066 - [HTMLTemplateElement] Disallow cycles within template content
Summary: [HTMLTemplateElement] Disallow cycles within template content
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
: 103214 105474 (view as bug list)
Depends on:
Blocks: 103547
  Show dependency treegraph
 
Reported: 2012-12-14 16:05 PST by Adam Klein
Modified: 2013-01-23 00:42 PST (History)
11 users (show)

See Also:


Attachments
Patch (14.82 KB, patch)
2012-12-14 16:13 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (20.26 KB, patch)
2012-12-14 16:35 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Tackle shadow dom too (26.21 KB, patch)
2013-01-03 11:20 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (26.95 KB, patch)
2013-01-03 12:19 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (26.77 KB, patch)
2013-01-03 12:27 PST, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2012-12-14 16:05:27 PST
[HTMLTemplateElement] Disallow cycles within template content
Comment 1 Adam Klein 2012-12-14 16:13:11 PST
Created attachment 179551 [details]
Patch
Comment 2 Adam Klein 2012-12-14 16:35:12 PST
Created attachment 179554 [details]
Patch
Comment 3 Eric Seidel (no email) 2012-12-18 12:17:49 PST
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?
Comment 4 Elliott Sprehn 2012-12-18 12:21:56 PST
(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.
Comment 5 Adam Klein 2012-12-18 12:36:43 PST
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.
Comment 6 Adam Klein 2012-12-18 14:57:04 PST
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.
Comment 7 Adam Klein 2013-01-03 11:20:51 PST
Created attachment 181193 [details]
Tackle shadow dom too
Comment 8 Adam Klein 2013-01-03 11:23:04 PST
+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).
Comment 9 Build Bot 2013-01-03 12:00:31 PST
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
Comment 10 Ojan Vafai 2013-01-03 12:10:50 PST
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.
Comment 11 Adam Klein 2013-01-03 12:18:51 PST
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.
Comment 12 Adam Klein 2013-01-03 12:19:27 PST
Created attachment 181202 [details]
Patch
Comment 13 Ojan Vafai 2013-01-03 12:22:52 PST
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.
Comment 14 Adam Klein 2013-01-03 12:27:19 PST
Created attachment 181203 [details]
Patch for landing
Comment 15 WebKit Review Bot 2013-01-03 13:18:18 PST
Comment on attachment 181203 [details]
Patch for landing

Clearing flags on attachment: 181203

Committed r138730: <http://trac.webkit.org/changeset/138730>
Comment 16 WebKit Review Bot 2013-01-03 13:18:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Ryosuke Niwa 2013-01-03 13:44:08 PST
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);
    ^
Comment 18 Adam Klein 2013-01-03 13:45:45 PST
(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
Comment 19 Dominic Cooney 2013-01-06 18:12:33 PST
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?
Comment 20 Shinya Kawanaka 2013-01-06 20:56:49 PST
*** Bug 105474 has been marked as a duplicate of this bug. ***
Comment 21 Adam Klein 2013-01-07 13:26:40 PST
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.
Comment 22 Hajime Morrita 2013-01-23 00:42:25 PST
*** Bug 103214 has been marked as a duplicate of this bug. ***