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..5c0e3dc67ff1b9847dcbdac38cdbfed2024b5905 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -8,11 +8,14 @@
 #include <stdint.h>
 
 #include <algorithm>
+#include <map>
 #include <vector>
 
+#include "base/bind.h"
+#include "base/callback.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 +47,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 +68,8 @@
 #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 "net/test/embedded_test_server/http_response.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/public/platform/WebInputEvent.h"
 #include "third_party/WebKit/public/platform/WebInsecureRequestPolicy.h"
@@ -9183,4 +9189,155 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
       FrameHostMsg_ContextMenu(rfh->GetRoutingID(), ContextMenuParams()));
 }
 
+// Test harness that allows for "barrier" style delaying of requests matching
+// certain paths. Call SetDelayedRequestsForPath to delay requests, then
+// SetUpEmbeddedTestServer to register handlers and start the server.
+class RequestDelayingSitePerProcessBrowserTest
+    : public SitePerProcessBrowserTest {
+ public:
+  RequestDelayingSitePerProcessBrowserTest()
+      : test_server_(base::MakeUnique<net::EmbeddedTestServer>()) {}
+
+  void AddDelayedResponse(const net::test_server::SendBytesCallback& send,
+                          const net::test_server::SendCompleteCallback& done) {

[mmenke, 2017/02/01 20:21:40]

Comment that this is called on the test server's thread?

[Charlie Harrison, 2017/02/01 22:22:33]

Done.

[Charlie Harrison, 2017/02/01 22:22:34]

Done.

+    // Just create a closure that closes the socket without sending a response.
+    // This will propagate an error to the underlying request.
+    send_response_closures_.push_back(base::Bind(send, "", done));
+  }

[mmenke, 2017/02/01 20:21:40]

This can be private (Inner classes can access even private class members of
outer classes, though outer classes only have public access to their inner
classes)

[Charlie Harrison, 2017/02/01 22:22:34]

Done.

+
+  // Must be called after any calls to SetDelayedRequestsForPath.
+  void SetUpEmbeddedTestServer() {
+    host_resolver()->AddRule("*", "127.0.0.1");
+    SetupCrossSiteRedirector(test_server_.get());
+    test_server_->RegisterRequestHandler(base::Bind(
+        &RequestDelayingSitePerProcessBrowserTest::HandleMockResource,
+        base::Unretained(this)));
+    ASSERT_TRUE(test_server_->Start());
+  }
+
+ protected:

[mmenke, 2017/02/01 20:21:40]

no need for protected - it doesn't really do anything.  Nothing has access to
this class in a manner where protected behaves differently from public.

[Charlie Harrison, 2017/02/01 22:22:35]

Done.

+  // Delays |num_delayed| requests with URLs whose path parts match |path|. When
+  // the |num_delayed| + 1 request matching the path comes in, the rest are
+  // unblocked.
+  // Note: must be called on the UI thread before |test_server_| is started.
+  void SetDelayedRequestsForPath(const std::string& path, int num_delayed) {

[mmenke, 2017/02/01 20:21:40]

DCHECK that the test_server isn't started?

[Charlie Harrison, 2017/02/01 22:22:35]

Done.

+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    num_remaining_requests_to_delay_for_path_[path] = num_delayed;
+  }
+
+ private:
+  // Custom embedded test server handler. Looks for requests matching
+  // num_remaining_requests_to_delay_for_path_, and delays them if necessary. As
+  // soon as a single request comes in and:
+  // 1) It matches a delayed path
+  // 2) No path has any more requests to delay
+  // Then we release the barrier and finish all delayed requests.
+  std::unique_ptr<net::test_server::HttpResponse> HandleMockResource(
+      const net::test_server::HttpRequest& request) {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);

[mmenke, 2017/02/01 20:21:40]

This DCHECK passes?  The TestServer uses its own thread, not the IO thread - it
can't even depend on BrowserThread.  I'm very confused.

[Charlie Harrison, 2017/02/01 22:22:34]

Me too, I am investigating why. For now I removed it.

+    auto it =
+        num_remaining_requests_to_delay_for_path_.find(request.GetURL().path());
+    if (it == num_remaining_requests_to_delay_for_path_.end())
+      return nullptr;
+
+    // If there are requests to delay for this path, make a delayed request
+    // which

[mmenke, 2017/02/01 20:21:40]

nit:  reflow comment.

[Charlie Harrison, 2017/02/01 22:22:34]

Done.

+    // will be finished later. Otherwise fall through to the bottom and send an
+    // empty response.
+    if (it->second) {

[mmenke, 2017/02/01 20:21:40]

nit:  > 0, to make it clear this is an int we're looking at, and not a pointer..

[Charlie Harrison, 2017/02/01 22:22:34]

Done.

+      --it->second;
+      return base::MakeUnique<DelayedResponse>(this);
+    }
+    MaybeStartRequests();
+    return std::unique_ptr<net::test_server::BasicHttpResponse>();
+  }
+
+  // If there are no more requests to delay, post a series of tasks finishing
+  // all the delayed tasks.

