Refactor the loading tracking logic in WebContentsImpl.

content/browser/web_contents/web_contents_impl_unittest.cc

 // Copyright (c) 2012 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 "base/command_line.h"
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/frame_host/cross_site_transferring_request.h"
 #include "content/browser/frame_host/interstitial_page_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
@@ skipping 2860 matching lines @@
   EXPECT_TRUE(observer.is_loading());
 
   // Send the DidStopLoading for the main frame and ensure it isn't loading
   // anymore.
   orig_rfh->OnMessageReceived(
       FrameHostMsg_DidStopLoading(orig_rfh->GetRoutingID()));
   EXPECT_FALSE(contents()->IsLoading());
   EXPECT_FALSE(observer.is_loading());
 }
 
+// Ensure that WebContentsImpl does not stop loading too early when there still
+// is a pending renderer. The sequence to reproduce the issue is as follows:
+// * StartLoading on a frame.
+// * Start another navigation before the previous frame has finished loading.
+// * StopLoading on the previous navigation.
+// At this point, the WebContentsImpl should still be loading as there is still
+// one pending navigation.

[clamy, 2015/02/26 16:05:00]

I think currently the test is not doing quite what you intend it to do.

Something better would be along the lines of:
- Navigate and commit url1.
- Start cross-site navigation to url2. This creates a pending RFH.
- Receive a DidStartLoading IPC from the pending RFH.
- Receive a DidStartLoading IPC from the current RFH (reflecting a
renderer-initiated navigation).
- Get a DidNavigate IPC from the current RFH for a non-user initiated
navigation. You should still have the pending RFH at that point.
- Get the DidStopLoading from the current RFH. You should still be loading.
- At that point the pending RFH commits ie you receive a DidNavigate from it.
The original RFH is swapped out.
- Finally you get the DidStopLoading from the (now) current RFH. You should have
stopped loading at that point.

[Fabrice (no longer in Chrome), 2015/02/27 17:45:25]

Thanks a lot for the explanation! I think I'm now doing the right thing.

+TEST_F(WebContentsImplTest, NoEarlyStop) {
+  const GURL orig_url("http://www.chromium.org");
+  const GURL new_url("http://www.google.com");
+
+  TestRenderFrameHost* orig_rfh = contents()->GetMainFrame();
+
+  // Navigate the RenderFrame, simulate the DidStartLoading, and commit.
+  // The frame should still be loading.
+  controller().LoadURL(
+      orig_url, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  orig_rfh->OnMessageReceived(
+      FrameHostMsg_DidStartLoading(orig_rfh->GetRoutingID(), false));
+  contents()->TestDidNavigate(orig_rfh, 1, orig_url, ui::PAGE_TRANSITION_TYPED);
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(orig_rfh, contents()->GetMainFrame());
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Navigate to new site, simulate the DidStartLoading, and commit.
+  // The frame should still be loading.
+  controller().LoadURL(
+      new_url, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation)) {
+    orig_rfh->PrepareForCommit(new_url);
+  }
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+
+  TestRenderFrameHost* new_rfh = contents()->GetPendingMainFrame();
+  ASSERT_NE(new_rfh, nullptr);
+  EXPECT_NE(orig_rfh, new_rfh);
+  new_rfh->OnMessageReceived(
+      FrameHostMsg_DidStartLoading(new_rfh->GetRoutingID(), false));
+  contents()->TestDidNavigate(new_rfh, 1, new_url, ui::PAGE_TRANSITION_TYPED);
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Simulate the DidStopLoading on the original navigation. The frame should
+  // still be loading.

[clamy, 2015/02/26 16:05:00]

At that point the original RFH is swapped out (it got swapped out following
TestDidNavigate). Therefore the DidStopLoading IPC will be filtered out, and
WebContents should not even be informed.

[Fabrice (no longer in Chrome), 2015/02/27 17:45:25]

Acknowledged.

+  orig_rfh->OnMessageReceived(
+      FrameHostMsg_DidStopLoading(orig_rfh->GetRoutingID()));
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Simulate the DidStopLoading on the new navigation. The frame should now
+  // have stopped loading.
+  new_rfh->OnMessageReceived(
+      FrameHostMsg_DidStopLoading(new_rfh->GetRoutingID()));
+  EXPECT_FALSE(contents()->IsLoading());
+}
+
 // ChromeOS doesn't use WebContents based power save blocking.
 #if !defined(OS_CHROMEOS)
 TEST_F(WebContentsImplTest, MediaPowerSaveBlocking) {
   // PlayerIDs are actually pointers cast to int64, so verify that both negative
   // and positive player ids don't blow up.
   const int kPlayerAudioVideoId = 15;
   const int kPlayerAudioOnlyId = -15;
   const int kPlayerVideoOnlyId = 30;
   const int kPlayerRemoteId = -30;
 
@@ skipping 147 matching lines @@
   frame->SendBeforeUnloadHandlersPresent(false);
   EXPECT_FALSE(frame->SuddenTerminationAllowed());
   frame->SendBeforeUnloadHandlersPresent(true);
   frame->SendUnloadHandlersPresent(false);
   EXPECT_FALSE(frame->SuddenTerminationAllowed());
   frame->SendBeforeUnloadHandlersPresent(false);
   EXPECT_TRUE(frame->SuddenTerminationAllowed());
 }
 
 }  // namespace content