AccessibilityRenderObject should not hold a raw pointer to RenderObject

https://bugs.webkit.org/show_bug.cgi?id=178144
<rdar://problem/34919287>

Reviewed by Chris Fleizach.

m_renderer's lifetime is not directly tied to the AX wrapper object's lifetime.

Covered by existing tests.

* accessibility/AccessibilityListBox.cpp:
(WebCore::AccessibilityListBox::elementAccessibilityHitTest const):
* accessibility/AccessibilityMathMLElement.cpp:
(WebCore::AccessibilityMathMLElement::isMathFenceOperator const):
(WebCore::AccessibilityMathMLElement::isMathSeparatorOperator const):
(WebCore::AccessibilityMathMLElement::mathLineThickness const):
* accessibility/AccessibilityMenuList.cpp:
(WebCore::AccessibilityMenuList::press):
(WebCore::AccessibilityMenuList::isCollapsed const):
* accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::AccessibilityRenderObject):
(WebCore::AccessibilityRenderObject::renderBoxModelObject const):
(WebCore::AccessibilityRenderObject::setRenderer):
(WebCore::AccessibilityRenderObject::previousSibling const):
(WebCore::AccessibilityRenderObject::anchorElement const):
(WebCore::AccessibilityRenderObject::helpText const):
(WebCore::AccessibilityRenderObject::boundingBoxRect const):
(WebCore::AccessibilityRenderObject::supportsPath const):
(WebCore::AccessibilityRenderObject::elementPath const):
(WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored const):
(WebCore::AccessibilityRenderObject::index const):
(WebCore::AccessibilityRenderObject::handleActiveDescendantChanged):
(WebCore::AccessibilityRenderObject::observableObject const):
(WebCore::AccessibilityRenderObject::determineAccessibilityRole):
(WebCore::AccessibilityRenderObject::textChanged):
(WebCore::AccessibilityRenderObject::remoteSVGRootElement const):
(WebCore::AccessibilityRenderObject::roleValueForMSAA const):
(WebCore::AccessibilityRenderObject::getScrollableAreaIfScrollable const):
(WebCore::AccessibilityRenderObject::scrollTo const):
* accessibility/AccessibilityRenderObject.h:
(WebCore::AccessibilityRenderObject::setRenderObject):
* accessibility/AccessibilitySlider.cpp:
(WebCore::AccessibilitySlider::elementAccessibilityHitTest const):
* accessibility/AccessibilityTable.cpp:
(WebCore::AccessibilityTable::addChildren):
* accessibility/AccessibilityTableCell.cpp:
(WebCore::AccessibilityTableCell::computeAccessibilityIsIgnored const):
(WebCore::AccessibilityTableCell::parentTable const):
(WebCore::AccessibilityTableCell::rowIndexRange const):
(WebCore::AccessibilityTableCell::columnIndexRange const):
(WebCore::AccessibilityTableCell::titleUIElement const):


