Beef up and de-flake UpdateLoadState test

This CL changes the test to:
 - Use the default timer to push load state updates, which should
   deflake the test. This is accomplished using a WebContentsDelegate.
 - Tests the actual LoadState, not just the host.

The test was currently flaking due to the update + ACK refactor which
wasn't compatible with the test.

Bug: 830991
Change-Id: I4d788e7a7388617af39f0981bd84fc7d1da6a87c
Reviewed-on: https://chromium-review.googlesource.com/1009287Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551084}

content/browser/loader/resource_dispatcher_host_impl.cc

@@ -2360,6 +2360,7 @@ void ResourceDispatcherHostImpl::UpdateLoadInfo() {
   // their render frame routing IDs yet (which is what we have for subresource
   // requests), we must go to the UI thread and compare the requests using their
   // WebContents.
+  DCHECK(!waiting_on_load_state_ack_);
   waiting_on_load_state_ack_ = true;
   main_thread_task_runner_->PostTaskAndReply(
       FROM_HERE,

content/browser/loader/resource_dispatcher_host_impl.h

@@ -360,7 +360,6 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
   FRIEND_TEST_ALL_PREFIXES(ResourceDispatcherHostTest, LoadInfoSamePriority);
   FRIEND_TEST_ALL_PREFIXES(ResourceDispatcherHostTest, LoadInfoUploadProgress);
   FRIEND_TEST_ALL_PREFIXES(ResourceDispatcherHostTest, LoadInfoTwoRenderViews);
-  FRIEND_TEST_ALL_PREFIXES(WebContentsImplBrowserTest, UpdateLoadState);
 
   struct OustandingRequestsStats {
     int memory_cost;

content/browser/web_contents/web_contents_impl_browsertest.cc

@@ -24,6 +24,7 @@
 #include "content/browser/web_contents/web_contents_view.h"
 #include "content/common/frame_messages.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/invalidate_type.h"
 #include "content/public/browser/javascript_dialog_manager.h"
 #include "content/public/browser/load_notification_details.h"
 #include "content/public/browser/navigation_controller.h"
@@ -88,8 +89,6 @@ class WebContentsImplBrowserTest : public ContentBrowserTest {
   }
 
   void SetUpOnMainThread() override {
-    host_resolver()->AddRuleWithLatency("slow.com", "127.0.0.1",
-                                        1000 * 60 * 60 /* ms */);
     // Setup the server to allow serving separate sites, so we can perform
     // cross-process navigation.
     host_resolver()->AddRule("*", "127.0.0.1");
@@ -2046,116 +2045,140 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, UpdateTargetURL) {
             target_url_waiter.WaitForUpdatedTargetURL());
 }
 
-// TODO(mmenke):  Beef up testing of LoadState a little. In particular, check
-// LoadState itself, not just the host name, check upload progress, check the
-// param, and make sure RDH pushes the data to the browser process.
+namespace {
+
+class LoadStateWaiter : public WebContentsDelegate {
+ public:
+  explicit LoadStateWaiter(content::WebContents* contents)
+      : web_contents_(contents) {
+    contents->SetDelegate(this);
+  }
+  ~LoadStateWaiter() override = default;
+
+  // Waits until the WebContents changes its LoadStateHost to |host|.
+  void Wait(net::LoadState load_state, const base::string16& host) {
+    waiting_host_ = host;
+    waiting_state_ = load_state;
+    if (!LoadStateMatches(web_contents_)) {
+      base::RunLoop run_loop;
+      quit_closure_ = run_loop.QuitClosure();
+      run_loop.Run();
+      DCHECK(LoadStateMatches(web_contents_));
+    }
+  }
+
+  // WebContentsDelegate:
+  void NavigationStateChanged(WebContents* source,
+                              InvalidateTypes changed_flags) override {
+    if (!quit_closure_)
+      return;
+    if (!(changed_flags & INVALIDATE_TYPE_LOAD))
+      return;
+    if (LoadStateMatches(source))
+      std::move(quit_closure_).Run();
+  }
+
+ private:
+  bool LoadStateMatches(content::WebContents* contents) {
+    DCHECK(contents == web_contents_);
+    return waiting_host_ == contents->GetLoadStateHost() &&
+           waiting_state_ == contents->GetLoadState().state;
+  }
+  base::OnceClosure quit_closure_;
+  content::WebContents* web_contents_ = nullptr;
+  base::string16 waiting_host_;
+  net::LoadState waiting_state_;
+
+  DISALLOW_COPY_AND_ASSIGN(LoadStateWaiter);
+};
+
+}  // namespace
+
+// TODO(csharrison,mmenke):  Beef up testing of LoadState a little. In
+// particular, check upload progress and check the LoadState param.
 IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, UpdateLoadState) {
+  base::string16 a_host = url_formatter::IDNToUnicode("a.com");
+  base::string16 b_host = url_formatter::IDNToUnicode("b.com");
+  base::string16 paused_host = url_formatter::IDNToUnicode("paused.com");
+
   // Controlled responses for image requests made in the test. They will
   // alternate being the "most interesting" for the purposes of notifying the
   // WebContents.
   auto a_response =
       std::make_unique<net::test_server::ControllableHttpResponse>(
           embedded_test_server(), "/a_img");
-  auto slow_response =
+  auto b_response =
       std::make_unique<net::test_server::ControllableHttpResponse>(
-          embedded_test_server(), "/slow_img");
-  auto c_response =
-      std::make_unique<net::test_server::ControllableHttpResponse>(
-          embedded_test_server(), "/c_img");
-  ASSERT_TRUE(embedded_test_server()->Start());
+          embedded_test_server(), "/b_img");
 
-  // This is a hack to ensure that the resource scheduler has at least one
-  // loading client for the duration of the test. Could alternatively delay some
-  // subresources on the main target page, but it would require care to ensure
-  // *all* other resources are completed before the test properly gets started.
-  Shell* popup = CreateBrowser();
-  const GURL kPopupUrl(embedded_test_server()->GetURL("/title1.html"));
-  TestNavigationManager popup_delayer(popup->web_contents(), kPopupUrl);
-  popup->LoadURL(kPopupUrl);
-  EXPECT_TRUE(popup_delayer.WaitForResponse());
+  LoadStateWaiter waiter(shell()->web_contents());
+  ASSERT_TRUE(embedded_test_server()->Start());
 
   EXPECT_TRUE(NavigateToURL(
       shell(), embedded_test_server()->GetURL(
-                   "a.com", "/cross_site_iframe_factory.html?a(b(c))")));
+                   "a.com", "/cross_site_iframe_factory.html?a(b)")));
   WebContentsImpl* web_contents =
       static_cast<WebContentsImpl*>(shell()->web_contents());
   FrameTreeNode* a_frame = web_contents->GetFrameTree()->root();
