PM: Teardown the TabHelper on WebContents destruction.

It would be a bit harder to separate construction from
initialization, because these instances are created through
WebContentsUserData<>::CreateForWebContents.

Bug: 966840
Change-Id: I18b48518769fb6c99237ae17e87683bd76d8758e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1808463Reviewed-by: Patrick Monette <pmonette@chromium.org>
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697257}

chrome/browser/performance_manager/performance_manager_tab_helper.cc

@@ -27,8 +27,16 @@ PerformanceManagerTabHelper* PerformanceManagerTabHelper::first_ = nullptr;
 
 // static
 void PerformanceManagerTabHelper::DetachAndDestroyAll() {
-  while (first_)
-    first_->web_contents()->RemoveUserData(UserDataKey());
+  while (first_) {
+    PerformanceManagerTabHelper* helper = first_;
+    // Tear it down and detach it from the WebContents, which will
+    // delete it.
+    content::WebContents* web_contents = helper->web_contents();
+    DCHECK(web_contents);
+    helper->TearDown();
+    DCHECK(!helper->web_contents());
+    web_contents->RemoveUserData(UserDataKey());
+  }
 }
 
 PerformanceManagerTabHelper::PerformanceManagerTabHelper(
@@ -72,6 +80,19 @@ PerformanceManagerTabHelper::PerformanceManagerTabHelper(
 }
 
 PerformanceManagerTabHelper::~PerformanceManagerTabHelper() {
+  DCHECK(!page_node_);
+  DCHECK(frames_.empty());
+  DCHECK_NE(this, first_);
+  DCHECK(!prev_);
+  DCHECK(!next_);
+}
+
+void PerformanceManagerTabHelper::TearDown() {
+  // Validate that this instance is in list of tab helpers.
+  DCHECK(first_ == this || next_ || prev_);
+  DCHECK_NE(this, next_);
+  DCHECK_NE(this, prev_);
+
   // Ship our page and frame nodes to the PerformanceManagerImpl for
   // incineration.
   std::vector<std::unique_ptr<NodeBase>> nodes;
@@ -84,17 +105,25 @@ PerformanceManagerTabHelper::~PerformanceManagerTabHelper() {
   // Delete the page and its entire frame tree from the graph.
   performance_manager_->BatchDeleteNodes(std::move(nodes));
 
-  if (first_ == this)
+  if (first_ == this) {
+    DCHECK(!prev_);
     first_ = next_;
+  }
 
   if (prev_) {
     DCHECK_EQ(prev_->next_, this);
     prev_->next_ = next_;
   }
+
   if (next_) {
     DCHECK_EQ(next_->prev_, this);
     next_->prev_ = prev_;
   }
+  prev_ = nullptr;
+  next_ = nullptr;
+
+  // Unsubscribe from the associated WebContents.
+  Observe(nullptr);
 }
 
 void PerformanceManagerTabHelper::RenderFrameCreated(
@@ -279,6 +308,10 @@ void PerformanceManagerTabHelper::TitleWasSet(content::NavigationEntry* entry) {
   PostToGraph(FROM_HERE, &PageNodeImpl::OnTitleUpdated, page_node_.get());
 }
 
+void PerformanceManagerTabHelper::WebContentsDestroyed() {
+  TearDown();
+}
+
 void PerformanceManagerTabHelper::DidUpdateFaviconURL(
     const std::vector<content::FaviconURL>& candidates) {
   // TODO(siggi): This logic belongs in the policy layer rather than here.

chrome/browser/performance_manager/performance_manager_tab_helper.h

@@ -52,6 +52,7 @@ class PerformanceManagerTabHelper
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
   void TitleWasSet(content::NavigationEntry* entry) override;
+  void WebContentsDestroyed() override;
   void DidUpdateFaviconURL(
       const std::vector<content::FaviconURL>& candidates) override;
   void OnInterfaceRequestFromFrame(
@@ -70,6 +71,7 @@ class PerformanceManagerTabHelper
   friend class WebContentsProxyImpl;
 
   explicit PerformanceManagerTabHelper(content::WebContents* web_contents);
+  void TearDown();
 
   // Post a task to run in the performance manager sequence. The |node| will be
   // passed as unretained, and the closure will be created with BindOnce.