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

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 "base/logging.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/profiler/scoped_tracker.h"
@@ skipping 40 matching lines @@
 
 ExtensionHost::ExtensionHost(const Extension* extension,
                              SiteInstance* site_instance,
                              const GURL& url,
                              ViewType host_type)
     : delegate_(ExtensionsBrowserClient::Get()->CreateExtensionHostDelegate()),
       extension_(extension),
       extension_id_(extension->id()),
       browser_context_(site_instance->GetBrowserContext()),
       render_view_host_(nullptr),
-      did_stop_loading_(false),
+      is_loading_(false),
+      has_loaded_once_(false),
       document_element_available_(false),
       initial_url_(url),
       extension_function_dispatcher_(browser_context_, this),
       extension_host_type_(host_type) {
   // Not used for panels, see PanelHost.
   DCHECK(host_type == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE ||
          host_type == VIEW_TYPE_EXTENSION_DIALOG ||
          host_type == VIEW_TYPE_EXTENSION_POPUP);
   host_contents_.reset(WebContents::Create(
       WebContents::CreateParams(browser_context_, site_instance))),
@@ skipping 13 matching lines @@
 
 ExtensionHost::~ExtensionHost() {
   ExtensionRegistry::Get(browser_context_)->RemoveObserver(this);
 
   if (extension_host_type_ == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE &&
       extension_ && BackgroundInfo::HasLazyBackgroundPage(extension_) &&
       load_start_.get()) {
     UMA_HISTOGRAM_LONG_TIMES("Extensions.EventPageActiveTime2",
                              load_start_->Elapsed());
   }
+
+  // Record UMA now because we're about to stop observing our WebContents.

[Yoyo Zhou, 2015/03/12 22:57:29]

We'll be recording incomplete loads? Hope that's okay.

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

Yeah I couldn't decide whether or not this was a good thing. Maybe it does skew
the results in an overall bad way. Funnily enough, adding this behavior was part
of the reason I touched this file in the first place. That would make this CL
basically a rename of has_loaded and removing some of the unnecessary
assertions.

Well - SGTM. Ok.

+  if (is_loading_)
+    RecordStopLoadingUMA();
+
   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));
   FOR_EACH_OBSERVER(DeferredStartRenderHostObserver,
                     deferred_start_render_host_observer_list_,
                     OnDeferredStartRenderHostDestroyed(this));
+
+  // 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);
 }
 
 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 93 matching lines @@
 }
 
 void ExtensionHost::LoadInitialURL() {
   load_start_.reset(new base::ElapsedTimer());
   host_contents_->GetController().LoadURL(
       initial_url_, content::Referrer(), ui::PAGE_TRANSITION_LINK,
       std::string());
 }
 
 bool ExtensionHost::IsBackgroundPage() const {
-  DCHECK(extension_host_type_ == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE);
+  DCHECK_EQ(extension_host_type_, VIEW_TYPE_EXTENSION_BACKGROUND_PAGE);
   return true;
 }
 
 void ExtensionHost::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,
     UnloadedExtensionInfo::Reason reason) {
   // The extension object will be deleted after this notification has been sent.
   // Null it out so that dirty pointer issues don't arise in cases when multiple
   // ExtensionHost objects pointing to the same Extension are present.
@@ skipping 22 matching lines @@
   // TODO(aa): This is suspicious. There can be multiple views in an extension,
   // and they aren't all going to use ExtensionHost. This should be in someplace
   // more central, like EPM maybe.
   content::NotificationService::current()->Notify(
       extensions::NOTIFICATION_EXTENSION_PROCESS_TERMINATED,
       content::Source<BrowserContext>(browser_context_),
       content::Details<ExtensionHost>(this));
 }
 
 void ExtensionHost::DidStartLoading(content::RenderViewHost* render_view_host) {
+  CHECK(!is_loading_);
+  is_loading_ = true;
   FOR_EACH_OBSERVER(DeferredStartRenderHostObserver,
                     deferred_start_render_host_observer_list_,
                     OnDeferredStartRenderHostDidStartLoading(this));
 }
 
 void ExtensionHost::DidStopLoading(content::RenderViewHost* render_view_host) {
-  bool notify = !did_stop_loading_;
-  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",
-                                 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));
-  }
+  CHECK(is_loading_);
+  is_loading_ = false;
+  has_loaded_once_ = true;
+  RecordStopLoadingUMA();
+  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() {
-  DCHECK(extension_host_type_ == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE);
+  DCHECK_EQ(extension_host_type_, VIEW_TYPE_EXTENSION_BACKGROUND_PAGE);
   // Nothing to do for background pages.
 }
 
 void ExtensionHost::DocumentAvailableInMainFrame() {
   // If the document has already been marked as available for this host, then
   // bail. No need for the redundant setup. http://crbug.com/31170
   if (document_element_available_)
     return;
   document_element_available_ = true;
 
@@ skipping 144 matching lines @@
     content::MediaStreamType type) {
   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;
 }
 
+void ExtensionHost::RecordStopLoadingUMA() {
+  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());
+  }
+}
+
 }  // namespace extensions