Canonical link: https://commits.webkit.org/194394@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@223151 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,57 @@
+2017-10-10  Zalan Bujtas  <zalan@apple.com>
+
+        AccessibilityRenderObject should not hold a raw pointer to RenderObject
+        https://bugs.webkit.org/show_bug.cgi?id=178144
+        <rdar://problem/34919287>
+
+        Reviewed by Chris Fleizach.
+
+        m_renderer's lifetime is not directly tied to the AX wrapper object's lifetime.
+
+        Covered by existing tests.
+
+        * accessibility/AccessibilityListBox.cpp:
+        (WebCore::AccessibilityListBox::elementAccessibilityHitTest const):
+        * accessibility/AccessibilityMathMLElement.cpp:
+        (WebCore::AccessibilityMathMLElement::isMathFenceOperator const):
+        (WebCore::AccessibilityMathMLElement::isMathSeparatorOperator const):
+        (WebCore::AccessibilityMathMLElement::mathLineThickness const):
+        * accessibility/AccessibilityMenuList.cpp:
+        (WebCore::AccessibilityMenuList::press):
+        (WebCore::AccessibilityMenuList::isCollapsed const):
+        * accessibility/AccessibilityRenderObject.cpp:
+        (WebCore::AccessibilityRenderObject::AccessibilityRenderObject):
+        (WebCore::AccessibilityRenderObject::renderBoxModelObject const):
+        (WebCore::AccessibilityRenderObject::setRenderer):
+        (WebCore::AccessibilityRenderObject::previousSibling const):
+        (WebCore::AccessibilityRenderObject::anchorElement const):
+        (WebCore::AccessibilityRenderObject::helpText const):
+        (WebCore::AccessibilityRenderObject::boundingBoxRect const):
+        (WebCore::AccessibilityRenderObject::supportsPath const):
+        (WebCore::AccessibilityRenderObject::elementPath const):
+        (WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored const):
+        (WebCore::AccessibilityRenderObject::index const):
+        (WebCore::AccessibilityRenderObject::handleActiveDescendantChanged):
+        (WebCore::AccessibilityRenderObject::observableObject const):
+        (WebCore::AccessibilityRenderObject::determineAccessibilityRole):
+        (WebCore::AccessibilityRenderObject::textChanged):
+        (WebCore::AccessibilityRenderObject::remoteSVGRootElement const):
+        (WebCore::AccessibilityRenderObject::roleValueForMSAA const):
+        (WebCore::AccessibilityRenderObject::getScrollableAreaIfScrollable const):
+        (WebCore::AccessibilityRenderObject::scrollTo const):
+        * accessibility/AccessibilityRenderObject.h:
+        (WebCore::AccessibilityRenderObject::setRenderObject):
+        * accessibility/AccessibilitySlider.cpp:
+        (WebCore::AccessibilitySlider::elementAccessibilityHitTest const):
+        * accessibility/AccessibilityTable.cpp:
+        (WebCore::AccessibilityTable::addChildren):
+        * accessibility/AccessibilityTableCell.cpp:
+        (WebCore::AccessibilityTableCell::computeAccessibilityIsIgnored const):
+        (WebCore::AccessibilityTableCell::parentTable const):
+        (WebCore::AccessibilityTableCell::rowIndexRange const):
+        (WebCore::AccessibilityTableCell::columnIndexRange const):
+        (WebCore::AccessibilityTableCell::titleUIElement const):
+
 2017-10-10  Sam Weinig  <sam@webkit.org>
 
         Replace copyKeysToVector/copyValuesToVector with copyToVector(map.keys())/copyToVector(map.values())

Source/WebCore/accessibility/AccessibilityListBox.cpp

@@ -172,7 +172,7 @@ AccessibilityObject* AccessibilityListBox::elementAccessibilityHitTest(const Int
     if (listBoxOption && !listBoxOption->accessibilityIsIgnored())
         return listBoxOption;
 
-    return axObjectCache()->getOrCreate(m_renderer);
+    return axObjectCache()->getOrCreate(renderer());
 }
 
 } // namespace WebCore

Source/WebCore/accessibility/AccessibilityMathMLElement.cpp

