Check documentElement conditions in parser* DOM mutation methods (issue 1163863005 by esprehn@chromium.org)

0 views
Skip to first unread message

esp...@chromium.org

unread,
Jun 2, 2015, 12:25:33 AM6/2/15
to kou...@chromium.org, ad...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, rob....@samsung.com, sigb...@opera.com
Reviewers: kouhei, adamk,

Description:
Check documentElement conditions in parser* DOM mutation methods

The parser should never violate the tree constraints by doing
something like appending multiple documentElements. The spec
says we should just drop these elements on the floor.

BUG=492843

Please review this at https://codereview.chromium.org/1163863005/

Base URL: svn://svn.chromium.org/blink/trunk

Affected files (+24, -7 lines):
M LayoutTests/fast/parser/append-child-followed-by-document-write.html
M
LayoutTests/fast/parser/append-child-followed-by-document-write-expected.txt
M Source/core/dom/ContainerNode.h
M Source/core/dom/ContainerNode.cpp


Index:
LayoutTests/fast/parser/append-child-followed-by-document-write-expected.txt
diff --git
a/LayoutTests/fast/parser/append-child-followed-by-document-write-expected.txt
b/LayoutTests/fast/parser/append-child-followed-by-document-write-expected.txt
index
9f1fa12acdfcb9c2b7eee1842cb2ad89b7c05a59..55c25ecd7aa300276ad7c6ec00a8a56abad9bf12
100644
---
a/LayoutTests/fast/parser/append-child-followed-by-document-write-expected.txt
+++
b/LayoutTests/fast/parser/append-child-followed-by-document-write-expected.txt
@@ -1,3 +1,5 @@
-ALERT: document.firstChild.outerHTML: <a></a>
-document.firstChild.nextSibling.outerHTML: <html><head></head><body><b
id="b"></b></body></html>
+ALERT: document.documentElement.outerHTML: <a></a>
+document.childNodes.length: 1
+b element: null
+

Index: LayoutTests/fast/parser/append-child-followed-by-document-write.html
diff --git
a/LayoutTests/fast/parser/append-child-followed-by-document-write.html
b/LayoutTests/fast/parser/append-child-followed-by-document-write.html
index
01698785f46101c0a9d5324e0f8d401c5aeaaecd..c3d9c5e97b6c3cf1f18b87f1de3056e1e60c736b
100644
--- a/LayoutTests/fast/parser/append-child-followed-by-document-write.html
+++ b/LayoutTests/fast/parser/append-child-followed-by-document-write.html
@@ -8,12 +8,11 @@ function go() {

// Ideally we would use the dump-as-markup test framework for this test,
but
// the contortions we go through here are too tricky for dump-as-markup.
+ // TODO(esprehn): Is this really true?

- var firstChildHTML = document.firstChild.outerHTML;
- var secondChildHTML = document.firstChild.nextSibling.outerHTML;
-
- alert("document.firstChild.outerHTML: " + firstChildHTML + "\n" +
- "document.firstChild.nextSibling.outerHTML: " + secondChildHTML);
+ alert("document.documentElement.outerHTML: " +
document.documentElement.outerHTML + "\n" +
+ "document.childNodes.length: " + document.childNodes.length + "\n"
+
+ "b element: " + b + "\n");
}

window.addEventListener("load", go, false);
Index: Source/core/dom/ContainerNode.cpp
diff --git a/Source/core/dom/ContainerNode.cpp
b/Source/core/dom/ContainerNode.cpp
index
cfbe083d297fa80cef24edb9d062aa92c1ab508d..8cf083c4cb92315fed4af4dc6ddee892dc2c5c8e
100644
--- a/Source/core/dom/ContainerNode.cpp
+++ b/Source/core/dom/ContainerNode.cpp
@@ -168,6 +168,15 @@ bool
ContainerNode::checkAcceptChildGuaranteedNodeTypes(const Node& newChild, Ex
return true;
}

