Unobserve the host WebContents of an ExtensionHost on destruction, to prevent

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 04a067ab314242954b7939fe220dda2447d23baa..1cc9ad172a77eac8fdc8f9f63711382bf2468892 100644
--- a/extensions/browser/extension_host.cc
+++ b/extensions/browser/extension_host.cc
@@ -163,6 +163,11 @@ ExtensionHost::~ExtensionHost() {
   FOR_EACH_OBSERVER(ExtensionHostObserver, observer_list_,
                     OnExtensionHostDestroyed(this));
   ProcessCreationQueue::GetInstance()->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.
+  content::WebContentsObserver::Observe(NULL);

[Marijn Kruisselbrink, 2015/02/17 20:17:44]

nullptr? or was this intentional to stay consistent with the rest of the file?

[not at google - send to devlin, 2015/02/17 21:17:34]

It's never intentional :-) (and I fixed the rest of the file too, only a couple
of other places).

 }
 
 content::RenderProcessHost* ExtensionHost::render_process_host() const {
@@ -307,24 +312,19 @@ void ExtensionHost::DidStopLoading(content::RenderViewHost* render_view_host) {
   did_stop_loading_ = true;
   OnDidStopLoading();
   if (notify) {
-    // Log metrics. It's tempting to CHECK(load_start_) here, but it's possible
-    // to get a DidStopLoading even if we never started loading, in convoluted
-    // notification and observer chains.
-    if (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());
       }
+    } else if (extension_host_type_ == VIEW_TYPE_EXTENSION_POPUP) {
+      UMA_HISTOGRAM_MEDIUM_TIMES("Extensions.PopupLoadTime2",
+                                 load_start_->Elapsed());
     }
-
     // Send the notification last, because it might result in this being
     // deleted.
     content::NotificationService::current()->Notify(