[content] Improve loading simulation in SimulateLoadingCompleted.

Ensure that RenderFrameHostTester::SimulateLoadingCompleted
fires DocumentAvailable / DOMContentLoaded / OnLoadCompleted / DidStopLoading
callbacks in addition to DidStopLoading to simulate real-world loading
callback sequence more accurately.

This patch also adds a browser test to ensure that the loading callback
order is the same in the real world.

This patch is a prerequisite for exposing the loading stages from
RenderFrameHost / WebContents as the tests need these callbacks to
be dispatched to work correctly.

R=clamy@chromium.org,arthursonzogni@chromium.org,ahemery@chromium.org
TBR=jianli@chromium.org,pasko@chromium.org,dbeam@chromium.org,mastiz@chromium.org,drubery@chromium.org
CC=​​bfcache-bugs@chromium.org
BUG=1011770

Change-Id: Ibe59b1b131f3911fe9c4bf641e8ea1c6bb1ba545
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1834343
Commit-Queue: Alexander Timin <altimin@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Arthur Hemery <ahemery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705655}

chrome/browser/metrics/renderer_uptime_tracker.cc

@@ -38,7 +38,7 @@ RendererUptimeTracker* RendererUptimeTracker::SetMockRendererUptimeTracker(
 
 // static
 RendererUptimeTracker* RendererUptimeTracker::Get() {
-  DCHECK(g_renderer_uptime_tracker_instance);
+  // This can return null in unit tests.
   return g_renderer_uptime_tracker_instance;
 }
 

chrome/browser/metrics/renderer_uptime_web_contents_observer.cc

@@ -30,8 +30,11 @@ RendererUptimeWebContentsObserver::CreateForWebContents(
 }
 
 void RendererUptimeWebContentsObserver::DocumentAvailableInMainFrame() {
-  RendererUptimeTracker::Get()->OnLoadInMainFrame(
-      web_contents()->GetMainFrame()->GetProcess()->GetID());
+  // RendererUptimeTracker can be null in unittests.
+  if (RendererUptimeTracker* tracker = RendererUptimeTracker::Get()) {
+    tracker->OnLoadInMainFrame(
+        web_contents()->GetMainFrame()->GetProcess()->GetID());
+  }
 }
 
 WEB_CONTENTS_USER_DATA_KEY_IMPL(RendererUptimeWebContentsObserver)

chrome/browser/offline_pages/recent_tab_helper_unittest.cc

@@ -279,6 +279,7 @@ void RecentTabHelperTest::FastForwardSnapshotController() {
 void RecentTabHelperTest::StartAndCommitNavigation(
     std::unique_ptr<content::NavigationSimulator> simulator) {
   simulator->SetAutoAdvance(false);
+  simulator->SetKeepLoading(true);
   simulator->Start();
 
   // Need to flush the task queue manually since there may be async tasks

chrome/browser/predictors/loading_predictor_tab_helper_unittest.cc

@@ -98,6 +98,10 @@ void LoadingPredictorTabHelperTest::NavigateAndCommitInFrame(
     content::RenderFrameHost* rfh) {
   auto navigation =
       content::NavigationSimulator::CreateRendererInitiated(GURL(url), rfh);
+  // These tests simulate loading events manually.
+  // TODO(ahemery): Consider refactoring to rely on load events dispatched by
+  // NavigationSimulator.
+  navigation->SetKeepLoading(true);
   navigation->Start();
   navigation->Commit();
 }
@@ -114,6 +118,11 @@ TEST_F(LoadingPredictorTabHelperTest, MainFrameNavigation) {
 TEST_F(LoadingPredictorTabHelperTest, MainFrameNavigationWithRedirects) {
   auto navigation = content::NavigationSimulator::CreateRendererInitiated(
       GURL("http://test.org"), main_rfh());
+  // The problem here is that mock_collector_ is a strict mock, which expects
+  // a particular set of loading events and fails when extra is present.
+  // TOOO(ahemery): Consider refactoring this to rely on loading events
+  // in NavigationSimulator.
+  navigation->SetKeepLoading(true);
   auto navigation_id = CreateNavigationID(GetTabID(), "http://test.org");
   EXPECT_CALL(*mock_collector_, RecordStartNavigation(navigation_id));
   navigation->Start();
@@ -143,6 +152,11 @@ TEST_F(LoadingPredictorTabHelperTest, SubframeNavigation) {
 TEST_F(LoadingPredictorTabHelperTest, MainFrameNavigationFailed) {
   auto navigation = content::NavigationSimulator::CreateRendererInitiated(
       GURL("http://test.org"), main_rfh());
+  navigation->SetKeepLoading(true);
+  // The problem here is that mock_collector_ is a strict mock, which expects
+  // a particular set of loading events and fails when extra is present.
+  // TOOO(ahemery): Consider refactoring this to rely on loading events
+  // in NavigationSimulator.
   auto navigation_id = CreateNavigationID(GetTabID(), "http://test.org");
   EXPECT_CALL(*mock_collector_, RecordStartNavigation(navigation_id));
   EXPECT_CALL(*mock_collector_,

chrome/browser/ui/search/search_tab_helper.cc

@@ -115,6 +115,9 @@ bool InInstantProcess(const InstantService* instant_service,
 // calculates and logs the total load time.
 void RecordNewTabLoadTime(content::WebContents* contents) {
   CoreTabHelper* core_tab_helper = CoreTabHelper::FromWebContents(contents);
+  // CoreTabHelper can be null in unittests.
+  if (!core_tab_helper)
+    return;
   if (core_tab_helper->new_tab_start_time().is_null())
     return;
 

components/favicon/content/content_favicon_driver_unittest.cc

@@ -15,6 +15,7 @@
 #include "components/favicon/core/test/mock_favicon_service.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/favicon_url.h"
+#include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_renderer_host.h"
 #include "content/public/test/web_contents_tester.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -79,8 +80,6 @@ class ContentFaviconDriverTest : public content::RenderViewHostTestHarness {
     ContentFaviconDriver* favicon_driver =
         ContentFaviconDriver::FromWebContents(web_contents());
     web_contents_tester()->NavigateAndCommit(page_url);
-    static_cast<content::WebContentsObserver*>(favicon_driver)
-        ->DocumentOnLoadCompletedInMainFrame();
     static_cast<content::WebContentsObserver*>(favicon_driver)
         ->DidUpdateFaviconURL(candidates);
     base::RunLoop().RunUntilIdle();
@@ -106,7 +105,10 @@ TEST_F(ContentFaviconDriverTest, ShouldCauseImageDownload) {
 TEST_F(ContentFaviconDriverTest, ShouldNotCauseImageDownload) {
   ContentFaviconDriver* favicon_driver =
       ContentFaviconDriver::FromWebContents(web_contents());
-  web_contents_tester()->NavigateAndCommit(kPageURL);
+  auto navigation = content::NavigationSimulator::CreateBrowserInitiated(
+      kPageURL, web_contents());
+  navigation->SetKeepLoading(true);
+  navigation->Commit();
   static_cast<content::WebContentsObserver*>(favicon_driver)
       ->DidUpdateFaviconURL({content::FaviconURL(
           kIconURL, content::FaviconURL::IconType::kFavicon, kEmptyIconSizes)});

components/safe_browsing/triggers/ad_sampler_trigger_unittest.cc

@@ -64,17 +64,7 @@ class AdSamplerTriggerTest : public content::RenderViewHostTestHarness {
   // Returns the final RenderFrameHost after navigation commits.
   RenderFrameHost* NavigateFrame(const std::string& url,
                                  RenderFrameHost* frame) {
-    GURL gurl(url);
-    auto navigation_simulator =
-        NavigationSimulator::CreateRendererInitiated(gurl, frame);
-    navigation_simulator->Commit();
-    RenderFrameHost* final_frame_host =
-        navigation_simulator->GetFinalRenderFrameHost();
-    // Call the trigger's FinishLoad event handler directly since it doesn't
-    // happen as part of the navigation.
-    safe_browsing::AdSamplerTrigger::FromWebContents(web_contents())
-        ->DidFinishLoad(final_frame_host, gurl);
-    return final_frame_host;
+    return NavigationSimulator::NavigateAndCommitFromDocument(GURL(url), frame);
   }
 
   // Returns the final RenderFrameHost after navigation commits.

content/browser/web_contents/web_contents_impl_browsertest.cc

@@ -4178,4 +4178,162 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
   histogram_tester.ExpectBucketCount(kMaxFrameCountUMA, /* bucket */ 3,
                                      /* count */ 1);
 }
+
+namespace {
+
+class LoadingObserver : public WebContentsObserver {
+ public:
+  explicit LoadingObserver(WebContents* web_contents)
+      : WebContentsObserver(web_contents) {}
+
+  std::vector<std::string>& GetEvents() { return events_; }
+
+  void DidStartNavigation(NavigationHandle* navigation_handle) override {
+    events_.push_back("DidStartNavigation");
+  }
+
+  void DidFinishNavigation(NavigationHandle* navigation_handle) override {
+    events_.push_back("DidFinishNavigation");
+  }
+
+  void DidStartLoading() override { events_.push_back("DidStartLoading"); }
+
+  void DidStopLoading() override {
+    events_.push_back("DidStopLoading");
+    run_loop_.Quit();
+  }
+
+  void DocumentAvailableInMainFrame() override {
+    events_.push_back("DocumentAvailableInMainFrame");
+  }
+
+  void DocumentOnLoadCompletedInMainFrame() override {
+    events_.push_back("DocumentOnLoadCompletedInMainFrame");
+  }
+
+  void DOMContentLoaded(RenderFrameHost* render_frame_host) override {
+    events_.push_back("DOMContentLoaded");
+  }
+
+  void DidFinishLoad(RenderFrameHost* render_frame_host,
+                     const GURL& url) override {
+    events_.push_back("DidFinishLoad");
+  }
+
+  void DidFailLoad(RenderFrameHost* render_frame_host,
+                   const GURL& url,
+                   int error_code,
+                   const base::string16& error_description) override {
+    events_.push_back("DidFailLoad");
+  }
+
+  void Wait() { run_loop_.Run(); }
+
+ private:
+  std::vector<std::string> events_;
+  base::RepeatingClosure completion_callback_;
+  base::RunLoop run_loop_;
+};
+
+}  // namespace
+
+// These tests provide a reference points for simulating the navigation events
+// for unittests.
+//
+// Keep in sync with TestRenderFrameHostTest.LoadingCallbacksOrder_*.
+IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
+                       LoadingCallbacksOrder_CrossDocumentNavigation) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+
+  LoadingObserver loading_observer(web_contents);
+
+  GURL url = embedded_test_server()->GetURL("a.com", "/title1.html");
+  EXPECT_TRUE(NavigateToURL(shell(), url));
+  loading_observer.Wait();
+
+  EXPECT_THAT(loading_observer.GetEvents(),
+              testing::ElementsAre(
+                  "DidStartLoading", "DidStartNavigation",
+                  "DidFinishNavigation", "DocumentAvailableInMainFrame",
+                  "DOMContentLoaded", "DocumentOnLoadCompletedInMainFrame",
+                  "DidFinishLoad", "DidStopLoading"));
+}
+
+IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
+                       LoadingCallbacksOrder_SameDocumentNavigation) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+
+  GURL url1 = embedded_test_server()->GetURL("a.com", "/title1.html");
+  GURL url2 = embedded_test_server()->GetURL("a.com", "/title1.html#foo");
+
+  LoadingObserver loading_observer1(web_contents);
+  EXPECT_TRUE(NavigateToURL(shell(), url1));
+  loading_observer1.Wait();
+
+  LoadingObserver loading_observer2(web_contents);
+  EXPECT_TRUE(NavigateToURL(shell(), url2));
+  loading_observer2.Wait();
+
+  EXPECT_THAT(loading_observer2.GetEvents(),
+              testing::ElementsAre("DidStartLoading", "DidStartNavigation",
+                                   "DidFinishNavigation", "DidStopLoading"));
+}
+
+IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
+                       LoadingCallbacksOrder_AbortedNavigation) {
+  const char kPageURL[] = "/controlled_page_load.html";
+  net::test_server::ControllableHttpResponse response(embedded_test_server(),
+                                                      kPageURL);
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL url = embedded_test_server()->GetURL("a.com", kPageURL);
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+
+  LoadingObserver loading_observer(web_contents);
+  shell()->LoadURL(url);
+  response.WaitForRequest();
+  response.Send(net::HttpStatusCode::HTTP_NO_CONTENT);
+  response.Done();
+
+  loading_observer.Wait();
+
+  EXPECT_THAT(loading_observer.GetEvents(),
+              testing::ElementsAre("DidStartLoading", "DidStartNavigation",
+                                   "DidFinishNavigation", "DidStopLoading"));
+}
+
+IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
+                       LoadingCallbacksOrder_ErrorPage) {
+  const char kPageURL[] = "/controlled_page_load.html";
+  net::test_server::ControllableHttpResponse response(embedded_test_server(),
+                                                      kPageURL);
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL url = embedded_test_server()->GetURL("a.com", kPageURL);
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+
+  LoadingObserver loading_observer(web_contents);
+  shell()->LoadURL(url);
+  response.WaitForRequest();
+  response.Send(net::HttpStatusCode::HTTP_REQUEST_TIMEOUT);
+  response.Done();
+
+  loading_observer.Wait();
+
+  EXPECT_THAT(loading_observer.GetEvents(),
+              testing::ElementsAre(
+                  "DidStartLoading", "DidStartNavigation",
+                  "DidFinishNavigation", "DocumentAvailableInMainFrame",
+                  "DOMContentLoaded", "DidFinishLoad", "DidStartNavigation",
+                  "DidFinishNavigation", "DocumentAvailableInMainFrame",
+                  "DOMContentLoaded", "DocumentOnLoadCompletedInMainFrame",
+                  "DidFinishLoad", "DidStopLoading"));
+}
+
 }  // namespace content

content/test/BUILD.gn

@@ -1963,6 +1963,7 @@ test("content_unittests") {
     "../test/renderer_audio_output_stream_factory_context_impl_unittest.cc",
     "navigation_simulator_unittest.cc",
     "proxy_service_mojo_unittest.cc",
+    "test_render_frame_host_unittest.cc",
     "url_request_context_builder_mojo_unittest.cc",
   ]
 

content/test/navigation_simulator_impl.cc

@@ -628,6 +628,8 @@ void NavigationSimulatorImpl::Commit() {
         FrameHostMsg_SwapOut_ACK(previous_rfh->GetRoutingID()));
   }
 
+  loading_scenario_ =
+      TestRenderFrameHost::LoadingScenario::NewDocumentNavigation;
   state_ = FINISHED;
   if (!keep_loading_)
     StopLoading();
@@ -801,6 +803,8 @@ void NavigationSimulatorImpl::CommitSameDocument() {
     state_ = FAILED;
     return;
   }
+  loading_scenario_ =
+      TestRenderFrameHost::LoadingScenario::kSameDocumentNavigation;
   state_ = FINISHED;
   if (!keep_loading_)
     StopLoading();
@@ -1341,7 +1345,7 @@ void NavigationSimulatorImpl::SetKeepLoading(bool keep_loading) {
 
 void NavigationSimulatorImpl::StopLoading() {
   CHECK(render_frame_host_);
-  render_frame_host_->SimulateLoadingCompleted();
+  render_frame_host_->SimulateLoadingCompleted(loading_scenario_);
 }
 
 void NavigationSimulatorImpl::FailLoading(

content/test/navigation_simulator_impl.h

@@ -16,6 +16,7 @@
 #include "content/public/browser/navigation_throttle.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/navigation_simulator.h"
+#include "content/test/test_render_frame_host.h"
 #include "mojo/public/cpp/bindings/pending_associated_receiver.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "net/base/host_port_pair.h"
@@ -274,6 +275,8 @@ class NavigationSimulatorImpl : public NavigationSimulator,
   bool was_initiated_by_link_click_ = false;
   bool browser_initiated_;
   bool same_document_ = false;
+  TestRenderFrameHost::LoadingScenario loading_scenario_ =
+      TestRenderFrameHost::LoadingScenario::kOther;
   blink::mojom::ReferrerPtr referrer_;
   ui::PageTransition transition_;
   ReloadType reload_type_ = ReloadType::NONE;

content/test/test_render_frame_host.cc

@@ -19,6 +19,7 @@
 #include "content/common/frame_owner_properties.h"
 #include "content/common/navigation_params.h"
 #include "content/common/navigation_params_utils.h"
+#include "content/common/view_messages.h"
 #include "content/public/browser/navigation_throttle.h"
 #include "content/public/common/navigation_policy.h"
 #include "content/public/common/url_constants.h"
@@ -693,7 +694,25 @@ TestRenderFrameHost::CreateStubBrowserInterfaceBrokerReceiver() {
       .InitWithNewPipeAndPassReceiver();
 }
 
-void TestRenderFrameHost::SimulateLoadingCompleted() {
+void TestRenderFrameHost::SimulateLoadingCompleted(
+    TestRenderFrameHost::LoadingScenario loading_scenario) {
+  if (!is_loading())
+    return;
+
+  if (loading_scenario == LoadingScenario::NewDocumentNavigation) {
+    static_cast<IPC::Listener*>(GetRenderViewHost())
+        ->OnMessageReceived(ViewHostMsg_DocumentAvailableInMainFrame(
+            GetRenderViewHost()->GetRoutingID(),
+            /* uses_temporary_zoom_level */ false));
+
+    OnMessageReceived(FrameHostMsg_DidFinishDocumentLoad(GetRoutingID()));
+
+    DocumentOnLoadCompleted();
+
+    OnMessageReceived(
+        FrameHostMsg_DidFinishLoad(GetRoutingID(), GetLastCommittedURL()));
+  }
+
   OnDidStopLoading();
 }
 

content/test/test_render_frame_host.h

@@ -207,9 +207,18 @@ class TestRenderFrameHost : public RenderFrameHostImpl,
     return navigation_requests_;
   }
 
+  enum class LoadingScenario {
+    NewDocumentNavigation,
+    kSameDocumentNavigation,
+
+    // TODO(altimin): Improve handling for the scenarios where navigation or
+    // page load have failed.
+    kOther
+  };
+
   // Simulates RenderFrameHost finishing loading and dispatching all relevant
   // callbacks.
-  void SimulateLoadingCompleted();
+  void SimulateLoadingCompleted(LoadingScenario loading_scenario);
 
  protected:
   void SendCommitNavigation(

content/test/test_render_frame_host_unittest.cc

+// Copyright 2019 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 "content/test/test_render_frame_host.h"
+
+#include <string>
+#include <vector>
+
+#include "content/public/browser/web_contents_observer.h"
+#include "content/test/navigation_simulator_impl.h"
+#include "content/test/test_render_view_host.h"
+#include "content/test/test_web_contents.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+
+class TestRenderFrameHostTest : public RenderViewHostImplTestHarness,
+                                public WebContentsObserver {
+ public:
+  void SetUp() override {
+    RenderViewHostImplTestHarness::SetUp();
+    contents()->GetMainFrame()->InitializeRenderFrameIfNeeded();
+    Observe(RenderViewHostImplTestHarness::web_contents());
+  }
+
+  std::vector<std::string>& Events() { return events_; }
+
+  void DidStartNavigation(NavigationHandle* navigation_handle) override {
+    events_.push_back("DidStartNavigation");
+  }
+
+  void DidFinishNavigation(NavigationHandle* navigation_handle) override {
+    events_.push_back("DidFinishNavigation");
+  }
+
+  void DidStartLoading() override { events_.push_back("DidStartLoading"); }
+
+  void DidStopLoading() override { events_.push_back("DidStopLoading"); }
+
+  void DocumentAvailableInMainFrame() override {
+    events_.push_back("DocumentAvailableInMainFrame");
+  }
+
+  void DocumentOnLoadCompletedInMainFrame() override {
+    events_.push_back("DocumentOnLoadCompletedInMainFrame");
+  }
+
+  void DOMContentLoaded(RenderFrameHost* render_frame_host) override {
+    events_.push_back("DOMContentLoaded");
+  }
+
+  void DidFinishLoad(RenderFrameHost* render_frame_host,
+                     const GURL& url) override {
+    events_.push_back("DidFinishLoad");
+  }
+
+  void DidFailLoad(RenderFrameHost* render_frame_host,
+                   const GURL& url,
+                   int error_code,
+                   const base::string16& error_description) override {
+    events_.push_back("DidFailLoad");
+  }
+
+ private:
+  std::vector<std::string> events_;
+};
+
+// These tests check that the loading events simulated by NavigationSimulator
+// together with TestRenderFrameHost::SimulateLoadingCompleted match
+// the real behaviour, captured by the browser tests.
+//
+// Keep in sync with WebContentsImplBrowserTest.LoadingCallbacksOrder_*.
+TEST_F(TestRenderFrameHostTest, LoadingCallbacksOrder_CrossDocument) {
+  std::unique_ptr<NavigationSimulator> simulator =
+      NavigationSimulator::CreateRendererInitiated(
+          GURL("https://example.test/"), main_rfh());
+  simulator->Start();
+  simulator->Commit();
+
+  EXPECT_THAT(Events(),
+              testing::ElementsAre(
+                  "DidStartLoading", "DidStartNavigation",
+                  "DidFinishNavigation", "DocumentAvailableInMainFrame",
+                  "DOMContentLoaded", "DocumentOnLoadCompletedInMainFrame",
+                  "DidFinishLoad", "DidStopLoading"));
+}
+
+TEST_F(TestRenderFrameHostTest, LoadingCallbacksOrder_SameDocument) {
+  std::unique_ptr<NavigationSimulator> simulator =
+      NavigationSimulator::CreateRendererInitiated(
+          GURL("https://example.test/"), main_rfh());
+  simulator->Start();
+  simulator->Commit();
+
+  Events().clear();
+
+  NavigationSimulator::CreateRendererInitiated(
+      GURL("https://example.test/#foo"), main_rfh())
+      ->CommitSameDocument();
+
+  EXPECT_THAT(Events(),
+              testing::ElementsAre("DidStartLoading", "DidStartNavigation",
+                                   "DidFinishNavigation", "DidStopLoading"));
+}
+
+}  // namespace content