Make ResourceScheduler work in OOPIF

content/browser/site_per_process_browsertest.cc

 // Copyright (c) 2012 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/browser/site_per_process_browsertest.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/path_service.h"
 #include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/pattern.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
@@ skipping 11 matching lines @@
 #include "content/browser/renderer_host/render_widget_host_input_event_router.h"
 #include "content/browser/renderer_host/render_widget_host_view_aura.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/child_process_messages.h"
 #include "content/common/frame_messages.h"
 #include "content/common/input/synthetic_tap_gesture_params.h"
 #include "content/common/input/touch_action.h"
 #include "content/common/input_messages.h"
 #include "content/common/renderer.mojom.h"
 #include "content/common/view_messages.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/interstitial_page_delegate.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/resource_dispatcher_host.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_frame_navigation_observer.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "content/test/content_browser_test_utils_internal.h"
 #include "ipc/ipc.mojom.h"
 #include "ipc/ipc_security_test_util.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/http_request.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/public/platform/WebInputEvent.h"
 #include "third_party/WebKit/public/platform/WebInsecureRequestPolicy.h"
 #include "third_party/WebKit/public/web/WebSandboxFlags.h"
 #include "ui/display/display_switches.h"
 #include "ui/display/screen.h"
 #include "ui/events/base_event_utils.h"
 #include "ui/events/event.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/latency_info.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/native_theme/native_theme_switches.h"
 
 #if defined(USE_AURA)
 #include "content/browser/renderer_host/render_widget_host_view_aura.h"
 #endif
 
 #if defined(OS_MACOSX)
 #include "ui/base/test/scoped_preferred_scroller_style_mac.h"
 #endif
 
 namespace content {
 
 namespace {
 
+// This class offloads a "complete" closure to the underlying test fixture,
+// which can call it at any time to finish a request.
+class DelayedResponse : public net::test_server::BasicHttpResponse {
+ public:
+  DelayedResponse(SitePerProcessBrowserTest* test_harness)
+      : test_harness_(test_harness) {}
+  void SendResponse(
+      const net::test_server::SendBytesCallback& send,
+      const net::test_server::SendCompleteCallback& done) override {
+    test_harness_->AddDelayedResponse(send, done);
+  }
+
+ private:
+  SitePerProcessBrowserTest* test_harness_;
+
+  DISALLOW_COPY_AND_ASSIGN(DelayedResponse);
+};
+
 // Helper function to send a postMessage and wait for a reply message.  The
 // |post_message_script| is executed on the |sender_ftn| frame, and the sender
 // frame is expected to post |reply_status| from the DOMAutomationController
 // when it receives a reply.
 void PostMessageAndWaitForReply(FrameTreeNode* sender_ftn,
                                 const std::string& post_message_script,
                                 const std::string& reply_status) {
   // Subtle: msg_queue needs to be declared before the ExecuteScript below, or
   // else it might miss the message of interest.  See https://crbug.com/518729.
   DOMMessageQueue msg_queue;
@@ skipping 443 matching lines @@
   // InterstitialPageDelegate:
   std::string GetHTMLContents() override { return "<p>Interstitial</p>"; }
 };
 
 }  // namespace
 
 //
 // SitePerProcessBrowserTest
 //
 
-SitePerProcessBrowserTest::SitePerProcessBrowserTest() {
-};
+SitePerProcessBrowserTest::SitePerProcessBrowserTest(){};
+SitePerProcessBrowserTest::~SitePerProcessBrowserTest(){};
+
+void SitePerProcessBrowserTest::AddDelayedResponse(
+    const net::test_server::SendBytesCallback& send,
+    const net::test_server::SendCompleteCallback& done) {
+  send_response_closures_.push_back(base::Bind(send, "", done));

[mmenke, 2017/01/30 23:03:51]

I think you should document what "base::Bind(send, "", done)" does.  It's not
exactly clear that it creates a closure that just closes the socket without
sending a response.

[Charlie Harrison, 2017/01/31 20:19:31]

Done.

+}
+
+void SitePerProcessBrowserTest::MaybeStartRequests() {
+  int sum = 0;
+  for (auto it : delayed_requests_for_path_) {

[mmenke, 2017/01/30 23:03:51]

delayed_requests_for_path_ seems a bit confusing. 
"num_remaining_requests_to_delay_for_path_"?

[Charlie Harrison, 2017/01/31 20:19:31]

Done.

+    sum += it.second;

[mmenke, 2017/01/30 23:03:51]

if (it.second > 0)
  return;

[Charlie Harrison, 2017/01/31 20:19:32]

Done.

+  }
+  if (sum == 0) {
+    for (const auto it : send_response_closures_) {
+      base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, it);

[mmenke, 2017/01/30 23:03:51]

This isn't threadsafe.  DelayedResponse is called on the TestServer's own
thread, not the UI thread.  So you need to post a task over to the UI thread to
add stuff to send_response_closures_, and then to resume, you need to post a
task back over to the IO thread.

[Charlie Harrison, 2017/01/31 20:19:31]

I thought this would be okay because send_response_closures_ is only ever
accessed or modified on the test servers own thread.

+    }
+  }
+}
 
 std::string SitePerProcessBrowserTest::DepictFrameTree(FrameTreeNode* node) {
   return visualizer_.DepictFrameTree(node);
 }
 
 void SitePerProcessBrowserTest::SetUpCommandLine(
     base::CommandLine* command_line) {
   IsolateAllSitesForTesting(command_line);
 #if !defined(OS_ANDROID)
   // TODO(bokan): Needed for scrollability check in
   // FrameOwnerPropertiesPropagationScrolling. crbug.com/662196.
   command_line->AppendSwitch(switches::kDisableOverlayScrollbar);
 #endif
 };
 
