Remove unnecessary function SerializerClearedNode()

The SerializerClearedNode() is part of some spidery code paths, where the AXObjectCache is called back by the serializer it owns.

The extra complexity is not necessary, because:
- The item that was being cleared, cached_bounding_boxes_, can be cleared in AXObjectCacheImpl when a node is removed, along other map entries for the node. This CL also updates RemoveAXID() to correctly clear other items as well.
- The only reason to have this additional function is to avoid leaks when the serializer had to reset due to a failure. This almost never happens anymore, as we added DCHECKs for these failures and have cleaning up the majority of the causes.
- Additional work in CL:4027071 will provide stable IDs for a11y objects that can have their own bounds, and therefore keeping the cached bounding box actually makes sense even in the rare case that the tree is reset.

This alters now-invalid unit tests for the tree serializer that checked for leaks in situations that required tree resets.

Bug: None
Change-Id: I7300dceb1cc7e09adf38ff1620e8e64e9f262e10
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3826007
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: David Tseng <dtseng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1073689}

third_party/blink/public/web/web_ax_object.h

@@ -109,8 +109,6 @@ class BLINK_EXPORT WebAXObject {
   void Serialize(ui::AXNodeData* node_data,
                  ui::AXMode accessibility_mode) const;
 
-  void SerializerClearedNode(int node_id) const;
-
   void InvalidateSerializerSubtree() const;
   bool SerializeChanges(ui::AXTreeUpdate* update);
   bool IsInClientTree();

third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc

@@ -685,6 +685,7 @@ void AXObjectCacheImpl::Dispose() {
   }
 
   permission_observer_receiver_.reset();
+  cached_bounding_boxes_.clear();
 }
 
 void AXObjectCacheImpl::AddInspectorAgent(InspectorAccessibilityAgent* agent) {
@@ -1894,10 +1895,17 @@ void AXObjectCacheImpl::RemoveAXID(AXObject* object) {
   DCHECK(!HashTraits<AXID>::IsDeletedValue(obj_id));
   DCHECK(ids_in_use_.Contains(obj_id));
   object->SetAXObjectID(0);
+  // Clear AXIDs from maps. Note: do not need to erase id from
+  // changed_bounds_ids_, a set which is cleared each time
+  // SerializeLocationChanges() is finished. Also, do not need to eerae id from
+  // invalidated_ids_main_ or invalidated_ids_popup_, which are cleared each
+  // time ProcessInvalidatedObjects() finishes, and having extra ids in those
+  // sets is not harmful.
   ids_in_use_.erase(obj_id);
   autofill_state_map_.erase(obj_id);
   fixed_or_sticky_node_ids_.erase(obj_id);
-
+  cached_bounding_boxes_.erase(obj_id);
+  // Clear id from relation cache.
   relation_cache_->RemoveAXID(obj_id);
 }
 
@@ -3927,6 +3935,7 @@ void AXObjectCacheImpl::SerializeLocationChanges() {
   changes.reserve(changed_bounds_ids_.size());
   for (AXID changed_bounds_id : changed_bounds_ids_) {
     if (AXObject* obj = ObjectFromAXID(changed_bounds_id)) {
+      DCHECK(!obj->IsDetached());
       // Only update locations that are already known.
       auto bounds = cached_bounding_boxes_.find(changed_bounds_id);
       if (bounds == cached_bounding_boxes_.end())
@@ -4407,10 +4416,6 @@ void AXObjectCacheImpl::SetCachedBoundingBox(
   cached_bounding_boxes_.Set(id, bounds);
 }
 
-void AXObjectCacheImpl::SerializerClearedNode(AXID id) {
-  cached_bounding_boxes_.erase(id);
-}
-
 void AXObjectCacheImpl::HandleScrollPositionChanged(
     LocalFrameView* frame_view) {
   SCOPED_DISALLOW_LIFECYCLE_TRANSITION();

third_party/blink/renderer/modules/accessibility/blink_ax_tree_source.cc

@@ -370,10 +370,6 @@ std::string BlinkAXTreeSource::GetDebugString(AXObject* node) const {
   return node->ToString(true).Utf8();
 }
 
-void BlinkAXTreeSource::SerializerClearedNode(int32_t node_id) {
-  ax_object_cache_->SerializerClearedNode(node_id);
-}
-
 void BlinkAXTreeSource::SerializeNode(AXObject* src,
                                       ui::AXNodeData* dst) const {
 #if DCHECK_IS_ON()

third_party/blink/renderer/modules/accessibility/blink_ax_tree_source.h

@@ -52,7 +52,6 @@ class MODULES_EXPORT BlinkAXTreeSource
   bool IsEqual(AXObject* node1, AXObject* node2) const override;
   AXObject* GetNull() const override;
   std::string GetDebugString(AXObject* node) const override;
-  void SerializerClearedNode(int32_t node_id) override;
 
   // Set the id of the node to fetch image data for. Normally the content
   // of images is not part of the accessibility tree, but one node at a

third_party/blink/renderer/modules/exported/web_ax_object.cc

@@ -208,10 +208,6 @@ void WebAXObject::Serialize(ui::AXNodeData* node_data,
   private_->Serialize(node_data, accessibility_mode);
 }
 
-void WebAXObject::SerializerClearedNode(int node_id) const {
-  private_->AXObjectCache().SerializerClearedNode(node_id);
-}
-
 void WebAXObject::InvalidateSerializerSubtree() const {
   if (IsDetached())
     return;

ui/accessibility/ax_tree_serializer.h

@@ -260,7 +260,9 @@ template <typename AXSourceNode>
 AXTreeSerializer<AXSourceNode>::~AXTreeSerializer() {
   // Clear |tree_| to prevent any additional calls to the tree source
   // during teardown.
+  // TODO(accessibility) How would that happen?
   tree_ = nullptr;
+  // Free up any resources allocated on the heap that are stored with raw_ptr.
   Reset();
 }
 
@@ -278,11 +280,8 @@ void AXTreeSerializer<AXSourceNode>::InternalReset() {
   // but Reset() needs to work even if the tree is in a broken state.
   // Instead, iterate over |client_id_map_| to ensure we clear all nodes and
   // start from scratch.
-  for (auto&& item : client_id_map_) {
-    if (tree_)
-      tree_->SerializerClearedNode(item.first);
+  for (auto&& item : client_id_map_)
     delete item.second;
-  }
   client_id_map_.clear();
   client_root_ = nullptr;
 }
@@ -562,7 +561,6 @@ void AXTreeSerializer<AXSourceNode>::DeleteClientSubtree(
 #endif
   } else {
     DeleteDescendants(client_node);
-    tree_->SerializerClearedNode(client_node->id);
     client_id_map_.erase(client_node->id);
     delete client_node;
   }

ui/accessibility/ax_tree_serializer_unittest.cc

@@ -466,159 +466,6 @@ TEST_F(AXTreeSerializerTest, ResetWorksWithNewRootId) {
   EXPECT_TRUE(dst_tree.Unserialize(update)) << dst_tree.error();
 }
 
-// Wraps an AXTreeSource and provides access to the results of the
-// SerializerClearedNode callback.
-class AXTreeSourceTestWrapper : public AXTreeSource<const AXNode*> {
- public:
-  explicit AXTreeSourceTestWrapper(AXTreeSource<const AXNode*>* tree_source)
-      : tree_source_(tree_source) {}
-  ~AXTreeSourceTestWrapper() override = default;
-
-  // Override SerializerClearedNode and provide a way to access it.
-  void SerializerClearedNode(AXNodeID node_id) override {
-    cleared_node_ids_.insert(node_id);
-  }
-
-  void ClearClearedNodeIds() { cleared_node_ids_.clear(); }
-  std::set<AXNodeID>& cleared_node_ids() { return cleared_node_ids_; }
-
-  // The rest of the AXTreeSource implementation just calls through to
-  // tree_source_.
-  bool GetTreeData(AXTreeData* data) const override {
-    return tree_source_->GetTreeData(data);
-  }
-  const AXNode* GetRoot() const override { return tree_source_->GetRoot(); }
-  const AXNode* GetFromId(AXNodeID id) const override {
-    return tree_source_->GetFromId(id);
-  }
-  AXNodeID GetId(const AXNode* node) const override {
-    return tree_source_->GetId(node);
-  }
-  void GetChildren(const AXNode* node,
-                   std::vector<const AXNode*>* out_children) const override {
-    return tree_source_->GetChildren(node, out_children);
-  }
-  const AXNode* GetParent(const AXNode* node) const override {
-    return tree_source_->GetParent(node);
-  }
-  bool IsIgnored(const AXNode* node) const override {
-    return tree_source_->IsIgnored(node);
-  }
-  bool IsValid(const AXNode* node) const override {
-    return tree_source_->IsValid(node);
-  }
-  bool IsEqual(const AXNode* node1, const AXNode* node2) const override {
-    return tree_source_->IsEqual(node1, node2);
-  }
-  const AXNode* GetNull() const override { return tree_source_->GetNull(); }
-  void SerializeNode(const AXNode* node, AXNodeData* out_data) const override {
-    tree_source_->SerializeNode(node, out_data);
-  }
-
- private:
-  raw_ptr<AXTreeSource<const AXNode*>> tree_source_;
-  std::set<AXNodeID> cleared_node_ids_;
-};
-
-TEST_F(AXTreeSerializerTest, TestClearedNodesWhenUpdatingRoot) {
-  // (1 (2 (3 (4))))
-  treedata0_.root_id = 1;
-  treedata0_.nodes.resize(4);
-  treedata0_.nodes[0].id = 1;
-  treedata0_.nodes[0].child_ids.push_back(2);
-  treedata0_.nodes[1].id = 2;
-  treedata0_.nodes[1].child_ids.push_back(3);
-  treedata0_.nodes[2].id = 3;
-  treedata0_.nodes[2].child_ids.push_back(4);
-  treedata0_.nodes[3].id = 4;
-
-  // (5 (2 (3 (4))))
-  treedata1_.root_id = 5;
-  treedata1_.nodes.resize(4);
-  treedata1_.nodes[0].id = 5;
-  treedata1_.nodes[0].child_ids.push_back(2);
-  treedata1_.nodes[1].id = 2;
-  treedata1_.nodes[1].child_ids.push_back(3);
-  treedata1_.nodes[2].id = 3;
-  treedata1_.nodes[2].child_ids.push_back(4);
-  treedata1_.nodes[3].id = 4;
-
-  // Similar sequence to CreateTreeSerializer, but using
-  // AXTreeSourceTestWrapper instead.
-  tree0_ = std::make_unique<AXSerializableTree>(treedata0_);
-  tree1_ = std::make_unique<AXSerializableTree>(treedata1_);
-  tree0_source_.reset(tree0_->CreateTreeSource());
-  AXTreeSourceTestWrapper tree0_source_wrapper(tree0_source_.get());
-  serializer_ = std::make_unique<BasicAXTreeSerializer>(&tree0_source_wrapper);
-  AXTreeUpdate unused_update;
-  ASSERT_TRUE(serializer_->SerializeChanges(tree0_->root(), &unused_update));
-  tree1_source_.reset(tree1_->CreateTreeSource());
-  AXTreeSourceTestWrapper tree1_source_wrapper(tree1_source_.get());
-  serializer_->ChangeTreeSourceForTesting(&tree1_source_wrapper);
-  ASSERT_EQ(4U, serializer_->ClientTreeNodeCount());
-
-  // If we swap out the root, all of the node IDs should have
-  // SerializerClearedNode called on them.
-  tree1_source_wrapper.ClearClearedNodeIds();
-  ASSERT_TRUE(serializer_->SerializeChanges(tree1_->root(), &unused_update));
-  EXPECT_THAT(tree1_source_wrapper.cleared_node_ids(),
-              UnorderedElementsAre(1, 2, 3, 4));
-
-  // Destroy the serializer first so that the AXTreeSources it points to
-  // don't go out of scope first.
-  serializer_.reset();
-}
-
-TEST_F(AXTreeSerializerTest, TestClearedNodesWhenUpdatingBranch) {
-  // (1 (2 (3 (4))))
-  treedata0_.root_id = 1;
-  treedata0_.nodes.resize(4);
-  treedata0_.nodes[0].id = 1;
-  treedata0_.nodes[0].child_ids.push_back(2);
-  treedata0_.nodes[1].id = 2;
-  treedata0_.nodes[1].child_ids.push_back(3);
-  treedata0_.nodes[2].id = 3;
-  treedata0_.nodes[2].child_ids.push_back(4);
-  treedata0_.nodes[3].id = 4;
-
-  // (1 (2 (5 (6))))
-  treedata1_.root_id = 1;
-  treedata1_.nodes.resize(4);
-  treedata1_.nodes[0].id = 1;
-  treedata1_.nodes[0].child_ids.push_back(2);
-  treedata1_.nodes[1].id = 2;
-  treedata1_.nodes[1].child_ids.push_back(5);
-  treedata1_.nodes[2].id = 5;
-  treedata1_.nodes[2].child_ids.push_back(6);
-  treedata1_.nodes[3].id = 6;
-
-  // Similar sequence to CreateTreeSerializer, but using
-  // AXTreeSourceTestWrapper instead.
-  tree0_ = std::make_unique<AXSerializableTree>(treedata0_);
-  tree1_ = std::make_unique<AXSerializableTree>(treedata1_);
-  tree0_source_.reset(tree0_->CreateTreeSource());
-  AXTreeSourceTestWrapper tree0_source_wrapper(tree0_source_.get());
-  serializer_ = std::make_unique<BasicAXTreeSerializer>(&tree0_source_wrapper);
-  AXTreeUpdate unused_update;
-  ASSERT_TRUE(serializer_->SerializeChanges(tree0_->root(), &unused_update));
-  tree1_source_.reset(tree1_->CreateTreeSource());
-  AXTreeSourceTestWrapper tree1_source_wrapper(tree1_source_.get());
-  serializer_->ChangeTreeSourceForTesting(&tree1_source_wrapper);
-  ASSERT_EQ(4U, serializer_->ClientTreeNodeCount());
-
-  // If we replace one branch with another, we should get calls to
-  // SerializerClearedNode with all of the node IDs no longer in the tree.
-  tree1_source_wrapper.ClearClearedNodeIds();
-  ASSERT_TRUE(
-      serializer_->SerializeChanges(tree1_->GetFromId(2), &unused_update));
-  EXPECT_THAT(tree1_source_wrapper.cleared_node_ids(),
-              UnorderedElementsAre(3, 4));
-
-  // Destroy the serializer first so that the AXTreeSources it points to
-  // don't go out of scope first.
-  serializer_.reset();
-}
-
 TEST_F(AXTreeSerializerTest, TestPartialSerialization) {
   // Serialize only part of the tree.
 

ui/accessibility/ax_tree_source.h

@@ -77,12 +77,6 @@ class AXTreeSource {
     return node_data.ToString();
   }
 
-  // This is called by AXTreeSerializer when it serializes a tree and
-  // discovers that a node previously in the tree is no longer part of
-  // the tree. It can be used to allow an AXTreeSource to keep a cache
-  // indexed by node ID and delete nodes when they're no longer needed.
-  virtual void SerializerClearedNode(AXNodeID node_id) {}
-
   // The following methods should be overridden in order to add or remove an
   // `AXTreeSourceObserver`, which is notified when nodes are added, removed or
   // updated in this tree source.