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

extensions/browser/extension_host.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/browser/extension_host.h"
 
 #include <list>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 145 matching lines @@
     UMA_HISTOGRAM_LONG_TIMES("Extensions.EventPageActiveTime2",
                              load_start_->Elapsed());
   }
   content::NotificationService::current()->Notify(
       extensions::NOTIFICATION_EXTENSION_HOST_DESTROYED,
       content::Source<BrowserContext>(browser_context_),
       content::Details<ExtensionHost>(this));
   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 {
   return render_view_host()->GetProcess();
 }
 
 RenderViewHost* ExtensionHost::render_view_host() const {
   // TODO(mpcomplete): This can be NULL. How do we handle that?
   return render_view_host_;
 }
@@ skipping 124 matching lines @@
       extensions::NOTIFICATION_EXTENSION_PROCESS_TERMINATED,
       content::Source<BrowserContext>(browser_context_),
       content::Details<ExtensionHost>(this));
 }
 
 void ExtensionHost::DidStopLoading(content::RenderViewHost* render_view_host) {
   bool notify = !did_stop_loading_;
   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(
         extensions::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING,
         content::Source<BrowserContext>(browser_context_),
         content::Details<ExtensionHost>(this));
   }
 }
 
 void ExtensionHost::OnDidStopLoading() {
@@ skipping 146 matching lines @@
   return delegate_->CheckMediaAccessPermission(
       web_contents, security_origin, type, extension());
 }
 
 bool ExtensionHost::IsNeverVisible(content::WebContents* web_contents) {
   ViewType view_type = extensions::GetViewType(web_contents);
   return view_type == extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE;
 }
 
 }  // namespace extensions