AXObjectCache::childrenChanged shouldn't update layout or style during another style...
authorcfleizach@apple.com <cfleizach@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Feb 2019 09:28:44 +0000 (09:28 +0000)
committercfleizach@apple.com <cfleizach@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Feb 2019 09:28:44 +0000 (09:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182280
<rdar://problem/37018386>

Reviewed by Alan Bujtas.

Source/WebCore:

Remove the possibility that changing children calls back into updating layout by
handling children changes in a deferred manner.

This follows the same architecture as many other deferred changes, but also requires us to check deferred changes
in updateBackingStore, because things like aria-hidden changes won't trigger a layout, but will require us to update children.

A few tests had to be modified to no longer change the tree and then check the children immediately.

* accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::remove):
(WebCore::AXObjectCache::childrenChanged):
(WebCore::AXObjectCache::prepareForDocumentDestruction):
(WebCore::AXObjectCache::performDeferredCacheUpdate):
* accessibility/AXObjectCache.h:
* accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::updateBackingStore):
* accessibility/mac/WebAccessibilityObjectWrapperBase.mm:
(convertToNSArray):
(-[WebAccessibilityObjectWrapperBase updateObjectBackingStore]):

LayoutTests:

* accessibility/aria-hidden-update.html:
* accessibility/aria-hidden-updates-alldescendants.html:
* accessibility/image-load-on-delay.html:
* accessibility/mac/aria-hidden-changes-for-non-ignored-elements.html:
* accessibility/removed-anonymous-block-child-causes-crash.html:

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

LayoutTests/ChangeLog
LayoutTests/accessibility/aria-hidden-update.html
LayoutTests/accessibility/aria-hidden-updates-alldescendants.html
LayoutTests/accessibility/image-load-on-delay.html
LayoutTests/accessibility/mac/aria-hidden-changes-for-non-ignored-elements.html
LayoutTests/accessibility/removed-anonymous-block-child-causes-crash.html
Source/WebCore/ChangeLog
Source/WebCore/accessibility/AXObjectCache.cpp
Source/WebCore/accessibility/AXObjectCache.h
Source/WebCore/accessibility/AccessibilityObject.cpp
Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm

index 7d3bee2..cc6f273 100644 (file)
@@ -1,3 +1,17 @@
+2019-02-08  Chris Fleizach  <cfleizach@apple.com>
+
+        AXObjectCache::childrenChanged shouldn't update layout or style during another style recalc
+        https://bugs.webkit.org/show_bug.cgi?id=182280
+        <rdar://problem/37018386>
+
+        Reviewed by Alan Bujtas.
+
+        * accessibility/aria-hidden-update.html:
+        * accessibility/aria-hidden-updates-alldescendants.html:
+        * accessibility/image-load-on-delay.html:
+        * accessibility/mac/aria-hidden-changes-for-non-ignored-elements.html:
+        * accessibility/removed-anonymous-block-child-causes-crash.html:
+
 2019-02-11  Myles C. Maxfield  <mmaxfield@apple.com>
 
         [Cocoa] Ask platform for generic font family mappings
