Changeset 212140 in webkit

Timestamp:

Feb 10, 2017, 1:15:51 PM (9 years ago)

Author:

rniwa@webkit.org

Message:

HTMLConstructionSiteTask::Insert should never be called on a node with a parent
​https://bugs.webkit.org/show_bug.cgi?id=168099

Reviewed by Sam Weinig.

insertAlreadyParsedChild always use HTMLConstructionSiteTask::InsertAlreadyParsedChild instead
of using HTMLConstructionSiteTask::Insert when fostering a child.

Also combine the step to take all children and re-parenting into a single task instead of
separately issuing TakeAllChildren and Reparent tasks.

No new tests since this is a refactoring.

• html/parser/HTMLConstructionSite.cpp:

(WebCore::insert): Now asserts that the child node never have a parent.
(WebCore::executeInsertAlreadyParsedChildTask): Moved the code to remove the parent here.
(WebCore::executeTakeAllChildrenAndReparentTask): Renamed from executeTakeAllChildrenTask
now that this function also does the reparenting.
(WebCore::executeTask):
(WebCore::HTMLConstructionSite::reparent): Removed the variant only used with takeAllChildren.
(WebCore::HTMLConstructionSite::insertAlreadyParsedChild): Always use InsertAlreadyParsedChild
instead of calling fosterParent which uses Insert when fostering parents.
(WebCore::HTMLConstructionSite::takeAllChildrenAndReparent): Renamed from takeAllChildren.

• html/parser/HTMLConstructionSite.h:

(WebCore::HTMLConstructionSiteTask:Operation):

• html/parser/HTMLTreeBuilder.cpp:

(WebCore::HTMLTreeBuilder::callTheAdoptionAgency):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• html/parser/HTMLConstructionSite.cpp (modified) (4 diffs)
• html/parser/HTMLConstructionSite.h (modified) (2 diffs)
• html/parser/HTMLTreeBuilder.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-02-10  Ryosuke Niwa  <rniwa@webkit.org>
+
+        HTMLConstructionSiteTask::Insert should never be called on a node with a parent
+        https://bugs.webkit.org/show_bug.cgi?id=168099
+
+        Reviewed by Sam Weinig.
+
+        insertAlreadyParsedChild always use HTMLConstructionSiteTask::InsertAlreadyParsedChild instead
+        of using HTMLConstructionSiteTask::Insert when fostering a child.
+
+        Also combine the step to take all children and re-parenting into a single task instead of
+        separately issuing TakeAllChildren and Reparent tasks.
+
+        No new tests since this is a refactoring.
+
+        * html/parser/HTMLConstructionSite.cpp:
+        (WebCore::insert): Now asserts that the child node never have a parent.
+        (WebCore::executeInsertAlreadyParsedChildTask): Moved the code to remove the parent here.
+        (WebCore::executeTakeAllChildrenAndReparentTask): Renamed from executeTakeAllChildrenTask
+        now that this function also does the reparenting.
+        (WebCore::executeTask):
+        (WebCore::HTMLConstructionSite::reparent): Removed the variant only used with takeAllChildren.
+        (WebCore::HTMLConstructionSite::insertAlreadyParsedChild): Always use InsertAlreadyParsedChild
+        instead of calling fosterParent which uses Insert when fostering parents.
+        (WebCore::HTMLConstructionSite::takeAllChildrenAndReparent): Renamed from takeAllChildren.
+        * html/parser/HTMLConstructionSite.h:
+        (WebCore::HTMLConstructionSiteTask:Operation):
+        * html/parser/HTMLTreeBuilder.cpp:
+        (WebCore::HTMLTreeBuilder::callTheAdoptionAgency):
+
 2017-02-10  Dave Hyatt  <hyatt@apple.com>
 

trunk/Source/WebCore/html/parser/HTMLConstructionSite.cpp

@@ -106 +106 @@
         task.parent = &downcast<HTMLTemplateElement>(*task.parent).content();
 
-    if (ContainerNode* parent = task.child->parentNode())
-        parent->parserRemoveChild(*task.child);
-
+    ASSERT(!task.child->parentNode());
     if (task.nextChild)
         task.parent->parserInsertBefore(*task.child, *task.nextChild);
@@ -141 +139 @@
     ASSERT(task.operation == HTMLConstructionSiteTask::InsertAlreadyParsedChild);
 
+    if (ContainerNode* parent = task.child->parentNode())
+        parent->parserRemoveChild(*task.child);
     insert(task);
 }
 