+bool ContainerNode::checkParserAcceptChild(const Node& newChild) const
+{
+ if (!isDocumentNode())
+ return true;
+ // TODO(esprehn): Are there other conditions where the parser can
create
+ // invalid trees?
+ return toDocument(*this).canAcceptChild(newChild, nullptr,
IGNORE_EXCEPTION);
+}
+
PassRefPtrWillBeRawPtr<Node>
ContainerNode::insertBefore(PassRefPtrWillBeRawPtr<Node> newChild, Node*
refChild, ExceptionState& exceptionState)
{
#if !ENABLE(OILPAN)
@@ -299,6 +308,9 @@ void
ContainerNode::parserInsertBefore(PassRefPtrWillBeRawPtr<Node> newChild, No
if (nextChild.previousSibling() == newChild || &nextChild == newChild)
// nothing to do
return;

+ if (!checkParserAcceptChild(*newChild))
+ return;
+
RefPtrWillBeRawPtr<Node> protect(this);

// FIXME: parserRemoveChild can run script which could then insert the
@@ -763,6 +775,9 @@ void
ContainerNode::parserAppendChild(PassRefPtrWillBeRawPtr<Node> newChild)
ASSERT(!newChild->isDocumentFragment());
ASSERT(!isHTMLTemplateElement(this));

+ if (!checkParserAcceptChild(*newChild))
+ return;
+
RefPtrWillBeRawPtr<Node> protect(this);

// FIXME: parserRemoveChild can run script which could then insert the
Index: Source/core/dom/ContainerNode.h
diff --git a/Source/core/dom/ContainerNode.h
b/Source/core/dom/ContainerNode.h
index
f1ed34de65a92b1f6e3074df73b00382ec15ebdd..fb8227dcb01037b4cec4fe7d6b6ed9bcdc4994b6
100644
--- a/Source/core/dom/ContainerNode.h
+++ b/Source/core/dom/ContainerNode.h
@@ -259,6 +259,7 @@ private:

inline bool checkAcceptChildGuaranteedNodeTypes(const Node& newChild,
ExceptionState&) const;
inline bool checkAcceptChild(const Node* newChild, const Node*
oldChild, ExceptionState&) const;
+ inline bool checkParserAcceptChild(const Node& newChild) const;
inline bool containsConsideringHostElements(const Node&) const;
inline bool isChildTypeAllowed(const Node& child) const;



kou...@chromium.org

unread,
Jun 2, 2015, 12:51:23 AM6/2/15
to esp...@chromium.org, ad...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, rob....@samsung.com, sigb...@opera.com
lgtm



https://codereview.chromium.org/1163863005/

'commit-bot: I haz the power' via codereview.chromium.org

unread,
Jun 2, 2015, 1:08:16 AM6/2/15
to esp...@chromium.org, ad...@chromium.org, kou...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, rob....@samsung.com, sigb...@opera.com
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1163863005/1


https://codereview.chromium.org/1163863005/

'commit-bot: I haz the power' via codereview.chromium.org

unread,
Jun 2, 2015, 3:21:13 AM6/2/15
to esp...@chromium.org, ad...@chromium.org, kou...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, rob....@samsung.com, sigb...@opera.com
Try jobs failed on following builders:
linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/64365)

https://codereview.chromium.org/1163863005/

esp...@chromium.org

unread,
Jun 2, 2015, 6:09:58 PM6/2/15
to ad...@chromium.org, kou...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, rob....@samsung.com, sigb...@opera.com
Okay do you want to look again? I had mess with editing to make this
actually
work.

https://codereview.chromium.org/1163863005/

kou...@chromium.org

unread,
Jun 3, 2015, 1:02:10 AM6/3/15
to esp...@chromium.org, ad...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, rob....@samsung.com, sigb...@opera.com
lgtm



https://codereview.chromium.org/1163863005/

'commit-bot: I haz the power' via codereview.chromium.org

unread,
Jun 3, 2015, 1:15:35 AM6/3/15
to esp...@chromium.org, ad...@chromium.org, kou...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, rob....@samsung.com, sigb...@opera.com
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1163863005/40001


https://codereview.chromium.org/1163863005/

'commit-bot: I haz the power' via codereview.chromium.org

unread,
Jun 3, 2015, 1:19:21 AM6/3/15
to esp...@chromium.org, ad...@chromium.org, kou...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, rob....@samsung.com, sigb...@opera.com
Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196371

https://codereview.chromium.org/1163863005/
Reply all
Reply to author
Forward
0 new messages