Track all changed nodes during an update

While deserializing, make a note of all changed nodes via id before performing any tree changes/removals.

While removing (non-atomically), reference the changed list to ensure we notify delegates of nodes that will actually be deleted and not just deleted then re-created as part of bookkeeping to enforce the "non-reparenting" invariant inside of AXTree::CreateChildVector.

BUG=642799
TEST=for each of the reproductions in the bug, ensure the automation api (as a client) does not destroy then re-create js objects; rather, ensure it re-uses them for reparented trees.

Review-Url: https://codereview.chromium.org/2080573003
Cr-Commit-Position: refs/heads/master@{#417207}

chrome/renderer/extensions/automation_internal_custom_bindings.cc

@@ -1170,6 +1170,18 @@ void AutomationInternalCustomBindings::OnSubtreeWillBeDeleted(
   // be needed for something.
 }
 
+void AutomationInternalCustomBindings::OnNodeWillBeReparented(
+    ui::AXTree* tree,
+    ui::AXNode* node) {
+  // Don't do anything here since the node will soon go away and be re-created.
+}
+
+void AutomationInternalCustomBindings::OnSubtreeWillBeReparented(
+    ui::AXTree* tree,
+    ui::AXNode* node) {
+  // Don't do anything here since the node will soon go away and be re-created.
+}
+
 void AutomationInternalCustomBindings::OnNodeCreated(ui::AXTree* tree,
                                                      ui::AXNode* node) {
   // Not needed, this is called in the middle of an update so it's not
@@ -1177,6 +1189,13 @@ void AutomationInternalCustomBindings::OnNodeCreated(ui::AXTree* tree,
   // OnAtomicUpdateFinished instead.
 }
 
+void AutomationInternalCustomBindings::OnNodeReparented(ui::AXTree* tree,
+                                                        ui::AXNode* node) {
+  // Not needed, this is called in the middle of an update so it's not
+  // safe to trigger JS from here. Wait for the notification in
+  // OnAtomicUpdateFinished instead.
+}
+
 void AutomationInternalCustomBindings::OnNodeChanged(ui::AXTree* tree,
                                                      ui::AXNode* node) {
   // Not needed, this is called in the middle of an update so it's not

chrome/renderer/extensions/automation_internal_custom_bindings.h

@@ -159,7 +159,10 @@ class AutomationInternalCustomBindings : public ObjectBackedNativeHandler,
   void OnTreeDataChanged(ui::AXTree* tree) override;
   void OnNodeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
   void OnSubtreeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
+  void OnNodeWillBeReparented(ui::AXTree* tree, ui::AXNode* node) override;
+  void OnSubtreeWillBeReparented(ui::AXTree* tree, ui::AXNode* node) override;
   void OnNodeCreated(ui::AXTree* tree, ui::AXNode* node) override;
+  void OnNodeReparented(ui::AXTree* tree, ui::AXNode* node) override;
   void OnNodeChanged(ui::AXTree* tree, ui::AXNode* node) override;
   void OnAtomicUpdateFinished(ui::AXTree* tree,
                               bool root_changed,

content/browser/accessibility/browser_accessibility_manager.cc

@@ -976,6 +976,24 @@ void BrowserAccessibilityManager::OnSubtreeWillBeDeleted(ui::AXTree* tree,
     obj->OnSubtreeWillBeDeleted();
 }
 
+void BrowserAccessibilityManager::OnNodeWillBeReparented(ui::AXTree* tree,
+                                                         ui::AXNode* node) {
+  // BrowserAccessibility should probably ask the tree source for the AXNode via
+  // an id rather than weakly holding a pointer to a AXNode that might have been
+  // destroyed under the hood and re-created later on. Treat this as a delete to
+  // make things work.
+  OnNodeWillBeDeleted(tree, node);
+}
+
+void BrowserAccessibilityManager::OnSubtreeWillBeReparented(ui::AXTree* tree,
+                                                            ui::AXNode* node) {
+  // BrowserAccessibility should probably ask the tree source for the AXNode via
+  // an id rather than weakly holding a pointer to a AXNode that might have been
+  // destroyed under the hood and re-created later on. Treat this as a delete to
+  // make things work.
+  OnSubtreeWillBeDeleted(tree, node);
+}
+
 void BrowserAccessibilityManager::OnNodeCreated(ui::AXTree* tree,
                                                 ui::AXNode* node) {
   BrowserAccessibility* wrapper = factory_->Create();
@@ -984,6 +1002,15 @@ void BrowserAccessibilityManager::OnNodeCreated(ui::AXTree* tree,
   wrapper->OnDataChanged();
 }
 
+void BrowserAccessibilityManager::OnNodeReparented(ui::AXTree* tree,
+                                                   ui::AXNode* node) {
+  // BrowserAccessibility should probably ask the tree source for the AXNode via
+  // an id rather than weakly holding a pointer to a AXNode that might have been
+  // destroyed under the hood and re-created later on. Treat this as a create to
+  // make things work.
+  OnNodeCreated(tree, node);
+}
+
 void BrowserAccessibilityManager::OnNodeChanged(ui::AXTree* tree,
                                                 ui::AXNode* node) {
   DCHECK(node);

content/browser/accessibility/browser_accessibility_manager.h

@@ -354,7 +354,10 @@ class CONTENT_EXPORT BrowserAccessibilityManager : public ui::AXTreeDelegate {
   void OnTreeDataChanged(ui::AXTree* tree) override;
   void OnNodeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
   void OnSubtreeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
+  void OnNodeWillBeReparented(ui::AXTree* tree, ui::AXNode* node) override;
+  void OnSubtreeWillBeReparented(ui::AXTree* tree, ui::AXNode* node) override;
   void OnNodeCreated(ui::AXTree* tree, ui::AXNode* node) override;
+  void OnNodeReparented(ui::AXTree* tree, ui::AXNode* node) override;
   void OnNodeChanged(ui::AXTree* tree, ui::AXNode* node) override;
   void OnAtomicUpdateFinished(
       ui::AXTree* tree,

ui/accessibility/ax_tree.cc

@@ -29,6 +29,15 @@ std::string TreeToStringHelper(AXNode* node, int indent) {
 // Intermediate state to keep track of during a tree update.
 struct AXTreeUpdateState {
   AXTreeUpdateState() : new_root(nullptr) {}
+  // Returns whether this update changes |node|.
+  bool HasChangedNode(const AXNode* node) {
+    return changed_node_ids.find(node->id()) != changed_node_ids.end();
+  }
+
+  // Returns whether this update removes |node|.
+  bool HasRemovedNode(const AXNode* node) {
+    return removed_node_ids.find(node->id()) != removed_node_ids.end();
+  }
 
   // During an update, this keeps track of all nodes that have been
   // implicitly referenced as part of this update, but haven't been
@@ -36,6 +45,12 @@ struct AXTreeUpdateState {
   // end of Unserialize.
   std::set<AXNode*> pending_nodes;
 
+  // This is similar to above, but we store node ids here because this list gets
+  // generated before any nodes get created or re-used. Its purpose is to allow
+  // us to know what nodes will be updated so we can make more intelligent
+  // decisions about when to notify delegates of removals or reparenting.
+  std::set<int> changed_node_ids;
+
   // Keeps track of new nodes created during this update.
   std::set<AXNode*> new_nodes;
 
@@ -92,6 +107,10 @@ bool AXTree::Unserialize(const AXTreeUpdate& update) {
   AXTreeUpdateState update_state;
   int32_t old_root_id = root_ ? root_->id() : 0;
 
+  // First, make a note of any nodes we will touch as part of this update.
+  for (size_t i = 0; i < update.nodes.size(); ++i)
+    update_state.changed_node_ids.insert(update.nodes[i].id);
+
   if (update.has_tree_data)
     UpdateData(update.tree_data);
 
@@ -146,9 +165,7 @@ bool AXTree::Unserialize(const AXTreeUpdate& update) {
       AXNode* node = GetFromId(update.nodes[i].id);
       bool is_new_node = new_nodes.find(node) != new_nodes.end();
       bool is_reparented_node =
-          is_new_node &&
-          update_state.removed_node_ids.find(node->id()) !=
-              update_state.removed_node_ids.end();
+          is_new_node && update_state.HasRemovedNode(node);
 
       AXTreeDelegate::ChangeType change = AXTreeDelegate::NODE_CHANGED;
       if (is_new_node) {
@@ -176,11 +193,17 @@ std::string AXTree::ToString() const {
 
 AXNode* AXTree::CreateNode(AXNode* parent,
                            int32_t id,
-                           int32_t index_in_parent) {
+                           int32_t index_in_parent,
+                           AXTreeUpdateState* update_state) {
   AXNode* new_node = new AXNode(parent, id, index_in_parent);
   id_map_[new_node->id()] = new_node;
-  if (delegate_)
+  if (delegate_) {
+    if (update_state->HasChangedNode(new_node) &&
+        !update_state->HasRemovedNode(new_node))
       delegate_->OnNodeCreated(this, new_node);
+    else
+      delegate_->OnNodeReparented(this, new_node);
+  }
   return new_node;
 }
 
@@ -208,7 +231,7 @@ bool AXTree::UpdateNode(const AXNodeData& src,
       return false;
     }
 
-    update_state->new_root = CreateNode(NULL, src.id, 0);
+    update_state->new_root = CreateNode(NULL, src.id, 0, update_state);
     node = update_state->new_root;
     update_state->new_nodes.insert(node);
     node->SetData(src);
@@ -247,15 +270,24 @@ bool AXTree::UpdateNode(const AXNodeData& src,
 
 void AXTree::DestroySubtree(AXNode* node,
                             AXTreeUpdateState* update_state) {
-  if (delegate_)
+  DCHECK(update_state);
+  if (delegate_) {
+    if (!update_state->HasChangedNode(node))
       delegate_->OnSubtreeWillBeDeleted(this, node);
+    else
+      delegate_->OnSubtreeWillBeReparented(this, node);
+  }
   DestroyNodeAndSubtree(node, update_state);
 }
 
 void AXTree::DestroyNodeAndSubtree(AXNode* node,
                                    AXTreeUpdateState* update_state) {
-  if (delegate_)
+  if (delegate_) {
+    if (!update_state || !update_state->HasChangedNode(node))
       delegate_->OnNodeWillBeDeleted(this, node);
+    else
+      delegate_->OnNodeWillBeReparented(this, node);
+  }
   id_map_.erase(node->id());
   for (int i = 0; i < node->child_count(); ++i)
     DestroyNodeAndSubtree(node->ChildAtIndex(i), update_state);
@@ -316,7 +348,7 @@ bool AXTree::CreateNewChildVector(AXNode* node,
       }
       child->SetIndexInParent(index_in_parent);
     } else {
-      child = CreateNode(node, child_id, index_in_parent);
+      child = CreateNode(node, child_id, index_in_parent, update_state);
       update_state->pending_nodes.insert(child);
       update_state->new_nodes.insert(child);
     }

ui/accessibility/ax_tree.h

@@ -28,7 +28,10 @@ struct AXTreeUpdateState;
 // don't walk the parents and children at this time:
 //   OnNodeWillBeDeleted
 //   OnSubtreeWillBeDeleted
+//   OnNodeWillBeReparented
+//   OnSubtreeWillBeReparented
 //   OnNodeCreated
+//   OnNodeReparented
 //   OnNodeChanged
 //
 // In addition, one additional notification is fired at the end of an
@@ -59,10 +62,22 @@ class AX_EXPORT AXTreeDelegate {
   // invalid state!
   virtual void OnSubtreeWillBeDeleted(AXTree* tree, AXNode* node) = 0;
 
+  // Called just before a node is deleted for reparenting. See
+  // |OnNodeWillBeDeleted| for additional information.
+  virtual void OnNodeWillBeReparented(AXTree* tree, AXNode* node) = 0;
+
+  // Called just before a subtree is deleted for reparenting. See
+  // |OnSubtreeWillBeDeleted| for additional information.
+  virtual void OnSubtreeWillBeReparented(AXTree* tree, AXNode* node) = 0;
+
   // Called immediately after a new node is created. The tree may be in
   // the middle of an update, don't walk the parents and children now.
   virtual void OnNodeCreated(AXTree* tree, AXNode* node) = 0;
 
+  // Called immediately after a node is reparented. The tree may be in the
+  // middle of an update, don't walk the parents and children now.
+  virtual void OnNodeReparented(AXTree* tree, AXNode* node) = 0;
+
   // Called when a node changes its data or children. The tree may be in
   // the middle of an update, don't walk the parents and children now.
   virtual void OnNodeChanged(AXTree* tree, AXNode* node) = 0;
@@ -131,7 +146,10 @@ class AX_EXPORT AXTree {
   int size() { return static_cast<int>(id_map_.size()); }
 
  private:
-  AXNode* CreateNode(AXNode* parent, int32_t id, int32_t index_in_parent);
+  AXNode* CreateNode(AXNode* parent,
+                     int32_t id,
+                     int32_t index_in_parent,
+                     AXTreeUpdateState* update_state);
 
   // This is called from within Unserialize(), it returns true on success.
   bool UpdateNode(const AXNodeData& src,

ui/accessibility/ax_tree_unittest.cc

@@ -40,10 +40,16 @@ class FakeAXTreeDelegate : public AXTreeDelegate {
     subtree_deleted_ids_.push_back(node->id());
   }
 
+  void OnNodeWillBeReparented(AXTree* tree, AXNode* node) override {}
+
+  void OnSubtreeWillBeReparented(AXTree* tree, AXNode* node) override {}
+
   void OnNodeCreated(AXTree* tree, AXNode* node) override {
     created_ids_.push_back(node->id());
   }
 
+  void OnNodeReparented(AXTree* tree, AXNode* node) override {}
+
   void OnNodeChanged(AXTree* tree, AXNode* node) override {
     changed_ids_.push_back(node->id());
   }
@@ -432,4 +438,45 @@ TEST(AXTreeTest, ReparentingDoesNotTriggerNodeCreated) {
             node_reparented.end());
 }
 
+TEST(AXTreeTest, TreeDelegateIsNotCalledForReparenting) {
+  AXTreeUpdate initial_state;
+  initial_state.root_id = 1;
+  initial_state.nodes.resize(2);
+  initial_state.nodes[0].id = 1;
+  initial_state.nodes[0].child_ids.push_back(2);
+  initial_state.nodes[1].id = 2;
+
+  AXTree tree(initial_state);
+  AXTreeUpdate update;
+  update.node_id_to_clear = 1;
+  update.root_id = 2;
+  update.nodes.resize(2);
+  update.nodes[0].id = 2;
+  update.nodes[0].child_ids.push_back(4);
+  update.nodes[1].id = 4;
+
+  FakeAXTreeDelegate fake_delegate;
+  tree.SetDelegate(&fake_delegate);
+
+  EXPECT_TRUE(tree.Unserialize(update));
+
+  ASSERT_EQ(1U, fake_delegate.deleted_ids().size());
+  EXPECT_EQ(1, fake_delegate.deleted_ids()[0]);
+
+  ASSERT_EQ(1U, fake_delegate.subtree_deleted_ids().size());
+  EXPECT_EQ(1, fake_delegate.subtree_deleted_ids()[0]);
+
+  ASSERT_EQ(1U, fake_delegate.created_ids().size());
+  EXPECT_EQ(4, fake_delegate.created_ids()[0]);
+
+  ASSERT_EQ(0U, fake_delegate.subtree_creation_finished_ids().size());
+
+  ASSERT_EQ(1U, fake_delegate.node_creation_finished_ids().size());
+  EXPECT_EQ(4, fake_delegate.node_creation_finished_ids()[0]);
+
+  ASSERT_EQ(true, fake_delegate.root_changed());
+
+  tree.SetDelegate(NULL);
+}
+
 }  // namespace ui

ui/accessibility/platform/test_ax_node_wrapper.cc

@@ -31,7 +31,10 @@ class TestAXTreeDelegate : public AXTreeDelegate {
     }
   }
   void OnSubtreeWillBeDeleted(AXTree* tree, AXNode* node) override {}
+  void OnNodeWillBeReparented(AXTree* tree, AXNode* node) override {}
+  void OnSubtreeWillBeReparented(AXTree* tree, AXNode* node) override {}
   void OnNodeCreated(AXTree* tree, AXNode* node) override {}
+  void OnNodeReparented(AXTree* tree, AXNode* node) override {}
   void OnNodeChanged(AXTree* tree, AXNode* node) override {}
   void OnAtomicUpdateFinished(AXTree* tree,
                               bool root_changed,