From 551ccd1418a14d9f4a4dc1a52da82cc8f8e99ffa Mon Sep 17 00:00:00 2001
From: Avi Drissman <avi@chromium.org>
Date: Thu, 30 Jul 2026 08:05:00 -0700
Subject: [PATCH] [a11y][mac] Give BrowserAccessibilityCocoa stable identity

-isEqual: was implemented as hash equality, and -hash returned the
per-tree AXNodeID while the wrapper was active and [super hash] once
detached. AXNodeID is only unique within a single tree, so wrappers from
different frames could compare equal, and -hash changed mid-lifetime for
objects that AppKit may still be holding in hash-keyed collections.

Fix this by using the unique id of the nodes, and caching it.

Fixed: 513135531
Change-Id: Ie1c4136a3dcdac0b3d7b0517e04941346a6a6964
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8156962
Commit-Queue: Avi Drissman <avi@chromium.org>
Auto-Submit: Avi Drissman <avi@chromium.org>
Reviewed-by: David Tseng <dtseng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1671071}
---

diff --git a/ui/accessibility/platform/ax_node_id_delegate.h b/ui/accessibility/platform/ax_node_id_delegate.h
index e05d51b..70b5abe 100644
--- a/ui/accessibility/platform/ax_node_id_delegate.h
+++ b/ui/accessibility/platform/ax_node_id_delegate.h
@@ -28,13 +28,7 @@
   virtual void OnAXNodeDeleted(AXNodeID ax_node_id) = 0;
 
  protected:
-  using PassKey = base::PassKey<AXNodeIdDelegate>;
-
   AXNodeIdDelegate() = default;
-
-  // Returns a PassKey to be used by implementations so that they may create
-  // AXPlatformNodeId instances with values of their own choosing.
-  static constexpr PassKey MakePassKey() { return PassKey(); }
 };
 
 }  // namespace ui
