Reproducible crash in RenderBoxModelObject::adjustedPositionRelativeToOffsetParent()
authorbdakin@apple.com <bdakin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 7 May 2013 00:33:05 +0000 (00:33 +0000)
committerbdakin@apple.com <bdakin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 7 May 2013 00:33:05 +0000 (00:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=115685
-and corresponding-
<rdar://problem/13700734>

Reviewed by Sam Weinig.

This fix here is just to rollout the change that caused this regression, which is
http://trac.webkit.org/changeset/147395 . That change was not intended to cause any
behavioral differences. The change made it so RenderObject::offsetParent() returned
an Element* instead of a RenderBoxModelObject*. However, can muddle things when the
object we are returning is a continuation. Multiple RenderObjects have the same
Element in a continuation, so this new code can lead to a crash in
adjustedPositionRelativeToOffsetParent() when we expect to walk the RenderObject
chain and find the offsetParent in the Element’s parent chain. But we might crash in
some complicated continuation scenarios because we lost this disambiguation of which
RenderObject to start with.

Roll out.
* dom/Element.cpp:
(WebCore::Element::offsetParent):
* rendering/RenderBoxModelObject.cpp:
(WebCore::RenderBoxModelObject::adjustedPositionRelativeToOffsetParent):
* rendering/RenderObject.cpp:
(WebCore::RenderObject::offsetParent):
* rendering/RenderObject.h:
(RenderObject):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@149653 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/Element.cpp
Source/WebCore/rendering/RenderBoxModelObject.cpp
Source/WebCore/rendering/RenderObject.cpp
Source/WebCore/rendering/RenderObject.h

index dd73dd4..acc00d4 100644 (file)
@@ -1,3 +1,33 @@
+2013-05-06  Beth Dakin  <bdakin@apple.com>
+
+        Reproducible crash in RenderBoxModelObject::adjustedPositionRelativeToOffsetParent()
+        https://bugs.webkit.org/show_bug.cgi?id=115685
+        -and corresponding-
+        <rdar://problem/13700734>
+
+        Reviewed by Sam Weinig.
+
+        This fix here is just to rollout the change that caused this regression, which is 
+        http://trac.webkit.org/changeset/147395 . That change was not intended to cause any 
+        behavioral differences. The change made it so RenderObject::offsetParent() returned 
+        an Element* instead of a RenderBoxModelObject*. However, can muddle things when the 
+        object we are returning is a continuation. Multiple RenderObjects have the same 
+        Element in a continuation, so this new code can lead to a crash in 
+        adjustedPositionRelativeToOffsetParent() when we expect to walk the RenderObject 
+        chain and find the offsetParent in the Element’s parent chain. But we might crash in 
+        some complicated continuation scenarios because we lost this disambiguation of which 
+        RenderObject to start with.
+
+        Roll out.
+        * dom/Element.cpp:
+        (WebCore::Element::offsetParent):
+        * rendering/RenderBoxModelObject.cpp:
+        (WebCore::RenderBoxModelObject::adjustedPositionRelativeToOffsetParent):
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::offsetParent):
+        * rendering/RenderObject.h:
+        (RenderObject):
+
 2013-05-06  Ryosuke Niwa  <rniwa@webkit.org>
 
         Unify ways to cache named item in HTMLCollections
index d324cb0..c9fe6c8 100644 (file)
@@ -548,8 +548,10 @@ Element* Element::bindingsOffsetParent()
 Element* Element::offsetParent()
 {
     document()->updateLayoutIgnorePendingStylesheets();
-    if (RenderObject* renderer = this->renderer())
-        return renderer->offsetParent();
+    if (RenderObject* renderer = this->renderer()) {
+        if (RenderObject* offsetParent = renderer->offsetParent())
+            return toElement(offsetParent->node());
+    }
     return 0;
 }
 