index c270a16..880f848 100644 (file)
@@ -18,6 +18,7 @@
      
     <script>
         if (window.accessibilityController) {
+            jsTestIsAsync = true;
             description("This test makes sure that when aria-hidden changes, the AX hierarchy is updated.");
 
             // Get the parent element.
 
             // Make the 2nd button hidden. Only 1 and 3 should be present.
             document.getElementById("button2").setAttribute("aria-hidden", "true");
-            shouldBeTrue("parent.childAtIndex(0).isEqual(button1)");
-            shouldBeTrue("parent.childAtIndex(1).isEqual(button3)");
+            setTimeout(function() {
+                shouldBeTrue("parent.childAtIndex(0).isEqual(button1)");
+                shouldBeTrue("parent.childAtIndex(1).isEqual(button3)");
            
-            // Make the 1st button hidden. Only 3 should be present.
-            document.getElementById("button1").setAttribute("aria-hidden", "true");
-            shouldBeTrue("parent.childAtIndex(0).isEqual(button3)");
-
-            // Make the 2nd button not hidden. 2 and 3 should be present.
-            document.getElementById("button2").setAttribute("aria-hidden", "false");
-            shouldBeTrue("parent.childAtIndex(0).isEqual(button2)");
-            shouldBeTrue("parent.childAtIndex(1).isEqual(button3)");
+                // Make the 1st button hidden. Only 3 should be present.
+                document.getElementById("button1").setAttribute("aria-hidden", "true");
+                setTimeout(function() {
+                    shouldBeTrue("parent.childAtIndex(0).isEqual(button3)");
 
+                    // Make the 2nd button not hidden. 2 and 3 should be present.
+                    document.getElementById("button2").setAttribute("aria-hidden", "false");
+                    setTimeout(function() {
+                        shouldBeTrue("parent.childAtIndex(0).isEqual(button2)");
+                        shouldBeTrue("parent.childAtIndex(1).isEqual(button3)");
+                        finishJSTest();
+                    }, 0);
+                }, 0);
+            }, 0);
         }
     </script>
 
index 4867e5b..9ccf12c 100644 (file)
@@ -26,6 +26,7 @@
     description("This tests that if aria-hidden changes on an element, all it's existing children will update their children caches");
 
     if (window.accessibilityController) {
+          jsTestIsAsync = true;
           document.getElementById("main").focus();
           
           var main = accessibilityController.focusedElement;
           var group = document.getElementsByTagName('main')[0];
           var items = group.getElementsByTagName('div');          
           items[0].removeAttribute('aria-hidden');
-
-          // After removing aria-hidden, the new count should be 2.
-          shouldBe("main.childrenCount", "2");          
+          setTimeout(function() {
+              // After removing aria-hidden, the new count should be 2.
+              shouldBe("main.childrenCount", "2");          
           
-          // And most importantly, the DIV that was made non-hidden should have one child now.
-          shouldBe("main.childAtIndex(1).childrenCount", "1");
+              // And most importantly, the DIV that was made non-hidden should have one child now.
+              shouldBe("main.childAtIndex(1).childrenCount", "1");
+              finishJSTest();
+          }, 0);
     }
 
 </script>
