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 2861 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.
+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.

[nasko, 2015/02/26 23:30:34]

nit: RenderFrameHost

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

Acknowledged.

+  // 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);

[nasko, 2015/02/26 23:30:33]

Should this be in an if statement? I thought PrepareForCommit does the right
thing in all cases.

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

You are correct, fixed.

+  }
+  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.
+  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());
+}
+
 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;
 
   EXPECT_FALSE(contents()->has_audio_power_save_blocker_for_testing());
   EXPECT_FALSE(contents()->has_video_power_save_blocker_for_testing());
@@ skipping 144 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