From 4d70022b8841a17850dcb9d670f23b3ab6f3bb93 Mon Sep 17 00:00:00 2001 From: Qasid Ahmad Sadiq Date: Thu, 21 Feb 2019 21:06:22 -0800 Subject: [PATCH] Revert "Fix a11y cache correctness bug" This fix introduced a painful crash that ends up disabling accessibility services for certain users. This happens when a client of AccessibilityCache tries to add a node, with the same id as a node previously in the cache, but fewer children, where the removed child is not in the cache. This is because, when children are removed, and a the node is updated, the cache tries to clear the child trees. But if the child is not in the cache, the cache clears the whole tree. Every node is recycled. Then the original node being replaced is attempted to be recycled again, and voila crash. The fix also didn't fix the original issue based on the discussion in b/114133438. The risk for this is pretty low, since nothing was built on top of this. This reverts commit 2f69c16c3dd0ea9522deebd9141ccc189b4a118e. Bug: 124676705 Test: Tested to see if above usecase still happens. Change-Id: I8a39698c4532a1613ba47e1c6ca70201cd496212 --- .../accessibility/AccessibilityCache.java | 14 +++---------- .../accessibility/AccessibilityCacheTest.java | 20 ------------------- 2 files changed, 3 insertions(+), 31 deletions(-) diff --git a/core/java/android/view/accessibility/AccessibilityCache.java b/core/java/android/view/accessibility/AccessibilityCache.java index 0e1e379d610a9..da5a1cd67922b 100644 --- a/core/java/android/view/accessibility/AccessibilityCache.java +++ b/core/java/android/view/accessibility/AccessibilityCache.java @@ -418,28 +418,20 @@ public class AccessibilityCache { * * @param nodes The nodes in the hosting window. * @param rootNodeId The id of the root to evict. - * - * @return {@code true} if the cache was cleared */ - private boolean clearSubTreeRecursiveLocked(LongSparseArray nodes, + private void clearSubTreeRecursiveLocked(LongSparseArray nodes, long rootNodeId) { AccessibilityNodeInfo current = nodes.get(rootNodeId); if (current == null) { - // The node isn't in the cache, but its descendents might be. - clear(); - return true; + return; } nodes.remove(rootNodeId); final int childCount = current.getChildCount(); for (int i = 0; i < childCount; i++) { final long childNodeId = current.getChildId(i); - if (clearSubTreeRecursiveLocked(nodes, childNodeId)) { - current.recycle(); - return true; - } + clearSubTreeRecursiveLocked(nodes, childNodeId); } current.recycle(); - return false; } /** diff --git a/core/tests/coretests/src/android/view/accessibility/AccessibilityCacheTest.java b/core/tests/coretests/src/android/view/accessibility/AccessibilityCacheTest.java index 993378d8a4d04..4de8155663f5b 100644 --- a/core/tests/coretests/src/android/view/accessibility/AccessibilityCacheTest.java +++ b/core/tests/coretests/src/android/view/accessibility/AccessibilityCacheTest.java @@ -299,26 +299,6 @@ public class AccessibilityCacheTest { } } - @Test - public void subTreeChangeEventFromUncachedNode_clearsNodeInCache() { - AccessibilityNodeInfo nodeInfo = getNodeWithA11yAndWindowId(CHILD_VIEW_ID, WINDOW_ID_1); - long id = nodeInfo.getSourceNodeId(); - mAccessibilityCache.add(nodeInfo); - nodeInfo.recycle(); - - AccessibilityEvent event = AccessibilityEvent - .obtain(AccessibilityEvent.TYPE_WINDOW_CONTENT_CHANGED); - event.setContentChangeTypes(AccessibilityEvent.CONTENT_CHANGE_TYPE_SUBTREE); - event.setSource(getMockViewWithA11yAndWindowIds(PARENT_VIEW_ID, WINDOW_ID_1)); - - mAccessibilityCache.onAccessibilityEvent(event); - AccessibilityNodeInfo shouldBeNull = mAccessibilityCache.getNode(WINDOW_ID_1, id); - if (shouldBeNull != null) { - shouldBeNull.recycle(); - } - assertNull(shouldBeNull); - } - @Test public void scrollEvent_clearsNodeAndChild() { AccessibilityEvent event = AccessibilityEvent