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:
+// * Navigate and commit url1.
+// * Start a cross-site navigation to url2.
+// * Receive a DidStartLoading IPC from the pending RFH.
+// * Receive a DidStartLoading IPC from the current RFH.
+// * Receive a DidStopLoading IPC from the current RFH. At this point, the
+//   WebContentsImpl should still be loading as there still is a pending frame.
+//   Additionally, the now current RFH should be the previous loading RFH.
+// * Finally, receive a DidStopLoading IPC from the now current RFH.
+//   WebContentsImpl should have stopped loading at this point.

[clamy, 2015/03/02 10:30:59]

I think this is a bit too long as a comment. Instead of the complete
description, which people can find by reading the comments inside the test, how
about the following: "This can happen if a same-process non user-initiated
navigation completes while there is an ongoing cross-process navigation."?

[Fabrice (no longer in Chrome), 2015/03/02 18:01:40]

Done.

+TEST_F(WebContentsImplTest, NoEarlyStop) {
+  const GURL url1("http://www.chromium.org");

[clamy, 2015/03/02 10:30:59]

Since these are const values, rename them kUrl1/kUrl2.

[Fabrice (no longer in Chrome), 2015/03/02 18:01:40]

Done.

+  const GURL url2("http://www.google.com");
+
+  TestRenderFrameHost* current_rfh = contents()->GetMainFrame();
+
+  // Navigate the RenderFrameHost and commit. The frame should still be loading.

[clamy, 2015/03/02 10:30:59]

You can replace all this below with contents()->NavigateAndCommit(kUrl1).

[Fabrice (no longer in Chrome), 2015/03/02 18:01:40]

Done.

+  controller().LoadURL(
+      url1, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  contents()->TestDidNavigate(current_rfh, 1, url1, ui::PAGE_TRANSITION_TYPED);
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(current_rfh, contents()->GetMainFrame());
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Navigate to new site and commit. The frame should still be loading.

[clamy, 2015/03/02 10:30:59]

The comment should indicate that this is a browser-initiated cross-process
navigation to url2. Also you are not committing the pending navigation at that
point (otherwise we don't test what we want since the pending_rfh will be
swapped in).

[Fabrice (no longer in Chrome), 2015/03/02 18:01:40]

Done.

+  controller().LoadURL(
+      url2, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  current_rfh->PrepareForCommit(url2);

[clamy, 2015/03/02 10:30:58]

I don't think we should get to that step right now.

[Fabrice (no longer in Chrome), 2015/03/02 18:01:39]

Done.

+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  TestRenderFrameHost* pending_rfh = contents()->GetPendingMainFrame();
+  ASSERT_NE(pending_rfh, nullptr);
+  EXPECT_NE(current_rfh, pending_rfh);
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Simulate the current RFH DidStartLoading. The frame should still be
+  // loading.

[clamy, 2015/03/02 10:30:59]

What you want to do here is simulate a renderer-initiated non user-initiated
navigation. The comment should reflect that. In order to have the test properly
work with PlzNavigate you may want to have a look a carlosk's patch
https://codereview.chromium.org/953503002/ which introduces a
TestRenderFrameHost::SendRendererInitiatedNavigationRequest that you will need
to use here when the patch lands.

[Fabrice (no longer in Chrome), 2015/03/02 18:01:40]

I suggest proceeding in another fashion, this test is going to fail with
PlzNavigate anyway because we don't properly call Did(Start|Stop)Loading. I will
add a TODO to revisit this test when I add the proper calls for PlzNavigate.
Does that sound good to you?

+  current_rfh->OnMessageReceived(
+      FrameHostMsg_DidStartLoading(current_rfh->GetRoutingID(), false));
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Simulate the pending RFH DidStartLoading. The frame should still be
+  // loading.
+  pending_rfh->OnMessageReceived(
+      FrameHostMsg_DidStartLoading(pending_rfh->GetRoutingID(), false));
+  contents()->TestDidNavigate(pending_rfh, 1, url2,

[clamy, 2015/03/02 10:30:59]

The actual commit in the pending RFH should happen after the DidStopLoading in
the current one (otherwise as in the previous version of the patch you simulate
an IPC on a swapped out RFH that will get filtered out).

[Fabrice (no longer in Chrome), 2015/03/02 18:01:39]

Done.

+                              ui::PAGE_TRANSITION_TYPED);
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Simulate the original RFH DidStopLoading. The frame should still be
+  // loading and the pending RFH should now be the current RFH.

[clamy, 2015/03/02 10:30:59]

You should simulate the commit of the current RFH provisional load before the
DidStopLoading and make sure we still have a pending rfh at that point (see
RenderFrameHostManagerTest.PageDoesBackAndReload for when that happens).

[Fabrice (no longer in Chrome), 2015/03/02 18:01:39]

Done.

+  current_rfh->OnMessageReceived(
+      FrameHostMsg_DidStopLoading(current_rfh->GetRoutingID()));
+  EXPECT_TRUE(contents()->IsLoading());

[clamy, 2015/03/02 10:30:59]

Following this you should commit the navigation in the pending RFH, and test
what you have below (it becomes true following the call to
contents()->TestDidNavigate(pending_rfh, 1, url2, ui::PAGE_TRANSITION_TYPED);).

[Fabrice (no longer in Chrome), 2015/03/02 18:01:39]

Done.

+  EXPECT_EQ(contents()->GetPendingMainFrame(), nullptr);
+  TestRenderFrameHost *new_current_rfh = contents()->GetMainFrame();
+  ASSERT_EQ(new_current_rfh, pending_rfh);
+
+  // Simulate the new current RFH DidStopLoading. The frame should now have
+  // stopped loading.
+  new_current_rfh->OnMessageReceived(
+      FrameHostMsg_DidStopLoading(new_current_rfh->GetRoutingID()));
+  EXPECT_FALSE(contents()->IsLoading());
+}

[carlosk, 2015/03/02 14:17:29]

A couple general comments:
* Replace RFH in your comments with RenderFrameHost
* I agree we probably want to simulate a few navigation commit steps to make
sure the loading state is working properly at that point.

[Fabrice (no longer in Chrome), 2015/03/02 18:01:39]

Done.

+
 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