+std::unique_ptr<net::test_server::HttpResponse>
+SitePerProcessBrowserTest::HandleMockResource(

[mmenke, 2017/01/30 23:03:51]

Think this needs documentation.

[Charlie Harrison, 2017/01/31 20:19:32]

Done.

+    const net::test_server::HttpRequest& request) {
+  auto it = delayed_requests_for_path_.find(request.GetURL().path());
+  if (it == delayed_requests_for_path_.end())
+    return nullptr;
+
+  // If there are requests to delay for this path, make a delayed request which
+  // will be finished later. Otherwise fall through to the bottom and send an
+  // empty response.
+  if (it->second) {
+    --it->second;
+    return base::MakeUnique<DelayedResponse>(this);
+  }
+  MaybeStartRequests();
+  return std::unique_ptr<net::test_server::BasicHttpResponse>();
+}
+
 void SitePerProcessBrowserTest::SetUpOnMainThread() {
   host_resolver()->AddRule("*", "127.0.0.1");
   SetupCrossSiteRedirector(embedded_test_server());
+  // Allows for arbitrary "barrier" style delaying of requests matching certain
+  // paths.
+  embedded_test_server()->RegisterRequestHandler(base::Bind(
+      &SitePerProcessBrowserTest::HandleMockResource, base::Unretained(this)));
   ASSERT_TRUE(embedded_test_server()->Start());
 }
 
+void SitePerProcessBrowserTest::SetDelayedRequestsForPath(std::string path,

[mmenke, 2017/01/30 23:03:51]

nit:  const std::string& path?

[mmenke, 2017/01/30 23:03:51]

Think this needs documentation.

[Charlie Harrison, 2017/01/31 20:19:31]

Added docs to the header.

[Charlie Harrison, 2017/01/31 20:19:31]

Done.

+                                                          int num_delayed) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  delayed_requests_for_path_[path] = num_delayed;
+}
+
 //
 // SitePerProcessHighDPIBrowserTest
 //
 
 
 class SitePerProcessHighDPIBrowserTest : public SitePerProcessBrowserTest {
  public:
   const double kDeviceScaleFactor = 2.0;
 
   SitePerProcessHighDPIBrowserTest() {}
@@ skipping 8588 matching lines @@
   EXPECT_TRUE(NavigateToURL(
       shell(), embedded_test_server()->GetURL("b.com", "/title3.html")));
 
   // Pretend that a.com just requested a context menu. This used to cause a
   // because the RenderWidgetHostView is destroyed when the frame is swapped and
   // added to pending delete list.
   rfh->OnMessageReceived(
       FrameHostMsg_ContextMenu(rfh->GetRoutingID(), ContextMenuParams()));
 }
 
+// Regression tests for crbug.com/678206, where the request throttling in

[nasko, 2017/01/31 15:50:34]

nit: https://

[Charlie Harrison, 2017/01/31 20:19:32]

Done.

+// ResourceScheduler was not updated for OOPIFs. This resulted in a single hung
+// delayable request (e.g. video) starving all other delayable requests.

[mmenke, 2017/01/30 23:03:51]

Think this needs more details on how the tests actually work.

[Charlie Harrison, 2017/01/31 20:19:32]

Done.

+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+                       DelayableSubframeRequestsOneFrame) {
+  std::string path = "/mock-video.mp4";
+  SetDelayedRequestsForPath(path, 2);

[mmenke, 2017/01/30 23:03:51]

Not safe to do this after setting up the test server, since the test server runs
on its own thread.

[Charlie Harrison, 2017/01/31 20:19:32]

Ensured that the test server is not started until after this is called.

+  GURL url(embedded_test_server()->GetURL(
+      "a.com", base::StringPrintf("/site_isolation/"
+                                  "subframes_with_resources.html?urls=%s&"
+                                  "numSubresources=3",
+                                  path.c_str())));
+  EXPECT_TRUE(NavigateToURL(shell(), url));
+  bool result;
+  EXPECT_TRUE(ExecuteScriptAndExtractBool(shell(), "createFrames()", &result));
+  EXPECT_TRUE(result);
+
+}
+
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+                       DelayableSubframeRequestsTwoFrames) {
+  std::string path0 = "/mock-video0.mp4";
+  std::string path1 = "/mock-video1.mp4";
+  SetDelayedRequestsForPath(path0, 2);
+  SetDelayedRequestsForPath(path1, 2);
+  GURL url(embedded_test_server()->GetURL(
+      "a.com", base::StringPrintf("/site_isolation/"
+                                  "subframes_with_resources.html?urls=%s,%s&"
+                                  "numSubresources=3",
+                                  path0.c_str(), path1.c_str())));
+  EXPECT_TRUE(NavigateToURL(shell(), url));
+  bool result;
+  EXPECT_TRUE(ExecuteScriptAndExtractBool(shell(), "createFrames()", &result));
+  EXPECT_TRUE(result);

[nasko, 2017/01/31 15:50:34]

How do those tests manifest when they fail? Timeout? Crash? Maybe add this in
the comment description of the tests cases.

[Charlie Harrison, 2017/01/31 20:19:31]

They will time out, I've updated the description.

+}
+
 }  // namespace content