Bug 23443: Table accessibility should be re-enabled after fixing crash that occurs...
authorcfleizach@apple.com <cfleizach@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Jan 2009 22:55:01 +0000 (22:55 +0000)
committercfleizach@apple.com <cfleizach@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Jan 2009 22:55:01 +0000 (22:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=23443

Re-enable Accessibility tables and make sure accessibility code does not interrogate the render tree
during render tree updates

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/accessibility/table-modification-crash-expected.txt [new file with mode: 0644]
LayoutTests/accessibility/table-modification-crash.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/page/AccessibilityObject.cpp
WebCore/page/AccessibilityObject.h
WebCore/page/AccessibilityRenderObject.cpp
WebCore/page/AccessibilityRenderObject.h
WebCore/page/AccessibilityTable.cpp
WebCore/page/mac/AccessibilityObjectWrapper.mm
WebCore/rendering/RenderObject.cpp
WebCore/rendering/RenderWidget.cpp

index f9e0eb8..1afe162 100644 (file)
@@ -1,3 +1,12 @@
+2009-01-21  Chris Fleizach  <cfleizach@apple.com>
+
+        Reviewed by Beth Dakin.
+
+        Test to make sure accessibility doesn't crash when a table is modified through JavaScript
+
+        * accessibility/table-modification-crash-expected.txt: Added.
+        * accessibility/table-modification-crash.html: Added.
+
 2009-01-16  Eric Seidel  <eric@webkit.org>
 
         Reviewed by Justin Garcia.
diff --git a/LayoutTests/accessibility/table-modification-crash-expected.txt b/LayoutTests/accessibility/table-modification-crash-expected.txt
new file mode 100644 (file)
index 0000000..bbe86fd
--- /dev/null
@@ -0,0 +1,3 @@
+column 1       column 2        column 3
+Test passed
+
diff --git a/LayoutTests/accessibility/table-modification-crash.html b/LayoutTests/accessibility/table-modification-crash.html
new file mode 100644 (file)
index 0000000..1634c9e
--- /dev/null
@@ -0,0 +1,50 @@
+<html>
+<script>
+    if (window.layoutTestController)
+        layoutTestController.dumpAsText();
+</script>
+
+<body id="body">
+
+    <!-- This test makes sure we do not crash if javascript changes a table element -->
+    <table border=1 id='table1'><tr><td>a</td><td>b</td><td>c</td></tr></table>
+
+    <div id="result"></div>
+     
+    <script>
+        if (window.accessibilityController) {
+            var result = document.getElementById("result");
+
+            var table = document.getElementById("table1");
+            table.focus();
+            tableAX = accessibilityController.focusedElement;
+
+            var string = tableAX.attributesOfChildren();
+
+             var row = document.createElement("tr")
+             var td1 = document.createElement("td")
+             td1.appendChild(document.createTextNode("column 1"))
+             row.appendChild(td1);
+
+             var td2 = document.createElement("td")
+             td2.appendChild(document.createTextNode("column 2"))
+             row.appendChild(td2);
+
+             var td3 = document.createElement("td")
+             td3.appendChild(document.createTextNode("column 3"))
+             row.appendChild(td3);
+
+             table.childNodes[0].appendChild(row);
+
+             string = tableAX.attributesOfChildren();
+             table.childNodes[0].removeChild(table.childNodes[0].childNodes[0]);
+
+             string = tableAX.attributesOfChildren();
+         
+             result.innerText += "Test passed\n";
+        }
+    </script>
+</body>
+</html>
index 3196e39..da539d9 100644 (file)
@@ -1,3 +1,44 @@
+2009-01-21  Chris Fleizach  <cfleizach@apple.com>
+
+        Reviewed by Beth Dakin.
+
+        Bug 23443: Table accessibility should be re-enabled after fixing crash that occurs at WebCore::AccessibilityTable::isTableExposableThroughAccessibility() 
+        https://bugs.webkit.org/show_bug.cgi?id=23443
+
+        Test: accessibility/table-modification-crash.html
+
+        * page/AccessibilityObject.cpp:
+        (WebCore::AccessibilityObject::updateBackingStore):
+        * page/AccessibilityObject.h:
+        * page/AccessibilityRenderObject.cpp:
+        (WebCore::AccessibilityRenderObject::childrenChanged):
+        (WebCore::AccessibilityRenderObject::children):
+        (WebCore::AccessibilityRenderObject::updateBackingStore):
+        * page/AccessibilityRenderObject.h:
+        (WebCore::AccessibilityRenderObject::markChildrenDirty):
+        * page/AccessibilityTable.cpp:
+        (WebCore::AccessibilityTable::AccessibilityTable):
+        * page/mac/AccessibilityObjectWrapper.mm:
+        (-[AccessibilityObjectWrapper accessibilityActionNames]):
+        (-[AccessibilityObjectWrapper accessibilityAttributeNames]):
+        (-[AccessibilityObjectWrapper accessibilityAttributeValue:]):
+        (-[AccessibilityObjectWrapper accessibilityFocusedUIElement]):
+        (-[AccessibilityObjectWrapper accessibilityHitTest:]):
+        (-[AccessibilityObjectWrapper accessibilityIsAttributeSettable:]):
+        (-[AccessibilityObjectWrapper accessibilityIsIgnored]):
+        (-[AccessibilityObjectWrapper accessibilityParameterizedAttributeNames]):
+        (-[AccessibilityObjectWrapper accessibilityPerformPressAction]):
+        (-[AccessibilityObjectWrapper accessibilityPerformAction:]):
+        (-[AccessibilityObjectWrapper accessibilitySetValue:forAttribute:]):
+        (-[AccessibilityObjectWrapper accessibilityAttributeValue:forParameter:]):
+        (-[AccessibilityObjectWrapper accessibilityIndexOfChild:]):
+        (-[AccessibilityObjectWrapper accessibilityArrayAttributeCount:]):
+        (-[AccessibilityObjectWrapper accessibilityArrayAttributeValues:index:maxCount:]):
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::destroy):
+        * rendering/RenderWidget.cpp:
+        (WebCore::RenderWidget::destroy):
+
 2009-01-16  Eric Seidel  <eric@webkit.org>
 
         Reviewed by Justin Garcia.