index 3681622..0239db6 100644 (file)
 
           document.getElementById("image").onload = function() {
               document.body.offsetHeight;
-              debug("AFTER: Group count: " + content.childrenCount);
-              finishJSTest();
+              setTimeout(function() {
+                  debug("AFTER: Group count: " + content.childrenCount);
+                  finishJSTest();
+              }, 0);
           };
 
           setTimeout(function() { 
index a209309..277299e 100644 (file)
@@ -25,7 +25,7 @@
     description("This tests that when aria-hidden is toggled, it will clear out the cached children for non-ignored elements that are descendants.");
 
     if (window.accessibilityController) {
-
+        jsTestIsAsync = true;
         document.getElementById("body").focus();
         var body = accessibilityController.focusedElement;
 
 
         // Toggle aria-hidden, and we should not be able to access the same elements anymore.
         document.getElementById("main").setAttribute("aria-hidden", "true");
-        shouldBe("body.childAtIndex(0).role", "'AXRole: AXCheckBox'");
-
-        // Toggle aria-hidden off again and we should again be able to access elements inside the tab panel.
-        document.getElementById("main").setAttribute("aria-hidden", "false");
-        shouldBe("body.childAtIndex(3).childAtIndex(0).role", "'AXRole: AXButton'");
+        setTimeout(function() {
+            shouldBe("body.childAtIndex(0).role", "'AXRole: AXCheckBox'");
+
+            // Toggle aria-hidden off again and we should again be able to access elements inside the tab panel.
+            document.getElementById("main").setAttribute("aria-hidden", "false");
+            setTimeout(function() {
+                shouldBe("body.childAtIndex(3).childAtIndex(0).role", "'AXRole: AXButton'");
+                finishJSTest();
+            }, 0);
+        }, 0);
 
     }
 
index c0fe724..96a8206 100644 (file)
@@ -8,6 +8,8 @@
     }\r
 \r
     function queryIsEnabledOnDecendants(accessibilityObject) {\r
+        if (!accessibilityObject)\r
+            return;\r
         accessibilityObject.isEnabled\r
 \r
         var count = accessibilityObject.childrenCount;\r
index 18ee725..b9b95ab 100644 (file)
@@ -1,3 +1,31 @@
+2019-02-08  Chris Fleizach  <cfleizach@apple.com>
+
+        AXObjectCache::childrenChanged shouldn't update layout or style during another style recalc
+        https://bugs.webkit.org/show_bug.cgi?id=182280
+        <rdar://problem/37018386>
+
+        Reviewed by Alan Bujtas.
+
+        Remove the possibility that changing children calls back into updating layout by
+        handling children changes in a deferred manner.
+
+        This follows the same architecture as many other deferred changes, but also requires us to check deferred changes
+        in updateBackingStore, because things like aria-hidden changes won't trigger a layout, but will require us to update children.
+
+        A few tests had to be modified to no longer change the tree and then check the children immediately. 
+
+        * accessibility/AXObjectCache.cpp:
+        (WebCore::AXObjectCache::remove):
+        (WebCore::AXObjectCache::childrenChanged):
+        (WebCore::AXObjectCache::prepareForDocumentDestruction):
+        (WebCore::AXObjectCache::performDeferredCacheUpdate):
+        * accessibility/AXObjectCache.h:
+        * accessibility/AccessibilityObject.cpp:
+        (WebCore::AccessibilityObject::updateBackingStore):
+        * accessibility/mac/WebAccessibilityObjectWrapperBase.mm:
+        (convertToNSArray):
+        (-[WebAccessibilityObjectWrapperBase updateObjectBackingStore]):
+
 2019-02-11  Myles C. Maxfield  <mmaxfield@apple.com>
 
         [Cocoa] Ask platform for generic font family mappings
index f032148..238a500 100644 (file)
@@ -747,6 +747,7 @@ void AXObjectCache::remove(Node& node)
     if (is<Element>(node)) {
         m_deferredRecomputeIsIgnoredList.remove(downcast<Element>(&node));
         m_deferredSelectedChildredChangedList.remove(downcast<Element>(&node));
+        m_deferredChildrenChangedNodeList.remove(&node);
         m_deferredTextFormControlValue.remove(downcast<Element>(&node));
         m_deferredAttributeChange.remove(downcast<Element>(&node));
     }
@@ -859,10 +860,8 @@ void AXObjectCache::handleLiveRegionCreated(Node* node)
     
 void AXObjectCache::childrenChanged(Node* node, Node* newChild)
 {
-    if (newChild) {
-        handleMenuOpened(newChild);
-        handleLiveRegionCreated(newChild);
-    }
+    if (newChild)
+        m_deferredChildrenChangedNodeList.add(newChild);
 
     childrenChanged(get(node));
 }
@@ -872,14 +871,9 @@ void AXObjectCache::childrenChanged(RenderObject* renderer, RenderObject* newChi
     if (!renderer)
         return;
 
-    // FIXME: Refactor the code to avoid calling updateLayout in this call stack.
-    ScriptDisallowedScope::LayoutAssertionDisableScope disableScope;
-    
-    if (newChild) {
-        handleMenuOpened(newChild->node());
-        handleLiveRegionCreated(newChild->node());
-    }
-    
+    if (newChild && newChild->node())
+        m_deferredChildrenChangedNodeList.add(newChild->node());
+
     childrenChanged(get(renderer));
 }
 
@@ -888,7 +882,7 @@ void AXObjectCache::childrenChanged(AccessibilityObject* obj)
     if (!obj)
         return;
 
-    obj->childrenChanged();
+    m_deferredChildredChangedList.add(obj);
 }
     
 void AXObjectCache::notificationPostTimerFired()
@@ -2854,6 +2848,7 @@ void AXObjectCache::prepareForDocumentDestruction(const Document& document)
     filterListForRemoval(m_deferredRecomputeIsIgnoredList, document, nodesToRemove);
     filterListForRemoval(m_deferredTextChangedList, document, nodesToRemove);
     filterListForRemoval(m_deferredSelectedChildredChangedList, document, nodesToRemove);
+    filterListForRemoval(m_deferredChildrenChangedNodeList, document, nodesToRemove);
     filterMapForRemoval(m_deferredTextFormControlValue, document, nodesToRemove);
     filterMapForRemoval(m_deferredAttributeChange, document, nodesToRemove);
     filterVectorPairForRemoval(m_deferredFocusedNodeChange, document, nodesToRemove);
@@ -2886,6 +2881,17 @@ void AXObjectCache::performDeferredCacheUpdate()
         return;
 
     SetForScope<bool> performingDeferredCacheUpdate(m_performingDeferredCacheUpdate, true);
+
+    for (auto* nodeChild : m_deferredChildrenChangedNodeList) {
+        handleMenuOpened(nodeChild);
+        handleLiveRegionCreated(nodeChild);
+    }
+    m_deferredChildrenChangedNodeList.clear();
+
+    for (auto& child : m_deferredChildredChangedList)
+        child->childrenChanged();
+    m_deferredChildredChangedList.clear();
+
     for (auto* node : m_deferredTextChangedList)
         textChanged(node);
     m_deferredTextChangedList.clear();
index c8afafb..1956e1c 100644 (file)
@@ -469,6 +469,8 @@ private:
     ListHashSet<Element*> m_deferredRecomputeIsIgnoredList;
     ListHashSet<Node*> m_deferredTextChangedList;
     ListHashSet<Element*> m_deferredSelectedChildredChangedList;
+    ListHashSet<RefPtr<AccessibilityObject>> m_deferredChildredChangedList;
+    ListHashSet<Node*> m_deferredChildrenChangedNodeList;
     HashMap<Element*, String> m_deferredTextFormControlValue;
     HashMap<Element*, QualifiedName> m_deferredAttributeChange;
     Vector<std::pair<Node*, Node*>> m_deferredFocusedNodeChange;
index df1331d..aeed718 100644 (file)
@@ -1786,6 +1786,10 @@ void AccessibilityObject::updateBackingStore()
         if (!document->view()->layoutContext().isInRenderTreeLayout() && !document->inRenderTreeUpdate() && !document->inStyleRecalc())
             document->updateLayoutIgnorePendingStylesheets();
     }