-  FrameTreeNode* slow_frame = a_frame->child_at(0);
-  FrameTreeNode* c_frame = slow_frame->child_at(0);
+  FrameTreeNode* b_frame = a_frame->child_at(0);
 
   // Start loading the respective resources in each frame.
   auto load_resource = [](FrameTreeNode* frame, const std::string url) {
-    std::string partial_script = R"(
+    const char kLoadResourceScript[] = R"(
       var img = new Image();
       img.src = '%s';
       document.body.appendChild(img);
     )";
-    std::string script =
-        base::StringPrintf(partial_script.c_str(), url.c_str());
+    std::string script = base::StringPrintf(kLoadResourceScript, url.c_str());
     EXPECT_TRUE(ExecuteScript(frame, script));
   };
 
-  // Blocks until the img element in |frame| finishes.
-  auto wait_for_img_finished = [](FrameTreeNode* frame) {
-    bool finished = false;
-    std::string script = R"(
-      var img = document.getElementsByTagName('img')[0];
-      if (img.complete)
-        window.domAutomationController.send(true);
-      else
-        img.onload = img.onerror = window.domAutomationController.send(true);
-    )";
-    EXPECT_TRUE(ExecuteScriptAndExtractBool(frame, script.c_str(), &finished));
-  };
-
-  // Requests a load state notification from the RDHI and waits until the update
-  // is posted back on the UI thread. Due to PostTaskAndReply, relies on
-  // UpdateLoadInfo synchronously posting a task to the WebContents.
-  auto update_load_state_and_wait = []() {
-    base::RunLoop run_loop;
-    BrowserThread::PostTaskAndReply(
-        BrowserThread::IO, FROM_HERE,
-        base::BindOnce(&ResourceDispatcherHostImpl::UpdateLoadInfo,
-                       base::Unretained(ResourceDispatcherHostImpl::Get())),
-        run_loop.QuitClosure());
-    run_loop.Run();
-  };
-
   // There should be no outgoing requests, so the load state should be empty.