[mmenke, 2017/02/01 20:21:40]

Again, mention thread this is called on.

[Charlie Harrison, 2017/02/01 22:22:35]

Done.

+  void MaybeStartRequests() {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    for (auto it : num_remaining_requests_to_delay_for_path_) {
+      if (it.second > 0)
+        return;
+    }
+    for (const auto it : send_response_closures_) {
+      base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, it);

[mmenke, 2017/02/01 20:21:40]

Why does this need to use PostTask?

[Charlie Harrison, 2017/02/01 22:22:35]

It doesn't need to.

+    }
+  }
+
+  // This class offloads a "complete" closure to the underlying test fixture,
+  // which can call it at any time to finish a request.

[mmenke, 2017/02/01 20:21:40]

I'm having trouble following this comment.

Maybe:  "This class passes the callbacks needed to respond to a request to the
underlying test fixture."

[Charlie Harrison, 2017/02/01 22:22:34]

Much better. Changed.

+  class DelayedResponse : public net::test_server::BasicHttpResponse {
+   public:
+    DelayedResponse(RequestDelayingSitePerProcessBrowserTest* test_harness)

[mmenke, 2017/02/01 20:21:40]

explicit

[Charlie Harrison, 2017/02/01 22:22:34]

Done.

+        : 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:
+    RequestDelayingSitePerProcessBrowserTest* test_harness_;
+
+    DISALLOW_COPY_AND_ASSIGN(DelayedResponse);
+  };
+
+  // Set of closures to call which will complete delayed requests. May only be
+  // modified on the test_server_'s thread.
+  std::vector<base::Closure> send_response_closures_;
+
+  // Map from URL paths to the number of requests to delay for that particular
+  // path. Initialized on the UI thread but modified and read on the IO thread
+  // after the |test_server_| is started.
+  std::map<std::string, int> num_remaining_requests_to_delay_for_path_;

[mmenke, 2017/02/01 20:21:41]

include <string>

[Charlie Harrison, 2017/02/01 22:22:34]

<string> is in the header.

+
+  // Don't use embedded_test_server() because this one requires custom
+  // initialization.
+  std::unique_ptr<net::EmbeddedTestServer> test_server_;
+};
+
+// Regression tests for https://crbug.com/678206, where the request throttling
+// in ResourceScheduler was not updated for OOPIFs. This resulted in a single
+// hung delayable request (e.g. video) starving all other delayable requests.
+// The tests work by delaying N requests in a cross-domain iframe. Once the n +

[mmenke, 2017/02/01 20:21:40]

nit:  This paragraph uses N here and n below.

[Charlie Harrison, 2017/02/01 22:22:34]

Changed to "n".

+// 1 request goes through to the network stack (ensuring it was not starved),
+// the delayed request completed.
+//
+// If the logic is not correct, these tests will time out, as the n + 1 request

[mmenke, 2017/02/01 20:21:40]

nit:  n + 1 request seems a bit confusing - it could mean the n+1th (n+1st?)
request, or n + 1 requests.  n + 1th?  (n + 1)th?  request n + 1?  Same as
above.

[Charlie Harrison, 2017/02/01 22:22:34]

Changed to n + 1st.

+// will never start.
+IN_PROC_BROWSER_TEST_F(RequestDelayingSitePerProcessBrowserTest,
+                       DelayableSubframeRequestsOneFrame) {
+  std::string path = "/mock-video.mp4";
+  SetDelayedRequestsForPath(path, 2);
+  SetUpEmbeddedTestServer();
+  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(RequestDelayingSitePerProcessBrowserTest,
+                       DelayableSubframeRequestsTwoFrames) {
+  std::string path0 = "/mock-video0.mp4";
+  std::string path1 = "/mock-video1.mp4";
+  SetDelayedRequestsForPath(path0, 2);
+  SetDelayedRequestsForPath(path1, 2);
+  SetUpEmbeddedTestServer();
+  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);
+}
+
 }  // namespace content