Try to avoid observing ABM that may be destroyed.

Sometimes WebAppMetrics is still holding a foreground_web_contents on
shutdown, and thus fails to deregister itself as an observer of the
associated AppBannerManager, which is already gone.
This change adds extra checks to try to prevent getting into this state,
and dumps crash reports if in an unexpected state.

Bug: 1162123
Change-Id: I142e6cc30395decf7a09ca2c6773de8c65865535
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2620919
Commit-Queue: Glen Robertson <glenrob@chromium.org>
Reviewed-by: Matthew Denton <mpdenton@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843424}

chrome/browser/ui/web_applications/web_app_metrics.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/web_applications/web_app_metrics.h"
 
 #include "base/bind.h"
+#include "base/debug/dump_without_crashing.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/optional.h"
 #include "base/power_monitor/power_monitor.h"
@@ -94,6 +95,10 @@ WebAppMetrics::WebAppMetrics(Profile* profile)
 }
 
 WebAppMetrics::~WebAppMetrics() {
+  // Foreground web contents should already be nullptr by this point, or we have
+  // somehow missed being notified of its destruction.
+  if (foreground_web_contents_ != nullptr)
+    base::debug::DumpWithoutCrashing();
   UpdateForegroundWebContents(nullptr);
   BrowserList::RemoveObserver(this);
   base::PowerMonitor::RemoveObserver(this);
@@ -188,6 +193,13 @@ void WebAppMetrics::OnTabStripModelChanged(
 
   UpdateForegroundWebContents(selection.new_contents);
 
+  // Contents being replaced should never be the new selection.
+  if (change.type() == TabStripModelChange::kReplaced &&
+      change.GetReplace()->old_contents == foreground_web_contents_) {
+    base::debug::DumpWithoutCrashing();
+    UpdateForegroundWebContents(nullptr);
+  }
+
   if (change.type() == TabStripModelChange::kRemoved) {
     for (const TabStripModelChange::ContentsWithIndexAndWillBeDeleted&
              contents : change.GetRemove()->contents) {
@@ -196,13 +208,16 @@ void WebAppMetrics::OnTabStripModelChanged(
             WebAppTabHelperBase::FromWebContents(contents.contents);
         if (tab_helper && !tab_helper->GetAppId().empty())
           app_last_interacted_time_.erase(tab_helper->GetAppId());
-        // Foreground contents should not be going away.
-        DCHECK_NE(contents.contents, foreground_web_contents_);
+        // Newly-selected foreground contents should not be going away.
+        if (contents.contents == foreground_web_contents_) {
+          base::debug::DumpWithoutCrashing();
+          UpdateForegroundWebContents(nullptr);
+        }
       }
     }
   }
 
-  UpdateUkmData(selection.new_contents, TabSwitching::kTo);
+  UpdateUkmData(foreground_web_contents_, TabSwitching::kTo);
 }
 
 void WebAppMetrics::OnSuspend() {
@@ -263,6 +278,13 @@ void WebAppMetrics::OnInstallableWebAppStatusUpdated() {
   UpdateUkmData(foreground_web_contents_, TabSwitching::kTo);
 }
 
+void WebAppMetrics::WebContentsDestroyed() {
+  // TODO (crbug.com/1162123): Remove this method in M91 or later.
+  // Should have stopped observing this WebContents before this point.
+  base::debug::DumpWithoutCrashing();
+  UpdateForegroundWebContents(nullptr);
+}
+
 void WebAppMetrics::RemoveBrowserListObserverForTesting() {
   BrowserList::RemoveObserver(this);
 }
@@ -359,6 +381,7 @@ void WebAppMetrics::UpdateForegroundWebContents(WebContents* web_contents) {
     return;
   app_banner_manager_observer_.RemoveAll();
   foreground_web_contents_ = web_contents;
+  WebContentsObserver::Observe(web_contents);
   if (web_contents) {
     auto* app_banner_manager =
         webapps::AppBannerManager::FromWebContents(web_contents);

chrome/browser/ui/web_applications/web_app_metrics.h

@@ -14,6 +14,7 @@
 #include "chrome/browser/web_applications/components/web_app_id.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/site_engagement/content/site_engagement_observer.h"
+#include "content/public/browser/web_contents_observer.h"
 
 class Profile;
 class Browser;
@@ -32,6 +33,7 @@ class WebAppMetrics : public KeyedService,
                       public BrowserListObserver,
                       public TabStripModelObserver,
                       public webapps::AppBannerManager::Observer,
+                      public content::WebContentsObserver,
                       public base::PowerObserver {
  public:
   static WebAppMetrics* Get(Profile* profile);
@@ -69,6 +71,9 @@ class WebAppMetrics : public KeyedService,
   // webapps::AppBannerManager::Observer:
   void OnInstallableWebAppStatusUpdated() override;
 
+  // content::WebContentsObserver:
+  void WebContentsDestroyed() override;
+
   // Browser activation causes flaky tests. Call observer methods directly.
   void RemoveBrowserListObserverForTesting();
   void CountUserInstalledAppsForTesting();