Aggregate LoadState per-frame before copying to UI

Currently we copy hosts + other state for every outgoing request to the
UI thread before doing per-WebContents aggregation there. This can
involve a lot of copying of data around. This CL adds some initial
per-frame aggregation beforehand to reduce this.

Also, we add a browsertest for this functionality, since none previously
existed.

Bug: 813445
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I33438bdd5d4cab9caf3fcd07419fef3f7153935e
Reviewed-on: https://chromium-review.googlesource.com/951743Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542157}

content/browser/loader/resource_dispatcher_host_impl.cc

@@ -2354,27 +2354,45 @@ ResourceDispatcherHostImpl::PickMoreInterestingLoadInfos(
 }
 
 std::unique_ptr<ResourceDispatcherHostImpl::LoadInfoList>
-ResourceDispatcherHostImpl::GetLoadInfoForAllRoutes() {
-  std::unique_ptr<LoadInfoList> infos(new LoadInfoList);
-
+ResourceDispatcherHostImpl::GetInterestingPerFrameLoadInfos() {
+  auto infos = std::make_unique<LoadInfoList>();
+  std::map<GlobalFrameRoutingId, LoadInfo> frame_infos;
   for (const auto& loader : pending_loaders_) {
     net::URLRequest* request = loader.second->request();
     net::UploadProgress upload_progress = request->GetUploadProgress();
 
     LoadInfo load_info;
-    load_info.web_contents_getter =
-        loader.second->GetRequestInfo()->GetWebContentsGetterForRequest();
     load_info.host = request->url().host();
     load_info.load_state = request->GetLoadState();
     load_info.upload_size = upload_progress.size();
     load_info.upload_position = upload_progress.position();
+
+    ResourceRequestInfoImpl* request_info = loader.second->GetRequestInfo();
+    load_info.web_contents_getter =
+        request_info->GetWebContentsGetterForRequest();
+
+    // Navigation requests have frame_tree_node_ids, and may not have frame
+    // routing ids. Just include them unconditionally.
+    if (request_info->frame_tree_node_id() != -1) {
       infos->push_back(load_info);
+    } else {
+      GlobalFrameRoutingId id(request_info->GetChildID(),
+                              request_info->GetRenderFrameID());
+      auto existing = frame_infos.find(id);
+      if (existing == frame_infos.end() ||
+          LoadInfoIsMoreInteresting(load_info, existing->second)) {
+        frame_infos[id] = std::move(load_info);
+      }
     }
+  }
+
+  for (auto it : frame_infos)
+    infos->push_back(std::move(it.second));
   return infos;
 }
 
 void ResourceDispatcherHostImpl::UpdateLoadInfo() {
-  std::unique_ptr<LoadInfoList> infos(GetLoadInfoForAllRoutes());
+  std::unique_ptr<LoadInfoList> infos(GetInterestingPerFrameLoadInfos());
 
   // Stop the timer if there are no more pending requests. Future new requests
   // will restart it as necessary.

content/browser/loader/resource_dispatcher_host_impl.h

@@ -359,6 +359,7 @@ 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;
@@ -521,8 +522,14 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
   static std::unique_ptr<LoadInfoMap> PickMoreInterestingLoadInfos(
       std::unique_ptr<LoadInfoList> infos);
 
-  // Gets all the LoadInfos for each pending request.
-  std::unique_ptr<LoadInfoList> GetLoadInfoForAllRoutes();
+  // Gets the most interesting LoadInfos for each GlobalFrameRoutingIds.
+  // Includes the LoadInfo for all navigation requests, which may not have valid
+  // frame ids.
+  //
+  // We aggregate per-frame on the IO thread, and per-WebContents on the UI
+  // thread. The IO thread aggregation is used to avoid copying state for every
+  // request across threads.
+  std::unique_ptr<LoadInfoList> GetInterestingPerFrameLoadInfos();
 
   // Checks all pending requests and updates the load info if necessary.
   void UpdateLoadInfo();

content/browser/web_contents/web_contents_impl_browsertest.cc

@@ -12,6 +12,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "build/build_config.h"
+#include "components/url_formatter/url_formatter.h"
 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
@@ -41,6 +42,7 @@
 #include "content/shell/browser/shell.h"
 #include "content/test/content_browser_test_utils_internal.h"
 #include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/controllable_http_response.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "url/gurl.h"
@@ -75,6 +77,8 @@ 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");
@@ -1687,4 +1691,113 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, UpdateTargetURL) {
             target_url_waiter.WaitForUpdatedTargetURL());
 }
 
+IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, UpdateLoadState) {
+  // 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 =
+      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());
+
+  // 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());
+
+  EXPECT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL(
+                   "a.com", "/cross_site_iframe_factory.html?a(b(c))")));
+  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);
+
+  // Start loading the respective resources in each frame.
+  auto load_resource = [](FrameTreeNode* frame, const std::string url) {
+    std::string partial_script = R"(
+      var img = new Image();
+      img.src = '%s';
+      document.body.appendChild(img);
+    )";
+    std::string script =
+        base::StringPrintf(partial_script.c_str(), 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());
+
+  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.
+  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());
+}
+
 }  // namespace content

content/test/BUILD.gn

@@ -870,6 +870,7 @@ test("content_browsertests") {
     "//components/discardable_memory/service",
     "//components/network_session_configurator/common",
     "//components/payments/mojom",
+    "//components/url_formatter:url_formatter",
     "//components/viz/test:test_support",
     "//content:resources",
     "//content/app:both_for_content_tests",

testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

@@ -113,5 +113,9 @@
 # Flakes https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.linux%2FLinux_Tests__dbg__1_%2F69105%2F%2B%2Frecipes%2Fsteps%2Fnetwork_service_content_browsertests%2F0%2Flogs%2FWebContentsImplBrowserTest.DownloadImage_Allow_FileImage%2F0
 -WebContentsImplBrowserTest.DownloadImage_Allow_FileImage
 
+# Network service path needs some way to update the per-WebContents load state.
+# http://crbug.com/819663.
+-WebContentsImplBrowserTest.UpdateLoadState
+
 # https://crbug.com/808759
 -DownloadContentTest.DownloadAttributeBlobURL