-  update_load_state_and_wait();
-  EXPECT_TRUE(web_contents->GetLoadStateHost().empty());
-  EXPECT_EQ(url_formatter::IDNToUnicode(kPopupUrl.host()),
-            popup->web_contents()->GetLoadStateHost());
+  waiter.Wait(net::LOAD_STATE_IDLE, base::string16());
+
+  // The |frame_pauser| pauses the navigation after every step. It will only
+  // finish by calling WaitForNavigationFinished or ResumeNavigation.
+  GURL paused_url(embedded_test_server()->GetURL("paused.com", "/title1.html"));
+  TestNavigationManager frame_pauser(web_contents, paused_url);
+  const char kLoadFrameScript[] = R"(
+    var frame = document.createElement('iframe');
+    frame.src = "%s";
+    document.body.appendChild(frame);
+  )";
+  EXPECT_TRUE(ExecuteScript(
+      web_contents,
+      base::StringPrintf(kLoadFrameScript, paused_url.spec().c_str())));
+
+  // Wait for the response to be ready, but never finish it.
+  EXPECT_TRUE(frame_pauser.WaitForResponse());
+  EXPECT_FALSE(frame_pauser.was_successful());
+  waiter.Wait(net::LOAD_STATE_WAITING_FOR_DELEGATE, paused_host);
 
   load_resource(a_frame, "/a_img");
   a_response->WaitForRequest();
-  update_load_state_and_wait();
-  EXPECT_EQ(url_formatter::IDNToUnicode("a.com"),
-            web_contents->GetLoadStateHost());
-
-  // slow_img should never get past DNS resolution for the remainder of the
-  // test. Ensure that a_img is further along (and therefore more interesting).
-  load_resource(slow_frame, "http://slow.com/slow_img");
-  update_load_state_and_wait();
-  EXPECT_EQ(url_formatter::IDNToUnicode("a.com"),
-            web_contents->GetLoadStateHost());
-
-  // Finish a_img and start c_img, ensure it passes slow_img.
+  waiter.Wait(net::LOAD_STATE_WAITING_FOR_RESPONSE, a_host);
+
+  // Start loading b_img and have it pass a_img by providing one byte of data.
+  load_resource(b_frame, "/b_img");
+  b_response->WaitForRequest();
+
+  const char kPartialResponse[] = "HTTP/1.1 200 OK\r\n\r\nx";
+  b_response->Send(kPartialResponse);
+  waiter.Wait(net::LOAD_STATE_READING_RESPONSE, b_host);
+
+  // Finish b_img and expect that a_img is back to being most interesting.
+  b_response->Done();
+  waiter.Wait(net::LOAD_STATE_WAITING_FOR_RESPONSE, a_host);
+
+  // Advance and finish a_img.
+  a_response->Send(kPartialResponse);
+  waiter.Wait(net::LOAD_STATE_READING_RESPONSE, a_host);
   a_response->Done();
-  load_resource(c_frame, "/c_img");
-  wait_for_img_finished(a_frame);
-  c_response->WaitForRequest();
-  update_load_state_and_wait();
-  EXPECT_EQ(url_formatter::IDNToUnicode("c.com"),
-            web_contents->GetLoadStateHost());
-
-  // Finish c_img and ensure slow_img (the last outgoing request) is the most
-  // interesting.
-  c_response->Done();
-  wait_for_img_finished(c_frame);
-  update_load_state_and_wait();
-  EXPECT_EQ(url_formatter::IDNToUnicode("slow.com"),
-            web_contents->GetLoadStateHost());
+
+  // Now the only request in flight should be the delayed frame.
+  waiter.Wait(net::LOAD_STATE_WAITING_FOR_DELEGATE, paused_host);
+  frame_pauser.ResumeNavigation();
+  waiter.Wait(net::LOAD_STATE_IDLE, base::string16());
 }
 
 // Disabled due to flakes on Linux. https://crbug.com/832191