index 0b072cf..e868da2 100644 (file)
@@ -1028,4 +1028,8 @@ const String& AccessibilityObject::actionVerb() const
     }
 }
 
+void AccessibilityObject::updateBackingStore()
+{
+}
+    
 } // namespace WebCore
index 543d59c..a751003 100644 (file)
@@ -400,6 +400,10 @@ public:
     bool accessibilityIgnoreAttachment() const { return true; }
 #endif
 
+    // allows for an AccessibilityObject to update its render tree or perform
+    // other operations update type operations
+    virtual void updateBackingStore();
+    
 protected:
     unsigned m_id;
     AccessibilityChildrenVector m_children;
index b206250..070675a 100644 (file)
@@ -2215,12 +2215,20 @@ bool AccessibilityRenderObject::canSetTextRangeAttributes() const
 
 void AccessibilityRenderObject::childrenChanged()
 {
-    clearChildren();
+    // this method is meant as a quick way of marking dirty
+    // a portion of the accessibility tree
     
-    if (accessibilityIsIgnored()) {
-        AccessibilityObject* parent = parentObject();
-        if (parent)
-            parent->childrenChanged();
+    markChildrenDirty();
+    
+    // this object may not be accessible (and thus may not appear
+    // in the hierarchy), which means we need to go up the parent
+    // chain and mark the parent's dirty. Ideally, we would want
+    // to only access the next object that is not ignored, but
+    // asking an element if it's ignored can lead to an examination of the
+    // render tree which is dangerous.
+    for (AccessibilityObject* parent = parentObject(); parent; parent = parent->parentObject()) {
+        if (parent->isAccessibilityRenderObject())
+            static_cast<AccessibilityRenderObject *>(parent)->markChildrenDirty();
     }
 }
     
@@ -2244,6 +2252,11 @@ bool AccessibilityRenderObject::canHaveChildren() const
 
 const AccessibilityObject::AccessibilityChildrenVector& AccessibilityRenderObject::children()
 {
+    if (m_childrenDirty) {
+        clearChildren();        
+        m_childrenDirty = false;
+    }
+    
     if (!m_haveChildren)
         addChildren();
     return m_children;
@@ -2404,5 +2417,11 @@ const String& AccessibilityRenderObject::actionVerb() const
     }
 }
      
+void AccessibilityRenderObject::updateBackingStore()
+{
+    if (!m_renderer)
+        return;
+    m_renderer->view()->layoutIfNeeded();
+}    
     
 } // namespace WebCore
