Bug 105066

Created attachment 179551 [details] Patch

Created attachment 179554 [details] Patch

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?

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

Created attachment 181193 [details] Tackle shadow dom too

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

Created attachment 181202 [details] Patch

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.

Created attachment 181203 [details] Patch for landing

Comment on attachment 181203 [details] Patch for landing Clearing flags on attachment: 181203 Committed r138730: <http://trac.webkit.org/changeset/138730>

All reviewed patches have been landed. Closing bug.

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

(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 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?

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

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.

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