AX: should init an AXObject only after AXObjectCache has added it
authordmazzoni@google.com <dmazzoni@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 Jan 2013 08:16:00 +0000 (08:16 +0000)
committerdmazzoni@google.com <dmazzoni@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 Jan 2013 08:16:00 +0000 (08:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=107533

Reviewed by Chris Fleizach.

Source/WebCore:

Initialize each AXObject after the AXObjectCache has
finished adding it to its hash maps, so that it's
impossible for initialization of an AXObject to result in
exploring the tree and creating another AXObject instance
that points to the same renderer / node.

Test: accessibility/duplicate-axrenderobject-crash.html

* accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::getOrCreate):
* accessibility/AccessibilityARIAGrid.cpp:
(WebCore::AccessibilityARIAGrid::create):
* accessibility/AccessibilityARIAGridCell.cpp:
(WebCore::AccessibilityARIAGridCell::create):
* accessibility/AccessibilityARIAGridRow.cpp:
(WebCore::AccessibilityARIAGridRow::create):
* accessibility/AccessibilityList.cpp:
(WebCore::AccessibilityList::create):
* accessibility/AccessibilityListBox.cpp:
(WebCore::AccessibilityListBox::create):
* accessibility/AccessibilityMediaControls.cpp:
(WebCore::AccessibilityMediaControl::create):
(WebCore::AccessibilityMediaControlsContainer::create):
(WebCore::AccessibilityMediaTimeline::create):
(WebCore::AccessibilityMediaTimeDisplay::create):
* accessibility/AccessibilityMenuList.cpp:
(WebCore::AccessibilityMenuList::create):
* accessibility/AccessibilityNodeObject.cpp:
(WebCore::AccessibilityNodeObject::create):
* accessibility/AccessibilityObject.h:
(WebCore::AccessibilityObject::init):
(AccessibilityObject):
* accessibility/AccessibilityProgressIndicator.cpp:
(WebCore::AccessibilityProgressIndicator::create):
* accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::create):
(WebCore::AccessibilityRenderObject::accessibilityIsIgnored):
    assert that the object has been initialized
* accessibility/AccessibilitySVGRoot.cpp:
(WebCore::AccessibilitySVGRoot::create):
* accessibility/AccessibilitySlider.cpp:
(WebCore::AccessibilitySlider::create):
* accessibility/AccessibilityTable.cpp:
(WebCore::AccessibilityTable::create):
* accessibility/AccessibilityTableCell.cpp:
(WebCore::AccessibilityTableCell::create):
* accessibility/AccessibilityTableRow.cpp:
(WebCore::AccessibilityTableRow::create):

LayoutTests:

Adds a new test that demonstrates a crash if an AXObject
initializes itself before the AXObjectCache has added it to
the cache.

* accessibility/duplicate-axrenderobject-crash-expected.txt: Added.
* accessibility/duplicate-axrenderobject-crash.html: Added.

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

22 files changed:
LayoutTests/ChangeLog
LayoutTests/accessibility/duplicate-axrenderobject-crash-expected.txt [new file with mode: 0644]
LayoutTests/accessibility/duplicate-axrenderobject-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/accessibility/AXObjectCache.cpp
Source/WebCore/accessibility/AccessibilityARIAGrid.cpp
Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp
Source/WebCore/accessibility/AccessibilityARIAGridRow.cpp
Source/WebCore/accessibility/AccessibilityList.cpp
Source/WebCore/accessibility/AccessibilityListBox.cpp
Source/WebCore/accessibility/AccessibilityMediaControls.cpp
Source/WebCore/accessibility/AccessibilityMenuList.cpp
Source/WebCore/accessibility/AccessibilityNodeObject.cpp
Source/WebCore/accessibility/AccessibilityNodeObject.h
Source/WebCore/accessibility/AccessibilityObject.h
Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp
Source/WebCore/accessibility/AccessibilityRenderObject.cpp
Source/WebCore/accessibility/AccessibilitySVGRoot.cpp
Source/WebCore/accessibility/AccessibilitySlider.cpp
Source/WebCore/accessibility/AccessibilityTable.cpp
Source/WebCore/accessibility/AccessibilityTableCell.cpp
Source/WebCore/accessibility/AccessibilityTableRow.cpp