index 7bacf1d..ca87359 100644 (file)
@@ -209,9 +209,12 @@ public:
     virtual String doAXStringForRange(const PlainTextRange&) const;
     virtual IntRect doAXBoundsForRange(const PlainTextRange&) const;
     
+    virtual void updateBackingStore();
+    
 protected:
     RenderObject* m_renderer;
     AccessibilityRole m_ariaRole;
+    mutable bool m_childrenDirty;
     
     void setRenderObject(RenderObject* renderer) { m_renderer = renderer; }
     virtual void removeAXObjectID();
@@ -231,6 +234,7 @@ private:
     AccessibilityObject* internalLinkElement() const;
     AccessibilityObject* accessibilityParentForImageMap(HTMLMapElement* map) const;
 
+    void markChildrenDirty() const { m_childrenDirty = true; }
 };
     
 } // namespace WebCore
index ee54496..9409283 100644 (file)
@@ -53,13 +53,11 @@ AccessibilityTable::AccessibilityTable(RenderObject* renderer)
     : AccessibilityRenderObject(renderer),
     m_headerContainer(0)
 {
-    // FIXME: We need to disable Accessibility Tables entirely on the Mac until <rdar://problem/6372481> is resolved.
-#if PLATFORM(MAC)
+#if defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD)
     m_isAccessibilityTable = false;
-#else    
+#else
     m_isAccessibilityTable = isTableExposableThroughAccessibility();
 #endif
-
 }
 
 AccessibilityTable::~AccessibilityTable()