+
+    if (auto cache = axObjectCache())
+        cache->performDeferredCacheUpdate();
+    
     updateChildrenIfNecessary();
 }
 #endif
index 44d3d69..814e1a3 100644 (file)
@@ -258,15 +258,15 @@ NSArray *convertToNSArray(const AccessibilityObject::AccessibilityChildrenVector
     NSMutableArray *array = [NSMutableArray arrayWithCapacity:vector.size()];
     for (const auto& child : vector) {
         auto wrapper = (WebAccessibilityObjectWrapperBase *)child->wrapper();
-        ASSERT(wrapper);
-        if (wrapper) {
-            // We want to return the attachment view instead of the object representing the attachment,
-            // otherwise, we get palindrome errors in the AX hierarchy.
-            if (child->isAttachment() && [wrapper attachmentView])
-                [array addObject:[wrapper attachmentView]];
-            else
-                [array addObject:wrapper];
-        }
+        if (!wrapper)
+            continue;
+
+        // We want to return the attachment view instead of the object representing the attachment,
+        // otherwise, we get palindrome errors in the AX hierarchy.
+        if (child->isAttachment() && [wrapper attachmentView])
+            [array addObject:[wrapper attachmentView]];
+        else
+            [array addObject:wrapper];
     }
     return [[array copy] autorelease];
 }