-static inline void executeTakeAllChildrenTask(HTMLConstructionSiteTask& task)
-{
-    ASSERT(task.operation == HTMLConstructionSiteTask::TakeAllChildren);
-
-    task.parent->takeAllChildrenFrom(task.oldParent());
-    // Notice that we don't need to manually attach the moved children
-    // because takeAllChildrenFrom does that work for us.
+static inline void executeTakeAllChildrenAndReparentTask(HTMLConstructionSiteTask& task)
+{
+    ASSERT(task.operation == HTMLConstructionSiteTask::TakeAllChildrenAndReparent);
+
+    auto* furthestBlock = task.oldParent();
+    task.parent->takeAllChildrenFrom(furthestBlock);
+
+    furthestBlock->parserAppendChild(*task.parent);
 }
 
@@ -166 +167 @@
         executeReparentTask(task);
         return;
-    case HTMLConstructionSiteTask::TakeAllChildren:
-        executeTakeAllChildrenTask(task);
+    case HTMLConstructionSiteTask::TakeAllChildrenAndReparent:
+        executeTakeAllChildrenAndReparentTask(task);
         return;
     }
@@ -600 +601 @@
 }
 
-void HTMLConstructionSite::reparent(HTMLElementStack::ElementRecord& newParent, HTMLStackItem& child)
-{
-    HTMLConstructionSiteTask task(HTMLConstructionSiteTask::Reparent);
-    task.parent = &newParent.node();
+void HTMLConstructionSite::insertAlreadyParsedChild(HTMLStackItem& newParent, HTMLElementStack::ElementRecord& child)
+{
+    HTMLConstructionSiteTask task(HTMLConstructionSiteTask::InsertAlreadyParsedChild);
+    if (causesFosterParenting(newParent)) {
+        findFosterSite(task);
+        ASSERT(task.parent);
+    } else
+        task.parent = &newParent.node();
     task.child = &child.element();
     m_taskQueue.append(WTFMove(task));
 }
 
-void HTMLConstructionSite::insertAlreadyParsedChild(HTMLStackItem& newParent, HTMLElementStack::ElementRecord& child)
-{
-    if (causesFosterParenting(newParent)) {
-        fosterParent(child.element());
-        return;
-    }
-
-    HTMLConstructionSiteTask task(HTMLConstructionSiteTask::InsertAlreadyParsedChild);
-    task.parent = &newParent.node();
-    task.child = &child.element();
-    m_taskQueue.append(WTFMove(task));
-}
-
-void HTMLConstructionSite::takeAllChildren(HTMLStackItem& newParent, HTMLElementStack::ElementRecord& oldParent)
-{
-    HTMLConstructionSiteTask task(HTMLConstructionSiteTask::TakeAllChildren);
+void HTMLConstructionSite::takeAllChildrenAndReparent(HTMLStackItem& newParent, HTMLElementStack::ElementRecord& oldParent)
+{
+    HTMLConstructionSiteTask task(HTMLConstructionSiteTask::TakeAllChildrenAndReparent);
     task.parent = &newParent.node();
     task.child = &oldParent.node();

trunk/Source/WebCore/html/parser/HTMLConstructionSite.h

@@ -42 +42 @@
         InsertAlreadyParsedChild,
         Reparent,
-        TakeAllChildren,
+        TakeAllChildrenAndReparent,
     };
 
@@ -120 +120 @@
 
     void reparent(HTMLElementStack::ElementRecord& newParent, HTMLElementStack::ElementRecord& child);
-    void reparent(HTMLElementStack::ElementRecord& newParent, HTMLStackItem& child);
     // insertAlreadyParsedChild assumes that |child| has already been parsed (i.e., we're just
     // moving it around in the tree rather than parsing it for the first time). That means
     // this function doesn't call beginParsingChildren / finishParsingChildren.
     void insertAlreadyParsedChild(HTMLStackItem& newParent, HTMLElementStack::ElementRecord& child);
-    void takeAllChildren(HTMLStackItem& newParent, HTMLElementStack::ElementRecord& oldParent);
+    void takeAllChildrenAndReparent(HTMLStackItem& newParent, HTMLElementStack::ElementRecord& oldParent);
 
     Ref<HTMLStackItem> createElementFromSavedToken(HTMLStackItem&);

trunk/Source/WebCore/html/parser/HTMLTreeBuilder.cpp

@@ -1497 +1497 @@
         // 11.
         auto newItem = m_tree.createElementFromSavedToken(formattingElementRecord->stackItem());
-        // 12.
-        m_tree.takeAllChildren(newItem, *furthestBlock);
-        // 13.
-        m_tree.reparent(*furthestBlock, newItem);
+        // 12. & 13.
+        m_tree.takeAllChildrenAndReparent(newItem, *furthestBlock);
         // 14.
         m_tree.activeFormattingElements().swapTo(*formattingElement, newItem.copyRef(), bookmark);