index ca0e57b..1e28f00 100644 (file)
@@ -580,6 +580,8 @@ static WebCoreTextMarkerRange* textMarkerRangeFromVisiblePositions(VisiblePositi
 
 - (NSArray*)accessibilityActionNames
 {
+    m_object->updateBackingStore();
+
     static NSArray* actionElementActions = [[NSArray alloc] initWithObjects: NSAccessibilityPressAction, NSAccessibilityShowMenuAction, nil];
     static NSArray* defaultElementActions = [[NSArray alloc] initWithObjects: NSAccessibilityShowMenuAction, nil];
     static NSArray* menuElementActions = [[NSArray alloc] initWithObjects: NSAccessibilityCancelAction, NSAccessibilityPressAction, nil];
@@ -599,6 +601,8 @@ static WebCoreTextMarkerRange* textMarkerRangeFromVisiblePositions(VisiblePositi
 
 - (NSArray*)accessibilityAttributeNames
 {
+    m_object->updateBackingStore();
+    
     if (m_object->isAttachment())
         return [[self attachmentView] accessibilityAttributeNames];
 
@@ -1124,6 +1128,8 @@ static NSString* roleValueToNSString(AccessibilityRole value)
     if (!m_object)
         return nil;
 
+    m_object->updateBackingStore();
+    
     if ([attributeName isEqualToString: NSAccessibilityRoleAttribute])
         return [self role];
 
@@ -1430,6 +1436,8 @@ static NSString* roleValueToNSString(AccessibilityRole value)
 
 - (id)accessibilityFocusedUIElement
 {
+    m_object->updateBackingStore();
+
     RefPtr<AccessibilityObject> focusedObj = m_object->focusedUIElement();
 
     if (!focusedObj)
@@ -1440,6 +1448,8 @@ static NSString* roleValueToNSString(AccessibilityRole value)
 
 - (id)accessibilityHitTest:(NSPoint)point
 {
+    m_object->updateBackingStore();
+
     RefPtr<AccessibilityObject> axObject = m_object->doAccessibilityHitTest(IntPoint(point));
     if (axObject)
         return NSAccessibilityUnignoredAncestor(axObject->wrapper());
@@ -1448,6 +1458,8 @@ static NSString* roleValueToNSString(AccessibilityRole value)
 
 - (BOOL)accessibilityIsAttributeSettable:(NSString*)attributeName
 {
+    m_object->updateBackingStore();
+
     if ([attributeName isEqualToString: @"AXSelectedTextMarkerRange"])
         return YES;
 
@@ -1482,6 +1494,8 @@ static NSString* roleValueToNSString(AccessibilityRole value)
 // Registering an object is also required for observing notifications. Only registered objects can be observed.
 - (BOOL)accessibilityIsIgnored
 {
+    m_object->updateBackingStore();
+
     if (m_object->isAttachment())
         return [[self attachmentView] accessibilityIsIgnored];
     return m_object->accessibilityIsIgnored();
@@ -1489,6 +1503,8 @@ static NSString* roleValueToNSString(AccessibilityRole value)
 
 - (NSArray* )accessibilityParameterizedAttributeNames
 {
+    m_object->updateBackingStore();
+
     if (m_object->isAttachment()) 
         return nil;
 
@@ -1569,6 +1585,8 @@ static NSString* roleValueToNSString(AccessibilityRole value)
 
 - (void)accessibilityPerformPressAction
 {
+    m_object->updateBackingStore();
+
     if (m_object->isAttachment())
         [[self attachmentView] accessibilityPerformAction:NSAccessibilityPressAction];
     
@@ -1613,6 +1631,8 @@ static NSString* roleValueToNSString(AccessibilityRole value)
 
 - (void)accessibilityPerformAction:(NSString*)action
 {
+    m_object->updateBackingStore();
+
     if ([action isEqualToString:NSAccessibilityPressAction])
         [self accessibilityPerformPressAction];
     
@@ -1622,6 +1642,8 @@ static NSString* roleValueToNSString(AccessibilityRole value)
 
 - (void)accessibilitySetValue:(id)value forAttribute:(NSString*)attributeName
 {
+    m_object->updateBackingStore();
+
     WebCoreTextMarkerRange* textMarkerRange = nil;
     NSNumber*               number = nil;
     NSString*               string = nil;
@@ -1744,6 +1766,8 @@ static RenderObject* rendererForView(NSView* view)
     if (!m_object || !attribute || !parameter)
         return nil;
 
+    m_object->updateBackingStore();
+    
     // common parameter type check/casting.  Nil checks in handlers catch wrong type case.
     // NOTE: This assumes nil is not a valid parameter, because it is indistinguishable from
     // a parameter of the wrong type.
@@ -1989,6 +2013,8 @@ static RenderObject* rendererForView(NSView* view)
 // API that AppKit uses for faster access
 - (NSUInteger)accessibilityIndexOfChild:(id)child
 {
+    m_object->updateBackingStore();
+    
     const AccessibilityObject::AccessibilityChildrenVector& children = m_object->children();
        
     if (children.isEmpty())
@@ -2005,6 +2031,8 @@ static RenderObject* rendererForView(NSView* view)
 
 - (NSUInteger)accessibilityArrayAttributeCount:(NSString *)attribute
 {
+    m_object->updateBackingStore();
+    
     if ([attribute isEqualToString:NSAccessibilityChildrenAttribute]) {
         const AccessibilityObject::AccessibilityChildrenVector& children = m_object->children();
         if (children.isEmpty())
@@ -2018,6 +2046,8 @@ static RenderObject* rendererForView(NSView* view)
 
 - (NSArray *)accessibilityArrayAttributeValues:(NSString *)attribute index:(NSUInteger)index maxCount:(NSUInteger)maxCount 
 {
+    m_object->updateBackingStore();
+    
     if ([attribute isEqualToString:NSAccessibilityChildrenAttribute]) {
         if (m_object->children().isEmpty()) {
             NSArray *children = [self renderWidgetChildren];
index 3a3cae4..45f11fc 100644 (file)
@@ -2622,9 +2622,10 @@ void RenderObject::destroy()
     if (m_hasCounterNodeMap)
         RenderCounter::destroyCounterNodes(this);
 
-    if (AXObjectCache::accessibilityEnabled())
+    if (AXObjectCache::accessibilityEnabled()) {
+        document()->axObjectCache()->childrenChanged(this->parent());
         document()->axObjectCache()->remove(this);
-
+    }
     animation()->cancelAnimations(this);
 
     // By default no ref-counting. RenderWidget::destroy() doesn't call
index 1a9744a..56fec80 100644 (file)
@@ -75,9 +75,10 @@ void RenderWidget::destroy()
     if (RenderView* v = view())
         v->removeWidget(this);
 
-    if (AXObjectCache::accessibilityEnabled())
+    if (AXObjectCache::accessibilityEnabled()) {
+        document()->axObjectCache()->childrenChanged(this->parent());
         document()->axObjectCache()->remove(this);
-
+    }
     remove();
 
     if (m_widget) {