index cad8e49..4a4d8fd 100644 (file)
@@ -1,3 +1,17 @@
+2013-01-24  Dominic Mazzoni  <dmazzoni@google.com>
+
+        AX: should init an AXObject only after AXObjectCache has added it
+        https://bugs.webkit.org/show_bug.cgi?id=107533
+
+        Reviewed by Chris Fleizach.
+
+        Adds a new test that demonstrates a crash if an AXObject
+        initializes itself before the AXObjectCache has added it to
+        the cache.
+
+        * accessibility/duplicate-axrenderobject-crash-expected.txt: Added.
+        * accessibility/duplicate-axrenderobject-crash.html: Added.
+
 2013-01-23  Kentaro Hara  <haraken@chromium.org>
 
         Implement MouseEvent constructor
diff --git a/LayoutTests/accessibility/duplicate-axrenderobject-crash-expected.txt b/LayoutTests/accessibility/duplicate-axrenderobject-crash-expected.txt
new file mode 100644 (file)
index 0000000..764eb94
--- /dev/null
@@ -0,0 +1,7 @@
+PASS successfullyParsed is true
+
+TEST COMPLETE
+Ensures that it's not possible to have two AXRenderObjects that point to the same renderer, if the initialization of an AXRenderObject results in another object with the same renderer being created before AXObjectCache has added that mapping to its hash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
diff --git a/LayoutTests/accessibility/duplicate-axrenderobject-crash.html b/LayoutTests/accessibility/duplicate-axrenderobject-crash.html
new file mode 100644 (file)
index 0000000..71166f5
--- /dev/null
@@ -0,0 +1,28 @@
+<!doctype html>
+<html>
+<head>
+<link rel="stylesheet" href="../fast/js/resources/js-test-style.css">
+<script src="../fast/js/resources/js-test-pre.js"></script>
+</head>
+<body>
+
+<label>
+    <summary>
+        <ul style="display: table-header-group">
+            <keygen></keygen>
+        </ul>
+    </summary>
+</label>
+
+<p id="description"></p>
+
+<script>
+    description("Ensures that it's not possible to have two AXRenderObjects that point to the same renderer, if the initialization of an AXRenderObject results in another object with the same renderer being created before AXObjectCache has added that mapping to its hash.");
+    if (window.accessibilityController)
+        accessibilityController.accessibleElementById("dummy");
+</script>
+
+<script src="../fast/js/resources/js-test-post.js"></script>
+
+</body>
+</html>
index 6182f18..98f174c 100644 (file)
@@ -1,3 +1,59 @@
+2013-01-24  Dominic Mazzoni  <dmazzoni@google.com>
+
+        AX: should init an AXObject only after AXObjectCache has added it
+        https://bugs.webkit.org/show_bug.cgi?id=107533
+
+        Reviewed by Chris Fleizach.
+
+        Initialize each AXObject after the AXObjectCache has
+        finished adding it to its hash maps, so that it's
+        impossible for initialization of an AXObject to result in
+        exploring the tree and creating another AXObject instance
+        that points to the same renderer / node.
+
+        Test: accessibility/duplicate-axrenderobject-crash.html
+
+        * accessibility/AXObjectCache.cpp:
+        (WebCore::AXObjectCache::getOrCreate):
+        * accessibility/AccessibilityARIAGrid.cpp:
+        (WebCore::AccessibilityARIAGrid::create):
+        * accessibility/AccessibilityARIAGridCell.cpp:
+        (WebCore::AccessibilityARIAGridCell::create):
+        * accessibility/AccessibilityARIAGridRow.cpp:
+        (WebCore::AccessibilityARIAGridRow::create):
+        * accessibility/AccessibilityList.cpp:
+        (WebCore::AccessibilityList::create):
+        * accessibility/AccessibilityListBox.cpp:
+        (WebCore::AccessibilityListBox::create):
+        * accessibility/AccessibilityMediaControls.cpp:
+        (WebCore::AccessibilityMediaControl::create):
+        (WebCore::AccessibilityMediaControlsContainer::create):
+        (WebCore::AccessibilityMediaTimeline::create):
+        (WebCore::AccessibilityMediaTimeDisplay::create):
+        * accessibility/AccessibilityMenuList.cpp:
+        (WebCore::AccessibilityMenuList::create):
+        * accessibility/AccessibilityNodeObject.cpp:
+        (WebCore::AccessibilityNodeObject::create):
+        * accessibility/AccessibilityObject.h:
+        (WebCore::AccessibilityObject::init):
+        (AccessibilityObject):
+        * accessibility/AccessibilityProgressIndicator.cpp:
+        (WebCore::AccessibilityProgressIndicator::create):
+        * accessibility/AccessibilityRenderObject.cpp:
+        (WebCore::AccessibilityRenderObject::create):
+        (WebCore::AccessibilityRenderObject::accessibilityIsIgnored):
+            assert that the object has been initialized
+        * accessibility/AccessibilitySVGRoot.cpp:
+        (WebCore::AccessibilitySVGRoot::create):
+        * accessibility/AccessibilitySlider.cpp:
+        (WebCore::AccessibilitySlider::create):
+        * accessibility/AccessibilityTable.cpp:
+        (WebCore::AccessibilityTable::create):
+        * accessibility/AccessibilityTableCell.cpp:
+        (WebCore::AccessibilityTableCell::create):
+        * accessibility/AccessibilityTableRow.cpp:
+        (WebCore::AccessibilityTableRow::create):
+
 2013-01-23  Kentaro Hara  <haraken@chromium.org>
 
         Implement MouseEvent constructor
index 215aee2..47a649d 100644 (file)
@@ -314,6 +314,7 @@ AccessibilityObject* AXObjectCache::getOrCreate(Widget* widget)
     m_widgetObjectMapping.set(widget, newObj->axObjectID());
     m_objects.set(newObj->axObjectID(), newObj);    
     attachWrapper(newObj.get());
+    newObj->init();
     return newObj.get();
 }
 
@@ -349,6 +350,7 @@ AccessibilityObject* AXObjectCache::getOrCreate(Node* node)
     m_objects.set(newObj->axObjectID(), newObj);
     attachWrapper(newObj.get());
 
+    newObj->init();
     newObj->setCachedIsIgnoredValue(newObj->accessibilityIsIgnored());
 
     return newObj.get();
@@ -373,6 +375,7 @@ AccessibilityObject* AXObjectCache::getOrCreate(RenderObject* renderer)
     m_objects.set(newObj->axObjectID(), newObj);
     attachWrapper(newObj.get());
 
+    newObj->init();
     newObj->setCachedIsIgnoredValue(newObj->accessibilityIsIgnored());
 
     return newObj.get();
@@ -440,6 +443,7 @@ AccessibilityObject* AXObjectCache::getOrCreate(AccessibilityRole role)
 
     m_objects.set(obj->axObjectID(), obj);    
     attachWrapper(obj.get());
+    obj->init();
     return obj.get();
 }
 
index 2232287..6c9f264 100644 (file)
@@ -61,9 +61,7 @@ void AccessibilityARIAGrid::init()
 
 PassRefPtr<AccessibilityARIAGrid> AccessibilityARIAGrid::create(RenderObject* renderer)
 {
-    AccessibilityARIAGrid* obj = new AccessibilityARIAGrid(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityARIAGrid(renderer));
 }
 
 bool AccessibilityARIAGrid::addTableCellChild(AccessibilityObject* child, HashSet<AccessibilityObject*>& appendedRows, unsigned& columnCount)
index 9d16584..55d09ad 100644 (file)
@@ -48,9 +48,7 @@ AccessibilityARIAGridCell::~AccessibilityARIAGridCell()
 
 PassRefPtr<AccessibilityARIAGridCell> AccessibilityARIAGridCell::create(RenderObject* renderer)
 {
-    AccessibilityARIAGridCell* obj = new AccessibilityARIAGridCell(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityARIAGridCell(renderer));
 }
 
 AccessibilityObject* AccessibilityARIAGridCell::parentTable() const
index 8ed28db..a6ec0e5 100644 (file)
@@ -48,9 +48,7 @@ AccessibilityARIAGridRow::~AccessibilityARIAGridRow()
 
 PassRefPtr<AccessibilityARIAGridRow> AccessibilityARIAGridRow::create(RenderObject* renderer)
 {
-    AccessibilityARIAGridRow* obj = new AccessibilityARIAGridRow(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityARIAGridRow(renderer));
 }
 
 bool AccessibilityARIAGridRow::isARIATreeGridRow() const
index 6807314..073b0fc 100644 (file)
@@ -50,9 +50,7 @@ AccessibilityList::~AccessibilityList()
 
 PassRefPtr<AccessibilityList> AccessibilityList::create(RenderObject* renderer)
 {
-    AccessibilityList* obj = new AccessibilityList(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityList(renderer));
 }
 
 bool AccessibilityList::accessibilityIsIgnored() const
index 60f8546..22e5b81 100644 (file)
@@ -54,9 +54,7 @@ AccessibilityListBox::~AccessibilityListBox()
     
 PassRefPtr<AccessibilityListBox> AccessibilityListBox::create(RenderObject* renderer)
 {
-    AccessibilityListBox* obj = new AccessibilityListBox(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityListBox(renderer));
 }
     
 bool AccessibilityListBox::canSetSelectedChildrenAttribute() const
index cc4630c..3dad71b 100644 (file)
@@ -67,11 +67,8 @@ PassRefPtr<AccessibilityObject> AccessibilityMediaControl::create(RenderObject*
     case MediaControlsPanel:
         return AccessibilityMediaControlsContainer::create(renderer);
 
-    default: {
-        AccessibilityMediaControl* obj = new AccessibilityMediaControl(renderer);
-        obj->init();
-        return adoptRef(obj);
-        }
+    default:
+        return adoptRef(new AccessibilityMediaControl(renderer));
     }
 }
 
@@ -226,9 +223,7 @@ AccessibilityMediaControlsContainer::AccessibilityMediaControlsContainer(RenderO
 
 PassRefPtr<AccessibilityObject> AccessibilityMediaControlsContainer::create(RenderObject* renderer)
 {
-    AccessibilityMediaControlsContainer* obj = new AccessibilityMediaControlsContainer(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityMediaControlsContainer(renderer));
 }
 
 String AccessibilityMediaControlsContainer::accessibilityDescription() const
@@ -272,9 +267,7 @@ AccessibilityMediaTimeline::AccessibilityMediaTimeline(RenderObject* renderer)
 
 PassRefPtr<AccessibilityObject> AccessibilityMediaTimeline::create(RenderObject* renderer)
 {
-    AccessibilityMediaTimeline* obj = new AccessibilityMediaTimeline(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityMediaTimeline(renderer));
 }
 
 String AccessibilityMediaTimeline::valueDescription() const
@@ -304,9 +297,7 @@ AccessibilityMediaTimeDisplay::AccessibilityMediaTimeDisplay(RenderObject* rende
 
 PassRefPtr<AccessibilityObject> AccessibilityMediaTimeDisplay::create(RenderObject* renderer)
 {
-    AccessibilityMediaTimeDisplay* obj = new AccessibilityMediaTimeDisplay(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityMediaTimeDisplay(renderer));
 }
 
 bool AccessibilityMediaTimeDisplay::accessibilityIsIgnored() const
index eebd784..78f44ab 100644 (file)
@@ -39,9 +39,7 @@ AccessibilityMenuList::AccessibilityMenuList(RenderMenuList* renderer)
 
 PassRefPtr<AccessibilityMenuList> AccessibilityMenuList::create(RenderMenuList* renderer)
 {
-    AccessibilityMenuList* obj = new AccessibilityMenuList(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityMenuList(renderer));
 }
 
 bool AccessibilityMenuList::press() const
index f74619a..dde4123 100644 (file)
@@ -89,6 +89,9 @@ AccessibilityNodeObject::AccessibilityNodeObject(Node* node)
     , m_childrenDirty(false)
     , m_roleForMSAA(UnknownRole)
     , m_node(node)
+#ifndef NDEBUG
+    , m_initialized(false)
+#endif
 {
 }
 
@@ -99,14 +102,16 @@ AccessibilityNodeObject::~AccessibilityNodeObject()
 
 void AccessibilityNodeObject::init()
 {
+#ifndef NDEBUG
+    ASSERT(!m_initialized);
+    m_initialized = true;
+#endif
     m_role = determineAccessibilityRole();
 }
 
 PassRefPtr<AccessibilityNodeObject> AccessibilityNodeObject::create(Node* node)
 {
-    AccessibilityNodeObject* obj = new AccessibilityNodeObject(node);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityNodeObject(node));
 }
 
 void AccessibilityNodeObject::detach()
@@ -386,6 +391,12 @@ bool AccessibilityNodeObject::canHaveChildren() const
 
 bool AccessibilityNodeObject::accessibilityIsIgnored() const
 {
+#ifndef NDEBUG
+    // Double-check that an AccessibilityObject is never accessed before
+    // it's been initialized.
+    ASSERT(m_initialized);
+#endif
+
     // If this element is within a parent that cannot have children, it should not be exposed.
     if (isDescendantOfBarrenParent())
         return true;
index 8e04841..4da51c2 100644 (file)
@@ -153,6 +153,9 @@ protected:
     AccessibilityRole m_ariaRole;
     bool m_childrenDirty;
     mutable AccessibilityRole m_roleForMSAA;
+#ifndef NDEBUG
+    bool m_initialized;
+#endif
 
     virtual bool isDetached() const { return !m_node; }
 
index de96214..d8a3cbd 100644 (file)
@@ -355,6 +355,15 @@ protected:
     
 public:
     virtual ~AccessibilityObject();
+
+    // After constructing an AccessibilityObject, it must be given a
+    // unique ID, then added to AXObjectCache, and finally init() must
+    // be called last.
+    void setAXObjectID(AXID axObjectID) { m_id = axObjectID; }
+    virtual void init() { }
+
+    // When the corresponding WebCore object that this AccessibilityObject
+    // wraps is deleted, it must be detached.
     virtual void detach();
     virtual bool isDetached() const;
 
@@ -569,7 +578,6 @@ public:
 
     virtual AXObjectCache* axObjectCache() const;
     AXID axObjectID() const { return m_id; }
-    void setAXObjectID(AXID axObjectID) { m_id = axObjectID; }
     
     static AccessibilityObject* anchorElementForNode(Node*);
     virtual Element* anchorElement() const { return 0; }
index d3c76bd..494d1c2 100644 (file)
@@ -39,9 +39,7 @@ AccessibilityProgressIndicator::AccessibilityProgressIndicator(RenderProgress* r
 
 PassRefPtr<AccessibilityProgressIndicator> AccessibilityProgressIndicator::create(RenderProgress* renderer)
 {
-    AccessibilityProgressIndicator* obj = new AccessibilityProgressIndicator(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityProgressIndicator(renderer));
 }
 
 bool AccessibilityProgressIndicator::accessibilityIsIgnored() const
index fba36ce..8c6a112 100644 (file)
@@ -123,9 +123,7 @@ void AccessibilityRenderObject::init()
 
 PassRefPtr<AccessibilityRenderObject> AccessibilityRenderObject::create(RenderObject* renderer)
 {
-    AccessibilityRenderObject* obj = new AccessibilityRenderObject(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityRenderObject(renderer));
 }
 
 void AccessibilityRenderObject::detach()
@@ -1126,6 +1124,10 @@ AccessibilityObjectInclusion AccessibilityRenderObject::accessibilityIsIgnoredBa
  
 bool AccessibilityRenderObject::accessibilityIsIgnored() const
 {
+#ifndef NDEBUG
+    ASSERT(m_initialized);
+#endif
+
     // Check first if any of the common reasons cause this element to be ignored.
     // Then process other use cases that need to be applied to all the various roles
     // that AccessibilityRenderObjects take on.
index 0a755bb..08a2d58 100644 (file)
@@ -45,9 +45,7 @@ AccessibilitySVGRoot::~AccessibilitySVGRoot()
 
 PassRefPtr<AccessibilitySVGRoot> AccessibilitySVGRoot::create(RenderObject* renderer)
 {
-    AccessibilitySVGRoot* obj = new AccessibilitySVGRoot(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilitySVGRoot(renderer));
 }
     
 AccessibilityObject* AccessibilitySVGRoot::parentObject() const
index 53143e0..b03b9c4 100644 (file)
@@ -47,9 +47,7 @@ AccessibilitySlider::AccessibilitySlider(RenderObject* renderer)
 
 PassRefPtr<AccessibilitySlider> AccessibilitySlider::create(RenderObject* renderer)
 {
-    AccessibilitySlider* obj = new AccessibilitySlider(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilitySlider(renderer));
 }
 
 AccessibilityOrientation AccessibilitySlider::orientation() const
index 942e4a8..7af8963 100644 (file)
@@ -69,9 +69,7 @@ void AccessibilityTable::init()
 
 PassRefPtr<AccessibilityTable> AccessibilityTable::create(RenderObject* renderer)
 {
-    AccessibilityTable* obj = new AccessibilityTable(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityTable(renderer));
 }
 
 bool AccessibilityTable::hasARIARole() const
index b30409b..b0881c6 100644 (file)
@@ -51,9 +51,7 @@ AccessibilityTableCell::~AccessibilityTableCell()
 
 PassRefPtr<AccessibilityTableCell> AccessibilityTableCell::create(RenderObject* renderer)
 {
-    AccessibilityTableCell* obj = new AccessibilityTableCell(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityTableCell(renderer));
 }
 
 bool AccessibilityTableCell::accessibilityIsIgnored() const
index dec0892..3b22058 100644 (file)
@@ -54,9 +54,7 @@ AccessibilityTableRow::~AccessibilityTableRow()
 
 PassRefPtr<AccessibilityTableRow> AccessibilityTableRow::create(RenderObject* renderer)
 {
-    AccessibilityTableRow* obj = new AccessibilityTableRow(renderer);
-    obj->init();
-    return adoptRef(obj);
+    return adoptRef(new AccessibilityTableRow(renderer));
 }
 
 AccessibilityRole AccessibilityTableRow::determineAccessibilityRole()