Changeset 117463 in webkit

Timestamp:

May 17, 2012 10:54:40 AM (12 years ago)

Author:

caio.oliveira@openbossa.org

Message:

[Qt] REGRESSION(101967): It made editing/style/iframe-onload-crash-mac.html timeout
​https://bugs.webkit.org/show_bug.cgi?id=73802

Reviewed by Ryosuke Niwa.

Source/WebCore:

Timeout was caused by an infinite in the outer loop of
pushDownInlineStyleAroundNode(). The outer loop variable 'current' should point at the
node containing 'targetNode'. The inner loop traverse the children of 'current'
and discover the children that contains 'targetNode'.

However, before the inner loop, we call removeInlineStyleFromElement() that can
potentially remove the 'current' node from the tree, moving its children to
'current' former parent. For that reason 'child' and 'lastChild' are collected
before this call.

The tricky part is that changing the 'current' children parent, we might trigger
further side-effects, that can remove either 'child' or 'lastChild' from the tree
too. The infinite loop was due to 'child' being off the document, so it's
nextSibling() is 0, and we go another run of outer loop without changing
'current' because the 'targetNode' wasn't in the first child that inner loop
couldn't reach.

When testing Qt on Mac, there was also a crash in RenderTextControl when the font
family was empty, this patch fixes it as well.

• editing/ApplyStyleCommand.cpp:

(WebCore::ApplyStyleCommand::pushDownInlineStyleAroundNode): Use NodeVector
instead of relying on first/last child being valid after
removeInlineStyleFromElement() is called. Skip the child if it has no parent,
this is an indication that it was removed from the tree.

• rendering/RenderTextControl.cpp:

(WebCore::RenderTextControl::hasValidAvgCharWidth): Empty AtomicStrings aren't
supported by HashSet, so we have to early return in this case.

LayoutTests:

• platform/qt/Skipped: Unskipped. Note that it is still skipped for wk2 because

setEditingBehavior is not implemented for WebKitTestRunner yet.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/qt/Skipped (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/editing/ApplyStyleCommand.cpp (modified) (3 diffs)
• Source/WebCore/rendering/RenderTextControl.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2012-05-17  Caio Marcelo de Oliveira Filho  <caio.oliveira@openbossa.org>
+
+        [Qt] REGRESSION(101967): It made editing/style/iframe-onload-crash-mac.html timeout
+        https://bugs.webkit.org/show_bug.cgi?id=73802
+
+        Reviewed by Ryosuke Niwa.
+
+        * platform/qt/Skipped: Unskipped. Note that it is still skipped for wk2 because
+        setEditingBehavior is not implemented for WebKitTestRunner yet.
+
 2012-05-17  Raphael Kubo da Costa  <rakuco@webkit.org>
 

trunk/LayoutTests/platform/qt/Skipped

@@ -2323 +2323 @@
 fast/forms/select/listbox-in-multi-column.html
 
-# [Qt] REGRESSION(101967): It made editing/style/iframe-onload-crash-mac.html timeout
-# https://bugs.webkit.org/show_bug.cgi?id=73802
-editing/style/iframe-onload-crash-mac.html
-
 # [Qt] fails fast/inline/nested-text-descendants.html
 # https://bugs.webkit.org/show_bug.cgi?id=75321

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-05-17  Caio Marcelo de Oliveira Filho  <caio.oliveira@openbossa.org>
+
+        [Qt] REGRESSION(101967): It made editing/style/iframe-onload-crash-mac.html timeout
+        https://bugs.webkit.org/show_bug.cgi?id=73802
+
+        Reviewed by Ryosuke Niwa.
+
+        Timeout was caused by an infinite in the outer loop of
+        pushDownInlineStyleAroundNode(). The outer loop variable 'current' should point at the
+        node containing 'targetNode'. The inner loop traverse the children of 'current'
+        and discover the children that contains 'targetNode'.
+
+        However, before the inner loop, we call removeInlineStyleFromElement() that can
+        potentially remove the 'current' node from the tree, moving its children to
+        'current' former parent. For that reason 'child' and 'lastChild' are collected
+        before this call.
+
+        The tricky part is that changing the 'current' children parent, we might trigger
+        further side-effects, that can remove either 'child' or 'lastChild' from the tree
+        too. The infinite loop was due to 'child' being off the document, so it's
+        nextSibling() is 0, and we go another run of outer loop without changing
+        'current' because the 'targetNode' wasn't in the first child that inner loop
+        couldn't reach.
+
+        When testing Qt on Mac, there was also a crash in RenderTextControl when the font
+        family was empty, this patch fixes it as well.
+
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::pushDownInlineStyleAroundNode): Use NodeVector
+        instead of relying on first/last child being valid after
+        removeInlineStyleFromElement() is called. Skip the child if it has no parent,
+        this is an indication that it was removed from the tree.
+
+        * rendering/RenderTextControl.cpp:
+        (WebCore::RenderTextControl::hasValidAvgCharWidth): Empty AtomicStrings aren't
+        supported by HashSet, so we have to early return in this case.
+
 2012-05-17  Pavel Feldman  <pfeldman@chromium.org>
 

trunk/Source/WebCore/editing/ApplyStyleCommand.cpp

@@ -978 +978 @@
         ASSERT(current);
         ASSERT(current->contains(targetNode));
-        Node* child = current->firstChild();
-        Node* lastChild = current->lastChild();
+        NodeVector currentChildren;
+        getChildNodes(current, currentChildren);
         RefPtr<StyledElement> styledElement;
         if (current->isStyledElement() && isStyledInlineElementToRemove(static_cast<Element*>(current))) {
@@ -992 +992 @@
         // The inner loop will go through children on each level
         // FIXME: we should aggregate inline child elements together so that we don't wrap each child separately.
-        while (child) {
-            Node* nextChild = child->nextSibling();
-
+        for (size_t i = 0; i < currentChildren.size(); ++i) {
+            Node* child = currentChildren[i].get();
+            if (!child->parentNode())
+                continue;
             if (!child->contains(targetNode) && elementsToPushDown.size()) {
                 for (size_t i = 0; i < elementsToPushDown.size(); i++) {
@@ -1012 +1013 @@
             if (child == targetNode || child->contains(targetNode))
                 current = child;
-
-            if (child == lastChild || child->contains(lastChild))
-                break;
-            child = nextChild;
         }
     }

trunk/Source/WebCore/rendering/RenderTextControl.cpp

@@ -209 +209 @@
     static HashSet<AtomicString>* fontFamiliesWithInvalidCharWidthMap = 0;
 
+    if (family.isEmpty())
+        return false;
+
     if (!fontFamiliesWithInvalidCharWidthMap) {
         fontFamiliesWithInvalidCharWidthMap = new HashSet<AtomicString>;