WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
105066
[HTMLTemplateElement] Disallow cycles within template content
https://bugs.webkit.org/show_bug.cgi?id=105066
Summary
[HTMLTemplateElement] Disallow cycles within template content
Adam Klein
Reported
2012-12-14 16:05:27 PST
[HTMLTemplateElement] Disallow cycles within template content
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Adam Klein
Comment 1
2012-12-14 16:13:11 PST
Created
attachment 179551
[details]
Patch
Adam Klein
Comment 2
2012-12-14 16:35:12 PST
Created
attachment 179554
[details]
Patch
Eric Seidel (no email)
Comment 3
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?
Elliott Sprehn
Comment 4
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.
Adam Klein
Comment 5
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.
Adam Klein
Comment 6
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.
Adam Klein
Comment 7
2013-01-03 11:20:51 PST
Created
attachment 181193
[details]
Tackle shadow dom too
Adam Klein
Comment 8
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).
Build Bot
Comment 9
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
Ojan Vafai
Comment 10
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.
Adam Klein
Comment 11
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.
Adam Klein
Comment 12
2013-01-03 12:19:27 PST
Created
attachment 181202
[details]
Patch
Ojan Vafai
Comment 13
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.
Adam Klein
Comment 14
2013-01-03 12:27:19 PST
Created
attachment 181203
[details]
Patch for landing
WebKit Review Bot
Comment 15
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
>
WebKit Review Bot
Comment 16
2013-01-03 13:18:23 PST
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 17
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); ^
Adam Klein
Comment 18
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
Dominic Cooney
Comment 19
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?
Shinya Kawanaka
Comment 20
2013-01-06 20:56:49 PST
***
Bug 105474
has been marked as a duplicate of this bug. ***
Adam Klein
Comment 21
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.
Hajime Morrita
Comment 22
2013-01-23 00:42:25 PST
***
Bug 103214
has been marked as a duplicate of this bug. ***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug