Make LoadMonitoringExtensionHostQueue remove itself as an ExtensionHost observer at the correct tim…

extensions/browser/extension_host.cc

Index: extensions/browser/extension_host.cc
diff --git a/extensions/browser/extension_host.cc b/extensions/browser/extension_host.cc
index bbf51ef4913b41b8d784b07c8eae28e7ed6eebad..beeddf1ba811b19b61b7486f74edd991b2c77e78 100644
--- a/extensions/browser/extension_host.cc
+++ b/extensions/browser/extension_host.cc
@@ -101,11 +101,21 @@ ExtensionHost::~ExtensionHost() {
   FOR_EACH_OBSERVER(DeferredStartRenderHostObserver,
                     deferred_start_render_host_observer_list_,
                     OnDeferredStartRenderHostDestroyed(this));
+
+  // Call DidStopLoading if we haven't already, because we're about to prevent
+  // it being called.
+  if (!did_stop_loading_)
+    DidStopLoading(render_view_host());
+
+  // Remove ourselves from the queue as late as possible (before effectively
+  // destroying self, but after everything else) so that queues that are
+  // monitoring lifetime get a chance to see stop-loading events.
   delegate_->GetExtensionHostQueue()->Remove(this);
-  // Immediately stop observing |host_contents_| because its destruction events
-  // (like DidStopLoading, it turns out) can call back into ExtensionHost
-  // re-entrantly, when anything declared after |host_contents_| has already
-  // been destroyed.
+
+  // Deliberately stop observing |host_contents_| because its destruction
+  // events (like DidStopLoading, it turns out) can call back into
+  // ExtensionHost re-entrantly, when anything declared after |host_contents_|
+  // has already been destroyed.
   content::WebContentsObserver::Observe(nullptr);
 }
 
@@ -268,31 +278,31 @@ void ExtensionHost::DidStartLoading(content::RenderViewHost* render_view_host) {
 }
 
 void ExtensionHost::DidStopLoading(content::RenderViewHost* render_view_host) {
-  bool notify = !did_stop_loading_;
+  CHECK(!did_stop_loading_);

[Yoyo Zhou, 2015/03/10 22:48:13]

I think this is wrong. Say the background page navigates to another page - don't
we get the stop loading event again?

[not at google - send to devlin, 2015/03/12 22:46:51]

Yeap, hopefully that explains the epic set of crashes in the tests. I guess we
should be tracking this across DidStartLoading-->DidStopLoading events, not
construction-->DidStopLoading.

   did_stop_loading_ = true;
+
   OnDidStopLoading();
-  if (notify) {
-    CHECK(load_start_.get());
-    if (extension_host_type_ == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE) {
-      if (extension_ && BackgroundInfo::HasLazyBackgroundPage(extension_)) {
-        UMA_HISTOGRAM_MEDIUM_TIMES("Extensions.EventPageLoadTime2",
-                                   load_start_->Elapsed());
-      } else {
-        UMA_HISTOGRAM_MEDIUM_TIMES("Extensions.BackgroundPageLoadTime2",
-                                   load_start_->Elapsed());
-      }
-    } else if (extension_host_type_ == VIEW_TYPE_EXTENSION_POPUP) {
-      UMA_HISTOGRAM_MEDIUM_TIMES("Extensions.PopupLoadTime2",
+  CHECK(load_start_.get());
+  if (extension_host_type_ == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE) {
+    if (extension_ && BackgroundInfo::HasLazyBackgroundPage(extension_)) {
+      UMA_HISTOGRAM_MEDIUM_TIMES("Extensions.EventPageLoadTime2",
+                                 load_start_->Elapsed());
+    } else {
+      UMA_HISTOGRAM_MEDIUM_TIMES("Extensions.BackgroundPageLoadTime2",
                                  load_start_->Elapsed());
     }
-    content::NotificationService::current()->Notify(
-        extensions::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING,
-        content::Source<BrowserContext>(browser_context_),
-        content::Details<ExtensionHost>(this));
-    FOR_EACH_OBSERVER(DeferredStartRenderHostObserver,
-                      deferred_start_render_host_observer_list_,
-                      OnDeferredStartRenderHostDidStopLoading(this));
+  } else if (extension_host_type_ == VIEW_TYPE_EXTENSION_POPUP) {
+    UMA_HISTOGRAM_MEDIUM_TIMES("Extensions.PopupLoadTime2",
+                               load_start_->Elapsed());
   }
+
+  content::NotificationService::current()->Notify(
+      extensions::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING,
+      content::Source<BrowserContext>(browser_context_),
+      content::Details<ExtensionHost>(this));
+  FOR_EACH_OBSERVER(DeferredStartRenderHostObserver,
+                    deferred_start_render_host_observer_list_,
+                    OnDeferredStartRenderHostDidStopLoading(this));
 }
 
 void ExtensionHost::OnDidStopLoading() {