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 187f3c42b3e7d0eca942e47303532da9095a4b48..e2808e9420ce630a47fb382fc0882c8b4255756e 100644
--- a/content/browser/web_contents/web_contents_impl_unittest.cc
+++ b/content/browser/web_contents/web_contents_impl_unittest.cc
@@ -2879,6 +2879,69 @@ 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. This can happen if a same-process non user-initiated
+// navigation completes while there is an ongoing cross-process navigation.
+// TODO(fdegans): Rewrite the test for PlzNavigate when DidStartLoading and
+// DidStopLoading are properly called.
+TEST_F(WebContentsImplTest, NoEarlyStop) {
+  const GURL kUrl1("http://www.chromium.org");
+  const GURL kUrl2("http://www.google.com");
+  const GURL kUrl3("http://www.wikipedia.org");
+
+  contents()->NavigateAndCommit(kUrl1);
+
+  TestRenderFrameHost* current_rfh = contents()->GetMainFrame();
+
+  // Start a browser-initiated cross-process navigation to |kUrl2|. There should
+  // be a pending RenderFrameHost and the WebContents should be loading.
+  controller().LoadURL(
+      kUrl2, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  TestRenderFrameHost* pending_rfh = contents()->GetPendingMainFrame();
+  ASSERT_NE(pending_rfh, nullptr);

[carlosk, 2015/03/03 15:29:12]

In the lines of what was mentioned earlier this should be change this to
ASSERT_FALSE(pending_rfh).

[Fabrice (no longer in Chrome), 2015/03/04 18:17:58]

In this specific case I prefer it for readability and semantic purposes. You're
not testing a boolean but a pointer.

[clamy, 2015/03/04 18:28:49]

We use ASSERT_FALSE(ptr) everywhere in the unit tests and ASSERT_NE(ptr,
nullptr) nowhere. As a rule of thumb, you should try to align yourself to the
style of what you are modifying.

[Fabrice (no longer in Chrome), 2015/03/05 12:37:42]

We actually want ASSERT_TRUE here.
Done.

+  EXPECT_TRUE(contents()->IsLoading());
+
+  // The current RenderFrameHost starts a non user-initiated render-initiated
+  // navigation and sends a DidStartLoading IPC. The WebContents still be

[carlosk, 2015/03/03 15:29:13]

nit: ... WebContents *should* still...

[Fabrice (no longer in Chrome), 2015/03/04 18:17:58]

I accidentally the verb.
Done.

+  // loading.
+  current_rfh->OnMessageReceived(
+    FrameHostMsg_DidStartLoading(current_rfh->GetRoutingID(), false));
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Simulate the pending RenderFrameHost DidStartLoading. There should still be
+  // a pending RenderFrameHost and the WebContents should still be loading.
+  pending_rfh->PrepareForCommit(kUrl2);
+  pending_rfh->OnMessageReceived(
+      FrameHostMsg_DidStartLoading(pending_rfh->GetRoutingID(), false));
+  EXPECT_EQ(contents()->GetPendingMainFrame(), pending_rfh);
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Simulate the current RenderFrameHost DidStopLoading. There should still be
+  // a pending RenderFrameHost and the WebContents should still be loading.
+  current_rfh->SendNavigate(1, kUrl3);

[carlosk, 2015/03/03 15:29:12]

I think the comments are a bit out of place. This is the point where the
renderer-initiated non user-initiated navigation is received.

[clamy, 2015/03/03 17:01:09]

On the renderer side it started when we received the DidStartLoading IPC. Maybe
something like "Simulate the commit and DidStopLoading from the
renderer-initiated in the current RenderFrameHost" is a bit clearer.

[Fabrice (no longer in Chrome), 2015/03/04 18:17:58]

Done.

+  current_rfh->OnMessageReceived(
+      FrameHostMsg_DidStopLoading(current_rfh->GetRoutingID()));
+  EXPECT_EQ(contents()->GetPendingMainFrame(), pending_rfh);
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Commit the navigation, the formerly pending RenderFrameHost should now be
+  // the current RenderFrameHost and the WebContents should still be loading.
+  contents()->TestDidNavigate(pending_rfh, 1, kUrl2,
+                              ui::PAGE_TRANSITION_TYPED);
+  EXPECT_EQ(contents()->GetPendingMainFrame(), nullptr);

[carlosk, 2015/03/03 15:29:13]

Replace with: EXPECT_FALSE(contents()->GetPendingMainFrame());

But I do see that in this file there's a lot of instances of pointer checks
using |== NULL| and |!= NULL|.

[Fabrice (no longer in Chrome), 2015/03/04 18:17:58]

Same as above.

[clamy, 2015/03/04 18:28:49]

Same here, please do the change asked by carlosk@

[Fabrice (no longer in Chrome), 2015/03/05 12:37:42]

Done.

+  TestRenderFrameHost* new_current_rfh = contents()->GetMainFrame();
+  ASSERT_EQ(new_current_rfh, pending_rfh);
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Simulate the new current RenderFrameHost DidStopLoading. The WebContents
+  // should now have stopped loading.
+  new_current_rfh->OnMessageReceived(
+      FrameHostMsg_DidStopLoading(new_current_rfh->GetRoutingID()));
+  EXPECT_EQ(contents()->GetMainFrame(), new_current_rfh);
+  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.