diff --git a/ui/accessibility/platform/ax_platform_node_id.h b/ui/accessibility/platform/ax_platform_node_id.h
index e62e936..1ab7121 100644
--- a/ui/accessibility/platform/ax_platform_node_id.h
+++ b/ui/accessibility/platform/ax_platform_node_id.h
@@ -12,8 +12,6 @@
 
 namespace ui {
 
-class AXNodeIdDelegate;
-
 // The underlying type of a uniquely-generated accessibility node identifier for
 // use with the platform. The actual id store can only a accommodate 32 bit
 // integers on Windows, because IAccessible2 uses LONG for get_uniqueID() and
@@ -37,12 +35,6 @@
   constexpr AXPlatformNodeId()
       : base::StrongAlias<class AXPlatformNodeIdTag, int32_t>::StrongAlias(0) {}
 
-  // Allow implementations of AXNodeIdDelegate to create instances, as they are
-  // responsible for per-window allocation of unique identifiers.
-  constexpr explicit AXPlatformNodeId(base::PassKey<AXNodeIdDelegate>,
-                                      int32_t v)
-      : base::StrongAlias<class AXPlatformNodeIdTag, int32_t>::StrongAlias(v) {}
-
   // Allow implicit conversion to the platform's node id type.
   constexpr operator const int32_t&() const { return value_; }
 
diff --git a/ui/accessibility/platform/browser_accessibility_cocoa.mm b/ui/accessibility/platform/browser_accessibility_cocoa.mm
index 03984bef..03d484e7 100644
--- a/ui/accessibility/platform/browser_accessibility_cocoa.mm
+++ b/ui/accessibility/platform/browser_accessibility_cocoa.mm
@@ -495,12 +495,19 @@
 @end
 
 @implementation BrowserAccessibilityCocoa {
-  // Dangling pointer https://crbug.com/1475830.
+  // Dangling pointer https://crbug.com/40928052.
   raw_ptr<ui::BrowserAccessibility, DanglingUntriaged> _owner;
+
+  // The global unique ID of the `_owner`. Cached because this value might be
+  // needed for comparisons/hashing after the owner value is reset.
+  ui::AXPlatformNodeId _uniqueId;
+
   // An array of children of this object. Cached to avoid re-computing.
   NSMutableArray* __strong _children;
+
   // 1-byte bitfield bundle. Main thread only (AppKit / NSAccessibility).
   BrowserAccessibilityCocoaBitfields _bitfields;
+
   // Stores the previous value of an edit field.
   std::u16string _oldValue;
 }
@@ -560,6 +567,7 @@
               withPlatformNode:(ui::AXPlatformNodeMac*)platform_node {
   if ((self = [super initWithNode:platform_node])) {
     _owner = accessibility;
+    _uniqueId = static_cast<ui::AXPlatformNodeDelegate*>(_owner)->GetUniqueId();
     _bitfields.gettingChildren = 0;
     _bitfields.emptyGroupCache = kEmptyGroupCacheUnknown;
   }
@@ -3556,16 +3564,16 @@
 }
 
 - (BOOL)isEqual:(id)object {
-  if (![object isKindOfClass:[BrowserAccessibilityCocoa class]])
-    return NO;
-  return ([self hash] == [object hash]);
+  if (BrowserAccessibilityCocoa* objectAccessibility =
+          base::apple::ObjCCast<BrowserAccessibilityCocoa>(object)) {
+    return _uniqueId == objectAccessibility->_uniqueId;
+  }
+
+  return NO;
 }
 
 - (NSUInteger)hash {
-  // Potentially called during dealloc.
-  if (![self instanceActive])
-    return [super hash];
-  return _owner->GetId();
+  return _uniqueId;
 }
 
 - (BOOL)accessibilityNotifiesWhenDestroyed {
diff --git a/ui/accessibility/platform/browser_accessibility_mac_unittest.mm b/ui/accessibility/platform/browser_accessibility_mac_unittest.mm
index c11015c..2cd7ef28 100644
--- a/ui/accessibility/platform/browser_accessibility_mac_unittest.mm
+++ b/ui/accessibility/platform/browser_accessibility_mac_unittest.mm
@@ -234,6 +234,46 @@
   EXPECT_NSEQ(nil, retainedFirstChild.accessibilityLabel);
 }
 
+// AppKit may retain a wrapper and key it in a hash-based collection, so
+// -hash must remain stable for the lifetime of the wrapper, and distinct
+// wrappers must never compare equal even if their backing nodes happen to
+// share the same per-tree id.
+TEST_F(BrowserAccessibilityMacTest, IdentityIsPerWrapperAndStable) {
+  // Build a second, independent tree whose root re-uses the same per-tree id
+  // as a node in the fixture's tree.
+  AXNodeData other_root;
+  other_root.id = 1000;
+  other_root.role = ax::mojom::Role::kRootWebArea;
+  TestAXNodeIdDelegate other_node_id_delegate;
+  std::unique_ptr<BrowserAccessibilityManager> other_manager =
+      std::make_unique<BrowserAccessibilityManagerMac>(
+          MakeAXTreeUpdateForTesting(other_root), other_node_id_delegate,
+          nullptr);
+  BrowserAccessibilityCocoa* other_wrapper =
+      base::apple::ObjCCastStrict<BrowserAccessibilityCocoa>(
+          other_manager->GetBrowserAccessibilityRoot()
+              ->GetNativeViewAccessible()
+              .Get());
+
+  ASSERT_NE(accessibility_, other_wrapper);
+  EXPECT_FALSE([accessibility_ isEqual:other_wrapper]);
+  EXPECT_FALSE([other_wrapper isEqual:accessibility_]);
+
+  // Hold the wrapper past detach, as the system might.
+  NS_VALID_UNTIL_END_OF_SCOPE BrowserAccessibilityCocoa* retained =
+      accessibility_;
+  const NSUInteger hash_before = retained.hash;
+  EXPECT_TRUE([retained isEqual:retained]);
+
+  // Tearing down the manager detaches the wrapper.
+  manager_.reset();
+  ASSERT_FALSE([retained instanceActive]);
+
+  EXPECT_EQ(hash_before, retained.hash);
+  EXPECT_TRUE([retained isEqual:retained]);
+  EXPECT_FALSE([retained isEqual:other_wrapper]);
+}
+
 TEST_F(BrowserAccessibilityMacTest, TestComputeTextEdit) {
   root_ = AXNodeData();
   root_.id = 1;
diff --git a/ui/accessibility/platform/test_ax_node_id_delegate.cc b/ui/accessibility/platform/test_ax_node_id_delegate.cc
index 39e1a1b..e2d609f 100644
--- a/ui/accessibility/platform/test_ax_node_id_delegate.cc
+++ b/ui/accessibility/platform/test_ax_node_id_delegate.cc
@@ -6,10 +6,21 @@
 
 namespace ui {
 
-AXPlatformNodeId TestAXNodeIdDelegate::GetOrCreateAXNodeUniqueId(
-    AXNodeID ax_node_id) {
-  // Per-tab uniqueness is not necessary in tests, so return the blink node id.
-  return AXPlatformNodeId(MakePassKey(), ax_node_id);
+TestAXNodeIdDelegate::TestAXNodeIdDelegate() = default;
+TestAXNodeIdDelegate::~TestAXNodeIdDelegate() = default;
+
+ui::AXPlatformNodeId TestAXNodeIdDelegate::GetOrCreateAXNodeUniqueId(
+    ui::AXNodeID ax_node_id) {
+  auto [iter, inserted] =
+      ax_unique_ids_.try_emplace(ax_node_id, ui::AXUniqueId::CreateInvalid());
+  if (inserted) {
+    iter->second = ui::AXUniqueId::Create();
+  }
+  return iter->second;
+}
+
+void TestAXNodeIdDelegate::OnAXNodeDeleted(ui::AXNodeID ax_node_id) {

diff --git a/ui/accessibility/platform/browser_accessibility_mac_unittest.mm b/ui/accessibility/platform/browser_accessibility_mac_unittest.mm
index c11015c..2cd7ef28 100644
--- a/ui/accessibility/platform/browser_accessibility_mac_unittest.mm
+++ b/ui/accessibility/platform/browser_accessibility_mac_unittest.mm
@@ -234,6 +234,46 @@
   EXPECT_NSEQ(nil, retainedFirstChild.accessibilityLabel);
 }
 
+// AppKit may retain a wrapper and key it in a hash-based collection, so
+// -hash must remain stable for the lifetime of the wrapper, and distinct
+// wrappers must never compare equal even if their backing nodes happen to
+// share the same per-tree id.
+TEST_F(BrowserAccessibilityMacTest, IdentityIsPerWrapperAndStable) {
+  // Build a second, independent tree whose root re-uses the same per-tree id
+  // as a node in the fixture's tree.
+  AXNodeData other_root;
+  other_root.id = 1000;
+  other_root.role = ax::mojom::Role::kRootWebArea;
+  TestAXNodeIdDelegate other_node_id_delegate;
+  std::unique_ptr<BrowserAccessibilityManager> other_manager =
+      std::make_unique<BrowserAccessibilityManagerMac>(
+          MakeAXTreeUpdateForTesting(other_root), other_node_id_delegate,
+          nullptr);
+  BrowserAccessibilityCocoa* other_wrapper =
+      base::apple::ObjCCastStrict<BrowserAccessibilityCocoa>(
+          other_manager->GetBrowserAccessibilityRoot()
+              ->GetNativeViewAccessible()
+              .Get());
+
+  ASSERT_NE(accessibility_, other_wrapper);
+  EXPECT_FALSE([accessibility_ isEqual:other_wrapper]);
+  EXPECT_FALSE([other_wrapper isEqual:accessibility_]);
+
+  // Hold the wrapper past detach, as the system might.
+  NS_VALID_UNTIL_END_OF_SCOPE BrowserAccessibilityCocoa* retained =
+      accessibility_;
+  const NSUInteger hash_before = retained.hash;
+  EXPECT_TRUE([retained isEqual:retained]);
+
+  // Tearing down the manager detaches the wrapper.
+  manager_.reset();
+  ASSERT_FALSE([retained instanceActive]);
+
+  EXPECT_EQ(hash_before, retained.hash);
+  EXPECT_TRUE([retained isEqual:retained]);
+  EXPECT_FALSE([retained isEqual:other_wrapper]);
+}
+
 TEST_F(BrowserAccessibilityMacTest, TestComputeTextEdit) {
   root_ = AXNodeData();
   root_.id = 1;