@@ -153,15 +153,15 @@ bool AccessibilityMathMLElement::isAnonymousMathOperator() const
 
 bool AccessibilityMathMLElement::isMathFenceOperator() const
 {
-    if (!is<RenderMathMLOperator>(m_renderer))
+    if (!is<RenderMathMLOperator>(renderer()))
         return false;
 
     return downcast<RenderMathMLOperator>(*m_renderer).hasOperatorFlag(MathMLOperatorDictionary::Fence);
 }
 
 bool AccessibilityMathMLElement::isMathSeparatorOperator() const
 {
-    if (!is<RenderMathMLOperator>(m_renderer))
+    if (!is<RenderMathMLOperator>(renderer()))
         return false;
 
     return downcast<RenderMathMLOperator>(*m_renderer).hasOperatorFlag(MathMLOperatorDictionary::Separator);
@@ -445,7 +445,7 @@ void AccessibilityMathMLElement::mathPostscripts(AccessibilityMathMultiscriptPai
 
 int AccessibilityMathMLElement::mathLineThickness() const
 {
-    if (!is<RenderMathMLFraction>(m_renderer))
+    if (!is<RenderMathMLFraction>(renderer()))
         return -1;
 
     return downcast<RenderMathMLFraction>(*m_renderer).relativeLineThickness();

Source/WebCore/accessibility/AccessibilityMenuList.cpp

@@ -45,7 +45,7 @@ Ref<AccessibilityMenuList> AccessibilityMenuList::create(RenderMenuList* rendere
 bool AccessibilityMenuList::press()
 {
 #if !PLATFORM(IOS)
-    RenderMenuList* menuList = static_cast<RenderMenuList*>(m_renderer);
+    RenderMenuList* menuList = static_cast<RenderMenuList*>(renderer());
     if (menuList->popupIsVisible())
         menuList->hidePopup();
     else
@@ -93,7 +93,7 @@ void AccessibilityMenuList::childrenChanged()
 bool AccessibilityMenuList::isCollapsed() const
 {
 #if !PLATFORM(IOS)
-    return !static_cast<RenderMenuList*>(m_renderer)->popupIsVisible();
+    return !static_cast<RenderMenuList*>(renderer())->popupIsVisible();
 #else
     return true;
 #endif

Source/WebCore/accessibility/AccessibilityRenderObject.cpp

@@ -108,7 +108,7 @@ using namespace HTMLNames;
 
 AccessibilityRenderObject::AccessibilityRenderObject(RenderObject* renderer)
     : AccessibilityNodeObject(renderer->node())
-    , m_renderer(renderer)
+    , m_renderer(makeWeakPtr(renderer))
 {
 #ifndef NDEBUG
     m_renderer->setHasAXObject(true);
@@ -145,14 +145,14 @@ void AccessibilityRenderObject::detach(AccessibilityDetachmentType detachmentTyp
 
 RenderBoxModelObject* AccessibilityRenderObject::renderBoxModelObject() const
 {
-    if (!is<RenderBoxModelObject>(m_renderer))
+    if (!is<RenderBoxModelObject>(renderer()))
         return nullptr;
-    return downcast<RenderBoxModelObject>(m_renderer);
+    return downcast<RenderBoxModelObject>(renderer());
 }
 
 void AccessibilityRenderObject::setRenderer(RenderObject* renderer)
 {
-    m_renderer = renderer;
+    m_renderer = makeWeakPtr(renderer);
     setNode(renderer->node());
 }
 
@@ -324,7 +324,7 @@ AccessibilityObject* AccessibilityRenderObject::previousSibling() const
     // last child is our previous sibling (or further back in the continuation chain)
     RenderInline* startOfConts;
     if (is<RenderBox>(*m_renderer) && (startOfConts = startOfContinuations(*m_renderer)))
-        previousSibling = childBeforeConsideringContinuations(startOfConts, m_renderer);
+        previousSibling = childBeforeConsideringContinuations(startOfConts, renderer());
 
     // Case 2: Anonymous block parent of the end of a continuation - skip all the way to before
     // the parent of the start, since everything in between will be linked up via the continuation.
@@ -561,7 +561,7 @@ Element* AccessibilityRenderObject::anchorElement() const
     RenderObject* currentRenderer;
 
     // Search up the render tree for a RenderObject with a DOM node.  Defer to an earlier continuation, though.
-    for (currentRenderer = m_renderer; currentRenderer && !currentRenderer->node(); currentRenderer = currentRenderer->parent()) {
+    for (currentRenderer = renderer(); currentRenderer && !currentRenderer->node(); currentRenderer = currentRenderer->parent()) {
         if (currentRenderer->isAnonymousBlock()) {
             if (RenderObject* continuation = downcast<RenderBlock>(*currentRenderer).continuation())
                 return cache->getOrCreate(continuation)->anchorElement();
@@ -596,7 +596,7 @@ String AccessibilityRenderObject::helpText() const
         return describedBy;
 
     String description = accessibilityDescription();
-    for (RenderObject* ancestor = m_renderer; ancestor; ancestor = ancestor->parent()) {
+    for (RenderObject* ancestor = renderer(); ancestor; ancestor = ancestor->parent()) {
         if (is<HTMLElement>(ancestor->node())) {
             HTMLElement& element = downcast<HTMLElement>(*ancestor->node());
             const AtomicString& summary = element.getAttribute(summaryAttr);
@@ -823,7 +823,7 @@ void AccessibilityRenderObject::offsetBoundingBoxForRemoteSVGElement(LayoutRect&
 
 LayoutRect AccessibilityRenderObject::boundingBoxRect() const
 {
-    RenderObject* obj = m_renderer;
+    RenderObject* obj = renderer();
 
     if (!obj)
         return LayoutRect();
@@ -885,12 +885,12 @@ LayoutRect AccessibilityRenderObject::elementRect() const
 
 bool AccessibilityRenderObject::supportsPath() const
 {
-    return is<RenderSVGShape>(m_renderer);
+    return is<RenderSVGShape>(renderer());
 }
 
 Path AccessibilityRenderObject::elementPath() const
 {
-    if (is<RenderSVGShape>(m_renderer) && downcast<RenderSVGShape>(*m_renderer).hasPath()) {
+    if (is<RenderSVGShape>(renderer()) && downcast<RenderSVGShape>(*m_renderer).hasPath()) {
         Path path = downcast<RenderSVGShape>(*m_renderer).path();
 
         // The SVG path is in terms of the parent's bounding box. The path needs to be offset to frame coordinates.
@@ -1195,9 +1195,9 @@ bool AccessibilityRenderObject::computeAccessibilityIsIgnored() const
         return true;
 
     // WebAreas should be ignored if their iframe container is marked as presentational.
-    if (webAreaIsPresentational(m_renderer))
+    if (webAreaIsPresentational(renderer()))
         return true;
-        
+
     // An ARIA tree can only have tree items and static text as children.
     if (!isAllowedChildOfTree())
         return true;
@@ -1332,7 +1332,7 @@ bool AccessibilityRenderObject::computeAccessibilityIsIgnored() const
         // First check the RenderImage's altText (which can be set through a style sheet, or come from the Element).
         // However, if this is not a native image, fallback to the attribute on the Element.
         AccessibilityObjectInclusion altTextInclusion = DefaultBehavior;
-        bool isRenderImage = is<RenderImage>(m_renderer);
+        bool isRenderImage = is<RenderImage>(renderer());
         if (isRenderImage)
             altTextInclusion = objectInclusionFromAltText(downcast<RenderImage>(*m_renderer).altText());
         else
@@ -2197,7 +2197,7 @@ int AccessibilityRenderObject::index(const VisiblePosition& position) const
     if (position.isNull() || !isTextControl())
         return -1;
 
-    if (renderObjectContainsPosition(m_renderer, position.deepEquivalent()))
+    if (renderObjectContainsPosition(renderer(), position.deepEquivalent()))
         return indexForVisiblePosition(position);
 
     return -1;
@@ -2510,7 +2510,7 @@ void AccessibilityRenderObject::handleActiveDescendantChanged()
         return;
 
     if (activeDescendant() && shouldNotifyActiveDescendant())
-        renderer()->document().axObjectCache()->postNotification(m_renderer, AXObjectCache::AXActiveDescendantChanged);
+        renderer()->document().axObjectCache()->postNotification(renderer(), AXObjectCache::AXActiveDescendantChanged);
 }
 
 AccessibilityObject* AccessibilityRenderObject::correspondingControlForLabelElement() const
@@ -2573,7 +2573,7 @@ bool AccessibilityRenderObject::renderObjectIsObservable(RenderObject& renderer)
 AccessibilityObject* AccessibilityRenderObject::observableObject() const
 {
     // Find the object going up the parent chain that is used in accessibility to monitor certain notifications.
-    for (RenderObject* renderer = m_renderer; renderer && renderer->node(); renderer = renderer->parent()) {
+    for (RenderObject* renderer = this->renderer(); renderer && renderer->node(); renderer = renderer->parent()) {
         if (renderObjectIsObservable(*renderer)) {
             if (AXObjectCache* cache = axObjectCache())
                 return cache->getOrCreate(renderer);
@@ -2727,7 +2727,7 @@ AccessibilityRole AccessibilityRenderObject::determineAccessibilityRole()
 
     // This return value is what will be used if AccessibilityTableCell determines
     // the cell should not be treated as a cell (e.g. because it is a layout table.
-    if (is<RenderTableCell>(m_renderer))
+    if (is<RenderTableCell>(renderer()))
         return TextGroupRole;
 
     // Table sections should be ignored.
@@ -2959,7 +2959,7 @@ void AccessibilityRenderObject::textChanged()
     if (!cache)
         return;
 
-    for (RenderObject* renderParent = m_renderer; renderParent; renderParent = renderParent->parent()) {
+    for (RenderObject* renderParent = renderer(); renderParent; renderParent = renderParent->parent()) {
         AccessibilityObject* parent = cache->get(renderParent);
         if (!parent)
             continue;
@@ -3046,7 +3046,7 @@ void AccessibilityRenderObject::detachRemoteSVGRoot()
 
 AccessibilitySVGRoot* AccessibilityRenderObject::remoteSVGRootElement(CreationChoice createIfNecessary) const
 {
-    if (!is<RenderImage>(m_renderer))
+    if (!is<RenderImage>(renderer()))
         return nullptr;
 
     CachedImage* cachedImage = downcast<RenderImage>(*m_renderer).cachedImage();
@@ -3666,7 +3666,7 @@ AccessibilityRole AccessibilityRenderObject::roleValueForMSAA() const
     if (m_roleForMSAA != UnknownRole)
         return m_roleForMSAA;
 
-    m_roleForMSAA = msaaRoleForRenderer(m_renderer);
+    m_roleForMSAA = msaaRoleForRenderer(renderer());
 
     if (m_roleForMSAA == UnknownRole)
         m_roleForMSAA = roleValue();
@@ -3696,7 +3696,7 @@ ScrollableArea* AccessibilityRenderObject::getScrollableAreaIfScrollable() const
     if (parentObject() && parentObject()->isAccessibilityScrollView())
         return nullptr;
 
-    if (!is<RenderBox>(m_renderer))
+    if (!is<RenderBox>(renderer()))
         return nullptr;
 
     auto& box = downcast<RenderBox>(*m_renderer);
@@ -3708,7 +3708,7 @@ ScrollableArea* AccessibilityRenderObject::getScrollableAreaIfScrollable() const
 
 void AccessibilityRenderObject::scrollTo(const IntPoint& point) const
 {
-    if (!is<RenderBox>(m_renderer))
+    if (!is<RenderBox>(renderer()))
         return;
 
     auto& box = downcast<RenderBox>(*m_renderer);

Source/WebCore/accessibility/AccessibilityRenderObject.h

@@ -30,6 +30,7 @@
 
 #include "AccessibilityNodeObject.h"
 #include "LayoutRect.h"
+#include "RenderObject.h"
 #include <wtf/Forward.h>
 #include <wtf/WeakPtr.h>
 
@@ -114,7 +115,7 @@ class AccessibilityRenderObject : public AccessibilityNodeObject {
     IntPoint clickPoint() override;
 
     void setRenderer(RenderObject*);
-    RenderObject* renderer() const override { return m_renderer; }
+    RenderObject* renderer() const override { return m_renderer.get(); }
     RenderBoxModelObject* renderBoxModelObject() const;
     Node* node() const override;
 
@@ -204,7 +205,6 @@ class AccessibilityRenderObject : public AccessibilityNodeObject {
 
 protected:
     explicit AccessibilityRenderObject(RenderObject*);
-    void setRenderObject(RenderObject* renderer) { m_renderer = renderer; }
     ScrollableArea* getScrollableAreaIfScrollable() const override;
     void scrollTo(const IntPoint&) const override;
 
@@ -217,7 +217,7 @@ class AccessibilityRenderObject : public AccessibilityNodeObject {
     virtual bool isIgnoredElementWithinMathTree() const;
 #endif
 
-    RenderObject* m_renderer;
+    WeakPtr<RenderObject> m_renderer;
 
 private:
     WeakPtrFactory<AccessibilityRenderObject> m_weakPtrFactory;

Source/WebCore/accessibility/AccessibilitySlider.cpp

@@ -108,7 +108,7 @@ AccessibilityObject* AccessibilitySlider::elementAccessibilityHitTest(const IntP
             return m_children[0].get();
     }
 
-    return axObjectCache()->getOrCreate(m_renderer);
+    return axObjectCache()->getOrCreate(renderer());
 }
 
 float AccessibilitySlider::valueForRange() const

Source/WebCore/accessibility/AccessibilityTable.cpp

@@ -386,7 +386,7 @@ void AccessibilityTable::addChildren()
     ASSERT(!m_haveChildren); 
 
     m_haveChildren = true;
-    if (!is<RenderTable>(m_renderer))
+    if (!is<RenderTable>(renderer()))
         return;
 
     RenderTable& table = downcast<RenderTable>(*m_renderer);

Source/WebCore/accessibility/AccessibilityTableCell.cpp

@@ -66,7 +66,7 @@ bool AccessibilityTableCell::computeAccessibilityIsIgnored() const
         return true;
 
     // Ignore anonymous table cells as long as they're not in a table (ie. when display:table is used).
-    RenderObject* renderTable = is<RenderTableCell>(m_renderer) ? downcast<RenderTableCell>(*m_renderer).table() : nullptr;
+    RenderObject* renderTable = is<RenderTableCell>(renderer()) ? downcast<RenderTableCell>(*m_renderer).table() : nullptr;
     bool inTable = renderTable && renderTable->node() && (renderTable->node()->hasTagName(tableTag) || nodeHasRole(renderTable->node(), "grid"));
     if (!node() && !inTable)
         return true;
@@ -79,7 +79,7 @@ bool AccessibilityTableCell::computeAccessibilityIsIgnored() const
 
 AccessibilityTable* AccessibilityTableCell::parentTable() const
 {
-    if (!is<RenderTableCell>(m_renderer))
+    if (!is<RenderTableCell>(renderer()))
         return nullptr;
 
     // If the document no longer exists, we might not have an axObjectCache.
@@ -314,7 +314,7 @@ AccessibilityTableRow* AccessibilityTableCell::parentRow() const
 
 void AccessibilityTableCell::rowIndexRange(std::pair<unsigned, unsigned>& rowRange) const
 {
-    if (!is<RenderTableCell>(m_renderer))
+    if (!is<RenderTableCell>(renderer()))
         return;
 
     RenderTableCell& renderCell = downcast<RenderTableCell>(*m_renderer);
@@ -332,7 +332,7 @@ void AccessibilityTableCell::rowIndexRange(std::pair<unsigned, unsigned>& rowRan
 
 void AccessibilityTableCell::columnIndexRange(std::pair<unsigned, unsigned>& columnRange) const
 {
-    if (!is<RenderTableCell>(m_renderer))
+    if (!is<RenderTableCell>(renderer()))
         return;
 
     const RenderTableCell& cell = downcast<RenderTableCell>(*m_renderer);
@@ -353,7 +353,7 @@ AccessibilityObject* AccessibilityTableCell::titleUIElement() const
     // Try to find if the first cell in this row is a <th>. If it is,
     // then it can act as the title ui element. (This is only in the
     // case when the table is not appearing as an AXTable.)
-    if (isTableCell() || !is<RenderTableCell>(m_renderer))
+    if (isTableCell() || !is<RenderTableCell>(renderer()))
         return nullptr;
 
     // Table cells that are th cannot have title ui elements, since by definition