Notify observers that AXTreeManager will be removed from map

In some cases, we can give a new AXTreeID to an existing AXTree,
replacing the key associated to the tree's AXTreeManager in the
AXTreeManagerMap for the new tree id. This happens on a page reload or
on same-page navigation.

We store a AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints -
which is an AXTreeObserver on the AXTree. When such a scenario happens,
the AXPlatformNodeTextRangeProviderWin loses its access to the access
because its nodes are not referencing the new tree id. we then start our
destruction process and try to remove the observer from the tree's
obersers list - without success, because we can't get the AXTreeManager
from the previous tree id. This causes us to keep a reference to a
destroyed object in the observers list.

To avoid this issue, the AXTreeManagerMap is now notifying the observers
about its removal right before it happens. This allows us to remove the
observer before if needed.

Bug: N/A
AX-Relnotes: Fix crash occurring on page reload with UIA enabled.
Change-Id: I6fe6b35764e3c7c3e7391733217b84bb43201a58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2596013
Commit-Queue: Benjamin Beaudry <benjamin.beaudry@microsoft.com>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838192}

content/browser/accessibility/ax_platform_node_textrangeprovider_win_browsertest.cc

@@ -25,6 +25,9 @@ using Microsoft::WRL::ComPtr;
 
 namespace content {
 
+#define ASSERT_UIA_ELEMENTNOTAVAILABLE(expr) \
+  ASSERT_EQ(static_cast<HRESULT>(UIA_E_ELEMENTNOTAVAILABLE), (expr))
+
 #define EXPECT_UIA_DOUBLE_SAFEARRAY_EQ(safearray, expected_property_values) \
   {                                                                         \
     EXPECT_EQ(sizeof(V_R8(LPVARIANT(NULL))),                                \
@@ -2659,7 +2662,7 @@ IN_PROC_BROWSER_TEST_F(AXPlatformNodeTextRangeProviderWinBrowserTest,
     EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"Before frame");
   }
 
-  // 1. Test when |start_| is not in the iframe but |end_| is.
+  // 2. Test when |start_| is not in the iframe but |end_| is.
   {
     ComPtr<ITextRangeProvider> text_range_provider;
     GetTextRangeProviderFromTextNode(*before_frame_node, &text_range_provider);
@@ -2688,7 +2691,7 @@ IN_PROC_BROWSER_TEST_F(AXPlatformNodeTextRangeProviderWinBrowserTest,
     EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"Before frame\nText ");
   }
 
-  // 1. Test when |start_| is in the iframe but |end_| is not.
+  // 3. Test when |start_| is in the iframe but |end_| is not.
   {
     ComPtr<ITextRangeProvider> text_range_provider;
     auto* after_frame_node =
@@ -2720,6 +2723,85 @@ IN_PROC_BROWSER_TEST_F(AXPlatformNodeTextRangeProviderWinBrowserTest,
   }
 }
 
+// Test that a page reload removes the AXTreeObserver from the AXTree's
+// observers list. If it doesn't, this test will crash.
+IN_PROC_BROWSER_TEST_F(AXPlatformNodeTextRangeProviderWinBrowserTest,
+                       ReloadTreeShouldRemoveObserverFromTree) {
+  LoadInitialAccessibilityTreeFromHtmlFilePath(
+      "/accessibility/html/simple_spans.html");
+  auto* web_contents = static_cast<WebContentsImpl*>(shell()->web_contents());
+  WaitForAccessibilityTreeToContainNodeWithName(web_contents, "Some text");
+
+  // 1. Reload the page and trigger a tree update - this should update the tree
+  // id without modifying the observers from the tree.
+  {
+    auto* node = FindNode(ax::mojom::Role::kStaticText, "Some text");
+    ASSERT_NE(nullptr, node);
+
+    ComPtr<ITextRangeProvider> text_range_provider;
+    GetTextRangeProviderFromTextNode(*node, &text_range_provider);
+    ASSERT_NE(nullptr, text_range_provider.Get());
+    EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"Some text");
+    AXTreeID old_tree_id = GetManager()->GetTreeID();
+
+    // Reloading changes the tree id, triggering an AXTreeManager replacement.
+    shell()->Reload();
+
+    AccessibilityNotificationWaiter waiter(web_contents, ui::kAXModeComplete,
+                                           ax::mojom::Event::kChildrenChanged);
+
+    // We do a style change here only to trigger an AXTree update - apparently,
+    // a shell reload doesn't update the tree by itself.
+    EXPECT_TRUE(ExecuteScript(
+        web_contents,
+        "document.getElementById('s1').style.outline = '1px solid black';"));
+
+    waiter.WaitForNotification();
+    ASSERT_NE(old_tree_id, GetManager()->GetTreeID());
+
+    // |text_range_provider| should now be invalid since it is using nodes
+    // pointing to the previous tree id. If the tree id has not been updated
+    // from the page reload, this should fail.
+    base::win::ScopedSafearray children;
+    ASSERT_UIA_ELEMENTNOTAVAILABLE(
+        text_range_provider->GetChildren(children.Receive()));
+  }
+
+  // 2. Validate that the observer for the previous range has been removed. Also
+  // test that the new observer has been added correctly.
+  {
+    auto* node = FindNode(ax::mojom::Role::kStaticText, "Some text");
+    ASSERT_NE(nullptr, node);
+
+    ComPtr<ITextRangeProvider> text_range_provider;
+    GetTextRangeProviderFromTextNode(*node, &text_range_provider);
+    ASSERT_NE(nullptr, text_range_provider.Get());
+    EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"Some text");
+
+    // Make the range span the entire document.
+    EXPECT_UIA_MOVE_ENDPOINT_BY_UNIT(
+        text_range_provider, TextPatternRangeEndpoint_End, TextUnit_Paragraph,
+        /*count*/ 1,
+        /*expected_text*/ L"Some text 3.14159",
+        /*expected_count*/ 1);
+
+    AccessibilityNotificationWaiter waiter(web_contents, ui::kAXModeComplete,
+                                           ax::mojom::Event::kChildrenChanged);
+
+    // We do a style change here only to trigger an AXTree update.
+    EXPECT_TRUE(ExecuteScript(
+        web_contents,
+        "document.getElementById('s2').style.outline = '1px solid black';"));
+
+    waiter.WaitForNotification();
+
+    // If the previous observer was not removed correctly, this will cause a
+    // crash. If it was removed correctly and this EXPECT fails, it's likely
+    // because the new observer has not been added as expected.
+    EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"Some text 3.14159");
+  }
+}
+
 IN_PROC_BROWSER_TEST_F(AXPlatformNodeTextRangeProviderWinBrowserTest,
                        GetAttributeValueNormalizesClonedRange) {
   LoadInitialAccessibilityTreeFromHtml(

content/browser/accessibility/browser_accessibility_manager.cc

@@ -1513,6 +1513,13 @@ ui::AXNode* BrowserAccessibilityManager::GetParentNodeFromParentTreeAsAXNode()
   return nullptr;
 }
 
+void BrowserAccessibilityManager::WillBeRemovedFromMap() {
+  if (!ax_tree())
+    return;
+
+  ax_tree()->NotifyTreeManagerWillBeRemoved(ax_tree_id_);
+}
+
 BrowserAccessibilityManager* BrowserAccessibilityManager::GetRootManager()
     const {
   BrowserAccessibility* parent = GetParentNodeFromParentTree();

content/browser/accessibility/browser_accessibility_manager.h

@@ -469,6 +469,7 @@ class CONTENT_EXPORT BrowserAccessibilityManager : public ui::AXTreeObserver,
   AXTreeID GetParentTreeID() const override;
   ui::AXNode* GetRootAsAXNode() const override;
   ui::AXNode* GetParentNodeFromParentTreeAsAXNode() const override;
+  void WillBeRemovedFromMap() override;
 
   BrowserAccessibilityDelegate* delegate() const { return delegate_; }
 

content/test/data/accessibility/html/simple_spans.html

+<!DOCTYPE html>
+<html>
+
+<body>
+  <span id="s1">Some text</span>
+  <span id="s2">3.14159</span>
+</body>
+
+</html>
\ No newline at end of file

ui/accessibility/ax_tree.cc

@@ -2435,6 +2435,14 @@ bool AXTree::HasPaginationSupport() const {
   return has_pagination_support_;
 }
 
+void AXTree::NotifyTreeManagerWillBeRemoved(AXTreeID previous_tree_id) {
+  if (previous_tree_id == AXTreeIDUnknown())
+    return;
+
+  for (AXTreeObserver& observer : observers_)
+    observer.OnTreeManagerWillBeRemoved(previous_tree_id);
+}
+
 void AXTree::RecordError(std::string new_error) {
   if (!error_.empty())
     error_ = error_ + "\n";  // Add visual separation between errors.

ui/accessibility/ax_tree.h

@@ -175,6 +175,13 @@ class AX_EXPORT AXTree : public AXNode::OwnerTree {
     return event_intents_;
   }
 
+  // Notify the delegate that the tree manager for |previous_tree_id| will be
+  // removed from the AXTreeManagerMap. Because we sometimes remove the tree
+  // manager after the tree's id has been modified, we need to pass the (old)
+  // tree id associated with the manager we are removing even though it is the
+  // same tree.
+  void NotifyTreeManagerWillBeRemoved(AXTreeID previous_tree_id);
+
  private:
   friend class AXTableInfoTest;
 

ui/accessibility/ax_tree_manager.h

@@ -44,6 +44,10 @@ class AX_EXPORT AXTreeManager {
   // hosts the current tree. Returns nullptr if this tree doesn't have a parent
   // tree.
   virtual AXNode* GetParentNodeFromParentTreeAsAXNode() const = 0;
+
+  // Called when the tree manager is about to be removed from the tree
+  // AXTreeManagerMap.
+  virtual void WillBeRemovedFromMap() {}
 };
 
 }  // namespace ui

ui/accessibility/ax_tree_manager_map.cc

@@ -26,8 +26,10 @@ void AXTreeManagerMap::AddTreeManager(AXTreeID tree_id,
 }
 
 void AXTreeManagerMap::RemoveTreeManager(AXTreeID tree_id) {
-  if (tree_id != AXTreeIDUnknown())
+  if (auto* manager = GetManager(tree_id)) {
+    manager->WillBeRemovedFromMap();
     map_.erase(tree_id);
+  }
 }
 
 AXTreeManager* AXTreeManagerMap::GetManager(AXTreeID tree_id) {

ui/accessibility/ax_tree_observer.h

@@ -8,6 +8,7 @@
 #include "base/observer_list_types.h"
 #include "ui/accessibility/ax_enums.mojom-forward.h"
 #include "ui/accessibility/ax_export.h"
+#include "ui/accessibility/ax_tree_id.h"
 
 namespace ui {
 
@@ -126,6 +127,15 @@ class AX_EXPORT AXTreeObserver : public base::CheckedObserver {
   // updating.
   virtual void OnNodeChanged(AXTree* tree, AXNode* node) {}
 
+  // Called just before a tree manager is removed from the AXTreeManagerMap.
+  //
+  // Why is this needed?
+  // In some cases, we update the tree id of an AXTree and need to update the
+  // map entry that corresponds to that tree. The observers maintained in the
+  // observers list of that AXTree might need to be notified of that change to
+  // remove themselves from the list, if needed.
+  virtual void OnTreeManagerWillBeRemoved(AXTreeID previous_tree_id) {}
+
   enum ChangeType {
     NODE_CREATED,
     SUBTREE_CREATED,

ui/accessibility/platform/ax_platform_node_textrangeprovider_win.cc

@@ -1543,4 +1543,12 @@ void AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::
   }
 }
 
+void AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::
+    OnTreeManagerWillBeRemoved(AXTreeID previous_tree_id) {
+  if (start_->tree_id() == previous_tree_id ||
+      end_->tree_id() == previous_tree_id) {
+    RemoveObserver(previous_tree_id);
+  }
+}
+
 }  // namespace ui

ui/accessibility/platform/ax_platform_node_textrangeprovider_win.h

@@ -211,6 +211,7 @@ class AX_EXPORT __declspec(uuid("3071e40d-a10d-45ff-a59f-6e8e1138e2c1"))
     void AddObserver(const AXTreeID tree_id);
     void RemoveObserver(const AXTreeID tree_id);
     void OnNodeWillBeDeleted(AXTree* tree, AXNode* node) override;
+    void OnTreeManagerWillBeRemoved(AXTreeID previous_tree_id) override;
 
    private:
     AXPositionInstance start_;