Make ResourceScheduler work in OOPIF

content/browser/site_per_process_browsertest.cc

Index: content/browser/site_per_process_browsertest.cc
diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc
index 592bad29af1230313982fe8597fade3e59b2aa2e..cd7008f2b08551eee2510f03ec3fadc2c3f58bc1 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -10,9 +10,11 @@
 #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"
@@ -44,6 +46,7 @@
 #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"
@@ -64,6 +67,7 @@
 #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"
@@ -89,6 +93,24 @@ 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
@@ -552,8 +574,26 @@ class TestInterstitialDelegate : public InterstitialPageDelegate {
 // 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);
@@ -569,12 +609,40 @@ void SitePerProcessBrowserTest::SetUpCommandLine(
 #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
 //
@@ -9183,4 +9251,40 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
       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