Remove the is_loading_ field from WebContentsImpl

content/browser/frame_host/render_frame_host_impl.cc

 // Copyright 2013 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 "content/browser/frame_host/render_frame_host_impl.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/containers/hash_tables.h"
@@ skipping 195 matching lines @@
       pending_web_ui_type_(WebUI::kNoWebUI),
       should_reuse_web_ui_(false),
       weak_ptr_factory_(this) {
   bool is_swapped_out = !!(flags & CREATE_RF_SWAPPED_OUT);
   bool hidden = !!(flags & CREATE_RF_HIDDEN);
   frame_tree_->AddRenderViewHostRef(render_view_host_);
   GetProcess()->AddRoute(routing_id_, this);
   g_routing_id_frame_map.Get().insert(std::make_pair(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_),
       this));
+  site_instance_->AddObserver(this);
 
   if (is_swapped_out) {
     rfh_state_ = STATE_SWAPPED_OUT;
   } else {
     rfh_state_ = STATE_DEFAULT;
     GetSiteInstance()->IncrementActiveFrameCount();
   }
 
   // New child frames should inherit the nav_entry_id of their parent.
   if (frame_tree_node_->parent()) {
@@ skipping 25 matching lines @@
 
 RenderFrameHostImpl::~RenderFrameHostImpl() {
   // Release the WebUI instances before all else as the WebUI may accesses the
   // RenderFrameHost during cleanup.
   ClearAllWebUI();
 
   GetProcess()->RemoveRoute(routing_id_);
   g_routing_id_frame_map.Get().erase(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_));
 
+  site_instance_->RemoveObserver(this);
+
   if (delegate_ && render_frame_created_)
     delegate_->RenderFrameDeleted(this);
 
   bool is_active = IsRFHStateActive(rfh_state_);
 
   // If this RenderFrameHost is swapped out, it already decremented the active
   // frame count of the SiteInstance it belongs to.
   if (is_active)
     GetSiteInstance()->DecrementActiveFrameCount();
 
@@ skipping 235 matching lines @@
     IPC_MESSAGE_HANDLER(FrameHostMsg_Detach, OnDetach)
     IPC_MESSAGE_HANDLER(FrameHostMsg_FrameFocused, OnFrameFocused)
     IPC_MESSAGE_HANDLER(FrameHostMsg_DidStartProvisionalLoad,
                         OnDidStartProvisionalLoad)
     IPC_MESSAGE_HANDLER(FrameHostMsg_DidFailProvisionalLoadWithError,
                         OnDidFailProvisionalLoadWithError)
     IPC_MESSAGE_HANDLER(FrameHostMsg_DidFailLoadWithError,
                         OnDidFailLoadWithError)
     IPC_MESSAGE_HANDLER_GENERIC(FrameHostMsg_DidCommitProvisionalLoad,
                                 OnDidCommitProvisionalLoad(msg))
-    IPC_MESSAGE_HANDLER(FrameHostMsg_DidDropNavigation, OnDidDropNavigation)
     IPC_MESSAGE_HANDLER(FrameHostMsg_UpdateState, OnUpdateState)
     IPC_MESSAGE_HANDLER(FrameHostMsg_OpenURL, OnOpenURL)
     IPC_MESSAGE_HANDLER(FrameHostMsg_DocumentOnLoadCompleted,
                         OnDocumentOnLoadCompleted)
     IPC_MESSAGE_HANDLER(FrameHostMsg_BeforeUnload_ACK, OnBeforeUnloadACK)
     IPC_MESSAGE_HANDLER(FrameHostMsg_SwapOut_ACK, OnSwapOutACK)
     IPC_MESSAGE_HANDLER(FrameHostMsg_ContextMenu, OnContextMenu)
     IPC_MESSAGE_HANDLER(FrameHostMsg_JavaScriptExecuteResponse,
                         OnJavaScriptExecuteResponse)
     IPC_MESSAGE_HANDLER(FrameHostMsg_VisualStateResponse,
@@ skipping 152 matching lines @@
 
 gfx::NativeViewAccessible
     RenderFrameHostImpl::AccessibilityGetNativeViewAccessible() {
   RenderWidgetHostViewBase* view = static_cast<RenderWidgetHostViewBase*>(
       render_view_host_->GetWidget()->GetView());
   if (view)
     return view->AccessibilityGetNativeViewAccessible();
   return NULL;
 }
 
+void RenderFrameHostImpl::ActiveFrameCountIsZero(
+    SiteInstanceImpl* site_instance) {}
+
+void RenderFrameHostImpl::RenderProcessGone(SiteInstanceImpl* site_instance) {

[ncarter (slow), 2016/01/21 00:26:40]

I'm glad you've done this. I also have envisioned making RenderFrameHostImpl be
a SiteInstanceImpl::Observer, as a way of replacing
FrameHostMsg_RenderProcessGone. 

Note this bug: https://code.google.com/p/chromium/issues/detail?id=575400

+  DCHECK(site_instance == site_instance_.get());
+
+  // The render process is gone, this frame can no longer be loading.
+  ResetLoadingState();

[nasko, 2016/01/20 20:58:34]

Let's not add implementation of SiteInstanceImpl::Observer here, since it
requires adding an empty method. RFHM is already an observer and is already
changing to support calling ResetLoadingState() on its RFH. Let's just move this
code to its implementation of RenderProcessGone.

[ncarter (slow), 2016/01/21 00:26:40]

I'm not sure I agree with nasko. I think it's more natural for the RFH to be the
SiteInstanceImpl::Observer.

RFHM currently observes the siteinstance on behalf of its proxies, and you can't
observe the same siteinstance twice, it seems troublesome for the RFHM to avoid
double-observation if it has to observe on behalf of its RFH's as well. We could
potentially wind up with a proxy in the same siteinstance as a pending delete
host, say -- so you'd need some kind of refcounting scheme.

Moreover, if RFH (instead of RFHM) observes the SiteInstance, then we get RAII
guarantees that AddObserver/RemoveObserver calls are balanced. If we do it in
the RFHM then it's yet another thing we have to manage manually for all the
various speculative/pending/ subcases.

[clamy, 2016/01/22 13:08:15]

I initially tried to make things work in RFHM::RenderProcessGone, but getting to
observe the right SiteInstances was somewhat complex. In particular, I was
worried about some edge cases (such as when we create a pending RFH for a
SiteInstance that was a before a RFHP and that pending RFH later gets destroyed
and we recreate a RFHP for it).

I think it is much easier conceptually to have RFH be the SiteInstanceObserver:
you start observing the SiteInstance at creation of the RFH and stop at
deletion, and just reset your own loading state when it is gone. As for the
empty method, if we go on the route of RFH being an observer, can we just change
the SiteInstanceObserver::ActiveFrameCountIsZero from pure virtual to just
virtual?

[nasko, 2016/01/22 17:52:59]

I would prefer not to have the empty method here.

[clamy, 2016/01/25 14:01:18]

I made it non pure virtual in SiteInstanceImpl::Observer. Let me know what you
think.

+}
+
 bool RenderFrameHostImpl::CreateRenderFrame(int proxy_routing_id,
                                             int opener_routing_id,
                                             int parent_routing_id,
                                             int previous_sibling_routing_id) {
   TRACE_EVENT0("navigation", "RenderFrameHostImpl::CreateRenderFrame");
   DCHECK(!IsRenderFrameLive()) << "Creating frame twice";
 
   // The process may (if we're sharing a process with another host that already
   // initialized it) or may not (we have our own process or the old process
   // crashed) have been initialized. Calling Init multiple times will be
@@ skipping 331 matching lines @@
       !validated_params.was_within_same_page) {
     RenderWidgetHostImpl::From(GetView()->GetRenderWidgetHost())
         ->StartNewContentRenderingTimeout();
   }
 
   // PlzNavigate
   if (IsBrowserSideNavigationEnabled())
     pending_commit_ = false;
 }
 
-void RenderFrameHostImpl::OnDidDropNavigation() {
-  // At the end of Navigate(), the FrameTreeNode's DidStartLoading is called to
-  // force the spinner to start, even if the renderer didn't yet begin the load.
-  // If it turns out that the renderer dropped the navigation, the spinner needs
-  // to be turned off.
-  frame_tree_node_->DidStopLoading();
-  navigation_handle_.reset();
-}
-
 void RenderFrameHostImpl::OnUpdateState(const PageState& state) {
   // TODO(creis): Verify the state's ISN matches the last committed FNE.
 
   // Without this check, the renderer can trick the browser into using
   // filenames it can't access in a future session restore.
   // TODO(creis): Move CanAccessFilesOfPageState to RenderFrameHostImpl.
   if (!render_view_host_->CanAccessFilesOfPageState(state)) {
     bad_message::ReceivedBadMessage(
         GetProcess(), bad_message::RFH_CAN_ACCESS_FILES_OF_PAGE_STATE);
     return;
@@ skipping 241 matching lines @@
       frame_tree_node_->render_manager()->IsPendingDeletion(this)) {
     render_view_host_->set_is_active(false);
     render_view_host_->set_is_swapped_out(true);
   }
 
   if (frame_tree_node_->render_manager()->DeleteFromPendingList(this)) {
     // We are now deleted.
     return;
   }
 
+  ResetLoadingState();
+
   // If this RFH wasn't pending deletion, then it is now swapped out.
   SetState(RenderFrameHostImpl::STATE_SWAPPED_OUT);
 }
 
 void RenderFrameHostImpl::OnContextMenu(const ContextMenuParams& params) {
   // Validate the URLs in |params|.  If the renderer can't request the URLs
   // directly, don't show them in the context menu.
   ContextMenuParams validated_params(params);
   RenderProcessHost* process = GetProcess();
 
@@ skipping 365 matching lines @@
     delegate_->EnterFullscreenMode(GetLastCommittedURL().GetOrigin());
   else
     delegate_->ExitFullscreenMode();
 
   // The previous call might change the fullscreen state. We need to make sure
   // the renderer is aware of that, which is done via the resize message.
   render_view_host_->GetWidget()->WasResized();
 }
 
 void RenderFrameHostImpl::OnDidStartLoading(bool to_different_document) {
-  // Any main frame load to a new document should reset the load since it will
-  // replace the current page and any frames.
-  if (to_different_document && !GetParent())
-    is_loading_ = false;
-
-  // This method should never be called when the frame is loading.
-  // Unfortunately, it can happen if a history navigation happens during a
-  // BeforeUnload or Unload event.
-  // TODO(fdegans): Change this to a DCHECK after LoadEventProgress has been
-  // refactored in Blink. See crbug.com/466089
-  if (is_loading_) {
-    LOG(WARNING) << "OnDidStartLoading was called twice.";
-    return;
-  }
-
   frame_tree_node_->DidStartLoading(to_different_document);
   is_loading_ = true;
 }
 
 void RenderFrameHostImpl::OnDidStopLoading() {
   // This method should never be called when the frame is not loading.
   // Unfortunately, it can happen if a history navigation happens during a
   // BeforeUnload or Unload event.
   // TODO(fdegans): Change this to a DCHECK after LoadEventProgress has been
   // refactored in Blink. See crbug.com/466089
@@ skipping 176 matching lines @@
   // message will be received asynchronously from the UI of the browser. But the
   // throbber needs to be kept in sync with what's happening in the UI. For
   // example, the throbber will start immediately when the user navigates even
   // if the renderer is delayed. There is also an issue with the throbber
   // starting because the WebUI (which controls whether the favicon is
   // displayed) happens synchronously. If the start loading messages was
   // asynchronous, then the default favicon would flash in.
   //
   // Blink doesn't send throb notifications for JavaScript URLs, so it is not
   // done here either.
-  if (!common_params.url.SchemeIs(url::kJavaScriptScheme))
+  if (!common_params.url.SchemeIs(url::kJavaScriptScheme)) {
     frame_tree_node_->DidStartLoading(true);
+    is_loading_ = true;
+  }
 }
 
 void RenderFrameHostImpl::NavigateToInterstitialURL(const GURL& data_url) {
   DCHECK(data_url.SchemeIs(url::kDataScheme));
   CommonNavigationParams common_params(
       data_url, Referrer(), ui::PAGE_TRANSITION_LINK,
       FrameMsg_Navigate_Type::NORMAL, false, false, base::TimeTicks::Now(),
       FrameMsg_UILoadMetricsReportType::NO_REPORT, GURL(), GURL(), LOFI_OFF,
       base::TimeTicks::Now());
   if (IsBrowserSideNavigationEnabled()) {
@@ skipping 320 matching lines @@
 
 const image_downloader::ImageDownloaderPtr&
 RenderFrameHostImpl::GetMojoImageDownloader() {
   if (!mojo_image_downloader_.get() && GetServiceRegistry()) {
     GetServiceRegistry()->ConnectToRemoteService(
         mojo::GetProxy(&mojo_image_downloader_));
   }
   return mojo_image_downloader_;
 }
 
+void RenderFrameHostImpl::ResetLoadingState() {
+  if (is_loading()) {
+    // Do not send notifications about load state when swapped out or pending
+    // swap out.
+    if (rfh_state_ == STATE_DEFAULT)
+      OnDidStopLoading();
+    else
+      is_loading_ = false;

[nasko, 2016/01/20 20:58:34]

Shouldn't this always be set, regardless of state? Also, swapped out is now
disabled, so we can consider it gone, so let's avoid using it in comments. It
can be rephrased as "when pending deletion."

[clamy, 2016/01/22 13:08:15]

OnDidStopLoading will set it. If it is set beforehand, then OnDidStopLoading
just returns early without informing the tree of the change in load state.

I have updated the comment to make it clearer.

+  }
+}
+
 bool RenderFrameHostImpl::IsSameSiteInstance(
     RenderFrameHostImpl* other_render_frame_host) {
   // As a sanity check, make sure the frame belongs to the same BrowserContext.
   CHECK_EQ(GetSiteInstance()->GetBrowserContext(),
            other_render_frame_host->GetSiteInstance()->GetBrowserContext());
   return GetSiteInstance() == other_render_frame_host->GetSiteInstance();
 }
 
 void RenderFrameHostImpl::SetAccessibilityMode(AccessibilityMode mode) {
   Send(new FrameMsg_SetAccessibilityMode(routing_id_, mode));
@@ skipping 283 matching lines @@
   *dst = src;
 
   if (src.routing_id != -1)
     dst->tree_id = RoutingIDToAXTreeID(src.routing_id);
 
   if (src.parent_routing_id != -1)
     dst->parent_tree_id = RoutingIDToAXTreeID(src.parent_routing_id);
 }
 
 }  // namespace content