VizDevtools: Fix bugs in OnDestroyedCompositorFrameSink

Update the expected behavior for OnDestroyedCompositorFrameSink
and OnInvalidatedFrameSinkId:
OnDestroyedCompositorFrameSink(element): Set the |element| to not
connected, move all its children to |element_root| and
remove |element| from its parents.
OnInvalidatedFrameSinkId(element): free the memory of |element|
and follow the same steps as OnDestroyedCompositorFrameSink.

TEST=Manual, doesn't crash after connected.

Change-Id: I06eb18fe14a017d50fc8209cc20251da9ed7f0a5
Reviewed-on: https://chromium-review.googlesource.com/c/1294411Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Yi Xu <yiyix@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602420}

components/ui_devtools/viz_views/dom_agent_viz.cc

@@ -79,13 +79,15 @@ void DOMAgentViz::OnInvalidatedFrameSinkId(
   auto it = frame_sink_elements_.find(frame_sink_id);
   DCHECK(it != frame_sink_elements_.end());
 
-  FrameSinkElement* element = it->second;
+  // Destroy the FrameSinkElement |element| after updating the frame-tree.
+  std::unique_ptr<FrameSinkElement> element(it->second);
   element->SetRegistered(false);
 
-  // If a FrameSink is destroyed and invalidated we should remove it from the
-  // tree.
-  RemoveFrameSinkSubtree(element);
-  DestroyChildSubtree(element->parent(), element);
+  // A FrameSinkElement with |frame_sink_id| can only be invalidated after
+  // being destroyed.
+  DCHECK(!element->is_client_connected());
+  RemoveFrameSinkSubtree(element.get());
+  frame_sink_elements_.erase(frame_sink_id);
 }
 
 void DOMAgentViz::OnCreatedCompositorFrameSink(
@@ -105,12 +107,8 @@ void DOMAgentViz::OnDestroyedCompositorFrameSink(
   DCHECK(it != frame_sink_elements_.end());
 
   FrameSinkElement* element = it->second;
+  // Set FrameSinkElement to not connected to make it as destroyed.
   element->SetClientConnected(false);
-
-  // If a FrameSink is invalidated and destroyed we should remove it from the
-  // tree.
-  RemoveFrameSinkSubtree(element);
-  DestroyChildSubtree(element->parent(), element);
 }
 
 void DOMAgentViz::OnRegisteredFrameSinkHierarchy(
@@ -259,14 +257,6 @@ std::unique_ptr<DOM::Node> DOMAgentViz::BuildTreeForUIElement(
   return nullptr;
 }
 
-void DOMAgentViz::DestroyChildSubtree(UIElement* parent, UIElement* child) {
-  std::vector<UIElement*> direct_children = child->children();
-  for (auto* kid : direct_children)
-    DestroyChildSubtree(child, kid);
-  parent->RemoveChild(child);
-  std::unique_ptr<UIElement> to_destroy(child);
-}
-
 void DOMAgentViz::Clear() {
   attached_frame_sinks_.clear();
   frame_sink_elements_.clear();
@@ -301,20 +291,11 @@ void DOMAgentViz::RemoveFrameSinkSubtree(UIElement* root) {
   // detach all its children and attach them to RootElement and then delete the
   // node we were asked for.
   std::vector<viz::FrameSinkId> children;
-  frame_sink_elements_.erase(FrameSinkElement::From(root));
-  for (auto* child : root->children()) {
-    RemoveFrameSinkElement(child);
+  for (auto* child : root->children())
     child->set_parent(element_root());
-  }
 
   if (root->parent())
     root->parent()->RemoveChild(root);
 }
 
-void DOMAgentViz::RemoveFrameSinkElement(UIElement* element) {
-  frame_sink_elements_.erase(FrameSinkElement::From(element));
-  for (auto* child : element->children())
-    RemoveFrameSinkElement(child);
-}
-
 }  // namespace ui_devtools

components/ui_devtools/viz_views/dom_agent_viz.h

@@ -52,10 +52,6 @@ class DOMAgentViz : public viz::FrameSinkObserver, public DOMAgent {
   std::unique_ptr<protocol::DOM::Node> BuildTreeForUIElement(
       UIElement* ui_element) override;
 
-  // Removes the |child| subtree from the |parent| subtree and destroys every
-  // element in the |child| subtree.
-  void DestroyChildSubtree(UIElement* parent, UIElement* child);
-
   // Every time the frontend disconnects we don't destroy DOMAgent so once we
   // establish the connection again we need to clear the FrameSinkId sets
   // because they may carry obsolete data. Then we initialize these with alive
@@ -70,14 +66,10 @@ class DOMAgentViz : public viz::FrameSinkObserver, public DOMAgent {
   // Mark a FrameSink that has |frame_sink_id| and all its subtree as attached.
   void SetAttachedFrameSink(const viz::FrameSinkId& frame_sink_id);
 
-  // We delete the FrameSinkElements in the subtree rooted at |root| from
-  // |frame_sink_elements_| and attach all its children to the root_element().
+  // We remove |root| from its parents and attach all its children to the
+  // root_element().
   void RemoveFrameSinkSubtree(UIElement* root);
 
-  // Remove FrameSinkElements for subtree rooted at |element| from the tree
-  // |frame_sink_elements_|.
-  void RemoveFrameSinkElement(UIElement* element);
-
   // This is used to track created FrameSinkElements in a FrameSink tree. Every
   // time we register/invalidate a FrameSinkId, create/destroy a FrameSink,
   // register/unregister hierarchy we change this set, because these actions