index 797bff0..5e0bf15 100644 (file)
@@ -488,11 +488,7 @@ LayoutPoint RenderBoxModelObject::adjustedPositionRelativeToOffsetParent(const L
     // If the offsetParent of the element is null, or is the HTML body element,
     // return the distance between the canvas origin and the left border edge 
     // of the element and stop this algorithm.
-    Element* element = offsetParent();
-    if (!element)
-        return referencePoint;
-
-    if (const RenderBoxModelObject* offsetParent = element->renderBoxModelObject()) {
+    if (const RenderBoxModelObject* offsetParent = this->offsetParent()) {
         if (offsetParent->isBox() && !offsetParent->isBody())
             referencePoint.move(-toRenderBox(offsetParent)->borderLeft(), -toRenderBox(offsetParent)->borderTop());
         if (!isOutOfFlowPositioned()) {
index 1be997a..99684ba 100644 (file)
@@ -2976,46 +2976,46 @@ void RenderObject::imageChanged(CachedImage* image, const IntRect* rect)
     imageChanged(static_cast<WrappedImagePtr>(image), rect);
 }
 
-Element* RenderObject::offsetParent() const
+RenderBoxModelObject* RenderObject::offsetParent() const
 {
-    if (isRoot() || isBody())
-        return 0;
-
-    if (isOutOfFlowPositioned() && style()->position() == FixedPosition)
+    // If any of the following holds true return null and stop this algorithm:
+    // A is the root element.
+    // A is the HTML body element.
+    // The computed value of the position property for element A is fixed.
+    if (isRoot() || isBody() || (isOutOfFlowPositioned() && style()->position() == FixedPosition))
         return 0;
 
     // If A is an area HTML element which has a map HTML element somewhere in the ancestor
     // chain return the nearest ancestor map HTML element and stop this algorithm.
     // FIXME: Implement!
 
-    // FIXME: Figure out the right behavior for elements inside a flow thread.
+    // FIXME: Stop the search at the flow thread boundary until we figure out the right
+    // behavior for elements inside a flow thread.
     // https://bugs.webkit.org/show_bug.cgi?id=113276
-
-    float effectiveZoom = style()->effectiveZoom();
-    Node* node = 0;
-    for (RenderObject* ancestor = parent(); ancestor; ancestor = ancestor->parent()) {
-        node = ancestor->node();
-
-        // Spec: http://www.w3.org/TR/cssom-view/#offset-attributes
-
-        if (!node)
-            continue;
-
-        if (ancestor->isPositioned())
-            break;
-
-        if (node->hasTagName(HTMLNames::bodyTag))
+    
+    // Return the nearest ancestor element of A for which at least one of the following is
+    // true and stop this algorithm if such an ancestor is found:
+    //     * The computed value of the position property is not static.
+    //     * It is the HTML body element.
+    //     * The computed value of the position property of A is static and the ancestor
+    //       is one of the following HTML elements: td, th, or table.
+    //     * Our own extension: if there is a difference in the effective zoom
+
+    bool skipTables = isPositioned();
+    float currZoom = style()->effectiveZoom();
+    RenderObject* curr = parent();
+    while (curr && (!curr->node() || (!curr->isPositioned() && !curr->isBody())) && !curr->isRenderNamedFlowThread()) {
+        Node* element = curr->node();
+        if (!skipTables && element && (element->hasTagName(tableTag) || element->hasTagName(tdTag) || element->hasTagName(thTag)))
             break;
-
-        if (!isPositioned() && (node->hasTagName(tableTag) || node->hasTagName(tdTag) || node->hasTagName(thTag)))
-            break;
-
-        // Webkit specific extension where offsetParent stops at zoom level changes.
-        if (effectiveZoom != ancestor->style()->effectiveZoom())
+        float newZoom = curr->style()->effectiveZoom();
+        if (currZoom != newZoom)
             break;
+        currZoom = newZoom;
+        curr = curr->parent();
     }
-
-    return node && node->isElementNode() ? toElement(node) : 0;
+    return curr && curr->isBoxModelObject() && !curr->isRenderNamedFlowThread() ? toRenderBoxModelObject(curr) : 0;
 }
 
 VisiblePosition RenderObject::createVisiblePosition(int offset, EAffinity affinity)
index 3fe9bf0..952b6d5 100644 (file)
@@ -659,7 +659,7 @@ public:
 
     virtual RenderObject* hoverAncestor() const { return parent(); }
 
-    Element* offsetParent() const;
+    RenderBoxModelObject* offsetParent() const;
 
     void markContainingBlocksForLayout(bool scheduleRelayout = true, RenderObject* newRoot = 0);
     void setNeedsLayout(bool needsLayout, MarkingBehavior = MarkContainingBlockChain);