Refactor the loading tracking logic in WebContentsImpl.

content/browser/web_contents/web_contents_impl_unittest.cc

Index: content/browser/web_contents/web_contents_impl_unittest.cc
diff --git a/content/browser/web_contents/web_contents_impl_unittest.cc b/content/browser/web_contents/web_contents_impl_unittest.cc
index da3c82e7d4826fe968344dadc907178eca8bb1d1..85fb7d74238a097b35b47fe4b34579227e2d7955 100644
--- a/content/browser/web_contents/web_contents_impl_unittest.cc
+++ b/content/browser/web_contents/web_contents_impl_unittest.cc
@@ -2879,6 +2879,71 @@ TEST_F(WebContentsImplTest, StartStopEventsBalance) {
   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.