Use mojo for Chrome Loading, Part 1

content/browser/loader/url_loader_factory_holder_unittest.cc

Index: content/browser/loader/url_loader_factory_holder_unittest.cc
diff --git a/content/browser/loader/url_loader_factory_holder_unittest.cc b/content/browser/loader/url_loader_factory_holder_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1e7c0e56f05db02c207607b0660ef75251684c2a
--- /dev/null
+++ b/content/browser/loader/url_loader_factory_holder_unittest.cc
@@ -0,0 +1,410 @@
+// Copyright 2016 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/loader/url_loader_factory_holder.h"
+
+#include <memory>
+#include <string>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/location.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/path_service.h"
+#include "base/run_loop.h"
+#include "content/browser/loader/mojo_async_resource_handler.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
+#include "content/browser/loader/resource_message_filter.h"
+#include "content/common/resource_request.h"
+#include "content/common/resource_request_completion_status.h"
+#include "content/common/url_loader.mojom.h"
+#include "content/common/url_loader_factory.mojom.h"
+#include "content/public/browser/resource_context.h"
+#include "content/public/browser/resource_dispatcher_host_delegate.h"
+#include "content/public/common/content_paths.h"
+#include "content/public/test/test_browser_context.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "mojo/public/c/system/data_pipe.h"
+#include "mojo/public/c/system/types.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/system/data_pipe.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_response_info.h"
+#include "net/http/http_status_code.h"
+#include "net/http/http_util.h"
+#include "net/test/url_request/url_request_failed_job.h"
+#include "net/test/url_request/url_request_mock_http_job.h"
+#include "net/url_request/url_request_filter.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace content {
+
+namespace {
+
+class FakeURLLoaderClient final : public mojom::URLLoaderClient {
+ public:
+  FakeURLLoaderClient() : binding_(this) {}
+
+  void OnReceiveResponse(const ResourceResponseHead& response_head) override {
+    has_received_response_ = true;
+    response_head_ = response_head;
+    if (!quit_closure_.Equals(base::Closure()))

[mmenke, 2016/06/13 20:56:58]

There's either an is_empty() or is_null() method that you should be using
instead, not sure which.

[kinuko, 2016/06/14 08:45:29]

is_null() is most common I believe

[yhirano, 2016/07/12 10:49:11]

operator bool() is simpler, I think.

+      quit_closure_.Run();
+  }
+  void OnStartLoadingResponseBody(
+      mojo::ScopedDataPipeConsumerHandle body) override {
+    response_body_ = std::move(body);
+    if (!quit_closure_.Equals(base::Closure()))

[mmenke, 2016/06/13 20:56:58]

is_null()

[yhirano, 2016/07/12 10:49:11]

Done.

+      quit_closure_.Run();
+  }
+  void OnComplete(const ResourceRequestCompletionStatus& status) override {
+    has_received_completion_ = true;
+    completion_status_ = status;
+    if (!quit_closure_.Equals(base::Closure()))

[mmenke, 2016/06/13 20:56:58]

is_null()

[yhirano, 2016/07/12 10:49:11]

Done.

+      quit_closure_.Run();
+  }
+
+  bool has_received_response() const { return has_received_response_; }
+  bool has_received_completion() const { return has_received_completion_; }
+  const ResourceResponseHead& response_head() const { return response_head_; }
+  mojo::DataPipeConsumerHandle response_body() { return response_body_.get(); }
+  const ResourceRequestCompletionStatus& completion_status() const {
+    return completion_status_;
+  }
+
+  mojom::URLLoaderClientPtr CreateInterfacePtrAndBind() {
+    return binding_.CreateInterfacePtrAndBind();
+  }
+
+  void set_quit_closure(const base::Closure& quit_closure) {
+    quit_closure_ = quit_closure;
+  }
+
+ private:
+  mojo::Binding<mojom::URLLoaderClient> binding_;
+  ResourceResponseHead response_head_;
+  mojo::ScopedDataPipeConsumerHandle response_body_;
+  ResourceRequestCompletionStatus completion_status_;
+  bool has_received_response_ = false;
+  bool has_received_completion_ = false;
+  base::Closure quit_closure_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeURLLoaderClient);
+};
+
+class RejectingResourceDispatcherHostDelegate final
+    : public ResourceDispatcherHostDelegate {
+ public:
+  RejectingResourceDispatcherHostDelegate() {}
+  bool ShouldBeginRequest(const std::string& method,
+                          const GURL& url,
+                          ResourceType resource_type,
+                          ResourceContext* resource_context) override {
+    return false;
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(RejectingResourceDispatcherHostDelegate);
+};
+
+class TestURLRequestJobForBrokenBody final : public net::URLRequestJob {
+ public:
+  TestURLRequestJobForBrokenBody(net::URLRequest* request,
+                                 net::NetworkDelegate* network_delegate)
+      : URLRequestJob(request, network_delegate), weak_factory_(this) {}
+
+  void Start() override {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&TestURLRequestJobForBrokenBody::StartAsync,
+                              weak_factory_.GetWeakPtr()));
+  }
+
+  int ReadRawData(net::IOBuffer* buf, int buf_size) override {
+    DCHECK_GT(buf_size, 0);
+    if (++counter_ > 1)
+      return net::ERR_FAILED;
+    buf->data()[0] = 'a';
+    return 1;
+  }
+
+  int GetResponseCode() const override { return 200; }
+
+  void GetResponseInfo(net::HttpResponseInfo* info) override {
+    std::string raw_headers;
+    raw_headers.append(
+        "HTTP/1.1 200 OK\n"
+        "Content-type: text/plain\n");
+    info->headers =
+        new net::HttpResponseHeaders(net::HttpUtil::AssembleRawHeaders(
+            raw_headers.c_str(), static_cast<int>(raw_headers.length())));
+  }
+
+ private:
+  void StartAsync() { NotifyHeadersComplete(); }
+
+  int counter_ = 0;
+  base::WeakPtrFactory<TestURLRequestJobForBrokenBody> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestURLRequestJobForBrokenBody);
+};
+
+class TestInterceptorForBrokenBody final : public net::URLRequestInterceptor {
+ public:
+  explicit TestInterceptorForBrokenBody(const GURL& url) : url_(url) {}
+
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    if (request->url() != url_)
+      return nullptr;
+
+    return new TestURLRequestJobForBrokenBody(request, network_delegate);
+  }
+
+ private:
+  const GURL url_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestInterceptorForBrokenBody);
+};
+
+class URLLoaderFactoryImplTest : public ::testing::TestWithParam<size_t> {

[mmenke, 2016/06/13 20:56:59]

You should document what the size_t is.

[yhirano, 2016/07/12 10:49:11]

Done.

+ public:
+  URLLoaderFactoryImplTest()
+      : thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP),
+        browser_context_(new TestBrowserContext()),
+        resource_message_filter_(new ResourceMessageFilter(
+            0,
+            0,
+            nullptr,
+            nullptr,
+            nullptr,
+            nullptr,
+            nullptr,
+            base::Bind(&URLLoaderFactoryImplTest::GetContexts,
+                       base::Unretained(this)))) {
+    MojoAsyncResourceHandler::SetAllocationSizeForTesting(GetParam());
+
+    factory_impl_holder_.reset(new URLLoaderFactoryHolder(
+        resource_message_filter_, mojo::GetProxy(&factory_)));
+
+    // Calling this function creates a request context.
+    browser_context_->GetResourceContext()->GetRequestContext();
+    base::RunLoop().RunUntilIdle();
+  }
+
+  ~URLLoaderFactoryImplTest() override {
+    rdh_.SetDelegate(nullptr);
+    net::URLRequestFilter::GetInstance()->ClearHandlers();
+
+    rdh_.CancelRequestsForProcess(resource_message_filter_->child_id());
+    base::RunLoop().RunUntilIdle();
+  }
+
+  void GetContexts(ResourceType resource_type,
+                   int origin_pid,
+                   ResourceContext** resource_context,
+                   net::URLRequestContext** request_context) {
+    *resource_context = browser_context_->GetResourceContext();
+    *request_context =
+        browser_context_->GetResourceContext()->GetRequestContext();
+  }
+
+  void RunUntilNextNotification(FakeURLLoaderClient* client) {
+    base::RunLoop run_loop;
+    client->set_quit_closure(run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
+  TestBrowserThreadBundle thread_bundle_;
+  ResourceDispatcherHostImpl rdh_;
+  std::unique_ptr<TestBrowserContext> browser_context_;
+  scoped_refptr<ResourceMessageFilter> resource_message_filter_;
+  mojom::URLLoaderFactoryPtr factory_;
+  std::unique_ptr<URLLoaderFactoryHolder> factory_impl_holder_;
+
+  DISALLOW_COPY_AND_ASSIGN(URLLoaderFactoryImplTest);
+};
+
+TEST_P(URLLoaderFactoryImplTest, GetResponse) {
+  mojom::URLLoaderPtr loader;
+  base::FilePath root;
+  PathService::Get(DIR_TEST_DATA, &root);
+  net::URLRequestMockHTTPJob::AddUrlHandlers(root,
+                                             BrowserThread::GetBlockingPool());

[mmenke, 2016/06/13 20:56:59]

Suggest adding all these mock handler setup calls to the test fixture's
constructor, instead.

[yhirano, 2016/07/12 10:49:11]

I'd prefer writing case-specific preparation in the test case unless it's
reusable.

[mmenke, 2016/07/12 15:10:45]

You call net::URLRequestFailedJob::AddUrlHandler() on two of them, and I
wouldn't be surprised to see more in the future.  I suspect we'll be using
URLRequestMockHTTPJob::AddUrlHandlers in other tests as well.

[yhirano, 2016/07/13 12:04:57]

I removed one of them at PS49, so there is one
net::URLRequestMockHTTPJob::AddUrlHandlers and one
URLRequestFailedJob::AddUrlHandler. I will reconsider if I have more duplicates.

+  ResourceRequest request;
+  FakeURLLoaderClient client;
+  // Assume the file contents is small enough to be stored in the data pipe.
+  request.url = net::URLRequestMockHTTPJob::GetMockUrl("hello.html");
+  request.method = "GET";
+  factory_->CreateLoaderAndStart(mojo::GetProxy(&loader), 1, request,
+                                 client.CreateInterfacePtrAndBind());
+
+  ASSERT_FALSE(client.has_received_response());
+  ASSERT_FALSE(client.response_body().is_valid());
+  ASSERT_FALSE(client.has_received_completion());
+
+  RunUntilNextNotification(&client);
+  ASSERT_TRUE(client.has_received_response());
+  ASSERT_FALSE(client.has_received_completion());
+  ASSERT_FALSE(client.has_received_completion());
+
+  RunUntilNextNotification(&client);
+  ASSERT_TRUE(client.has_received_response());
+  ASSERT_TRUE(client.response_body().is_valid());
+  ASSERT_FALSE(client.has_received_completion());
+
+  RunUntilNextNotification(&client);
+  ASSERT_TRUE(client.has_received_completion());
+
+  EXPECT_EQ(200, client.response_head().headers->response_code());
+  std::string content_type;
+  client.response_head().headers->GetNormalizedHeader("content-type",
+                                                      &content_type);
+  EXPECT_EQ("text/html", content_type);
+  EXPECT_EQ(0, client.completion_status().error_code);
+
+  std::string contents;
+  while (true) {
+    char buffer[16];
+    uint32_t read = sizeof(buffer);
+    MojoResult r = mojo::ReadDataRaw(client.response_body(), buffer, &read,
+                                     MOJO_READ_DATA_FLAG_NONE);
+    if (r == MOJO_RESULT_FAILED_PRECONDITION)
+      break;
+    if (r == MOJO_RESULT_SHOULD_WAIT)
+      continue;
+    ASSERT_EQ(MOJO_RESULT_OK, r);
+    contents += std::string(buffer, read);
+  }
+  EXPECT_EQ(
+      "<!doctype html>\n"
+      "<p>hello</p>\n",
+      contents);
+}
+
+TEST_P(URLLoaderFactoryImplTest, GetFailedResponse) {
+  mojom::URLLoaderPtr loader;
+  ResourceRequest request;
+  FakeURLLoaderClient client;
+  net::URLRequestFailedJob::AddUrlHandler();
+  request.url = net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(
+      net::URLRequestFailedJob::START, net::ERR_TIMED_OUT);
+  request.method = "GET";
+  factory_->CreateLoaderAndStart(mojo::GetProxy(&loader), 1, request,
+                                 client.CreateInterfacePtrAndBind());
+
+  RunUntilNextNotification(&client);
+  ASSERT_FALSE(client.has_received_response());
+  ASSERT_FALSE(client.response_body().is_valid());
+  ASSERT_TRUE(client.has_received_completion());
+
+  EXPECT_EQ(net::ERR_TIMED_OUT, client.completion_status().error_code);
+}
+
+// This test tests a case where resource loading is cancelled before started.
+TEST_P(URLLoaderFactoryImplTest, InvalidURL) {
+  mojom::URLLoaderPtr loader;
+  ResourceRequest request;
+  FakeURLLoaderClient client;
+  request.url = GURL();
+  request.method = "GET";
+  ASSERT_FALSE(request.url.is_valid());
+  factory_->CreateLoaderAndStart(mojo::GetProxy(&loader), 1, request,
+                                 client.CreateInterfacePtrAndBind());
+
+  RunUntilNextNotification(&client);
+  ASSERT_FALSE(client.has_received_response());
+  ASSERT_FALSE(client.response_body().is_valid());
+  ASSERT_TRUE(client.has_received_completion());
+
+  EXPECT_EQ(net::ERR_ABORTED, client.completion_status().error_code);
+}
+
+// This test tests a case where resource loading is cancelled before started.
+TEST_P(URLLoaderFactoryImplTest, ShouldNotRequestURL) {
+  mojom::URLLoaderPtr loader;
+  RejectingResourceDispatcherHostDelegate rdh_delegate;
+  rdh_.SetDelegate(&rdh_delegate);
+  ResourceRequest request;
+  FakeURLLoaderClient client;
+  request.url = GURL("http://localhost/");
+  request.method = "GET";
+  factory_->CreateLoaderAndStart(mojo::GetProxy(&loader), 1, request,
+                                 client.CreateInterfacePtrAndBind());
+
+  RunUntilNextNotification(&client);
+  rdh_.SetDelegate(nullptr);
+
+  ASSERT_FALSE(client.has_received_response());
+  ASSERT_FALSE(client.response_body().is_valid());
+  ASSERT_TRUE(client.has_received_completion());
+
+  EXPECT_EQ(net::ERR_ABORTED, client.completion_status().error_code);
+}
+
+TEST_P(URLLoaderFactoryImplTest, FailedAfterResponseStarted) {
+  mojom::URLLoaderPtr loader;
+  ResourceRequest request;
+  FakeURLLoaderClient client;
+  net::URLRequestFailedJob::AddUrlHandler();
+  request.url = net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(
+      net::URLRequestFailedJob::READ_ASYNC, net::ERR_TIMED_OUT);
+  request.method = "GET";
+  factory_->CreateLoaderAndStart(mojo::GetProxy(&loader), 1, request,
+                                 client.CreateInterfacePtrAndBind());
+
+  ASSERT_FALSE(client.has_received_response());
+  ASSERT_FALSE(client.response_body().is_valid());
+  ASSERT_FALSE(client.has_received_completion());
+
+  RunUntilNextNotification(&client);
+  ASSERT_FALSE(client.has_received_response());
+  ASSERT_TRUE(client.response_body().is_valid());
+  ASSERT_FALSE(client.has_received_completion());
+
+  RunUntilNextNotification(&client);
+  ASSERT_FALSE(client.has_received_response());
+  ASSERT_TRUE(client.response_body().is_valid());
+  ASSERT_TRUE(client.has_received_completion());
+
+  EXPECT_EQ(net::ERR_TIMED_OUT, client.completion_status().error_code);
+}
+
+TEST_P(URLLoaderFactoryImplTest, BrokenBody) {
+  mojom::URLLoaderPtr loader;
+  ResourceRequest request;
+  FakeURLLoaderClient client;
+  request.url = GURL("http://www.example.com/test");
+  request.method = "GET";
+  net::URLRequestFilter::GetInstance()->AddUrlInterceptor(
+      request.url,
+      base::WrapUnique(new TestInterceptorForBrokenBody(request.url)));
+  factory_->CreateLoaderAndStart(mojo::GetProxy(&loader), 1, request,
+                                 client.CreateInterfacePtrAndBind());
+
+  ASSERT_FALSE(client.has_received_response());
+  ASSERT_FALSE(client.response_body().is_valid());
+  ASSERT_FALSE(client.has_received_completion());
+
+  RunUntilNextNotification(&client);
+  ASSERT_FALSE(client.has_received_response());
+  ASSERT_TRUE(client.response_body().is_valid());
+  ASSERT_TRUE(client.has_received_completion());
+
+  EXPECT_EQ(net::ERR_FAILED, client.completion_status().error_code);
+}
+
+INSTANTIATE_TEST_CASE_P(URLLoaderFactoryImplTest,
+                        URLLoaderFactoryImplTest,
+                        ::testing::Values(128, 32 * 1024));

[mmenke, 2016/06/13 20:56:58]

None of these tests seem to return anything with a body longer than 128 bytes,
so an allocation size of 128 bytes doesn't really help us at all.

We do want to test cases with more data, though.  Maybe a test with a 512 byte
response?

[yhirano, 2016/07/12 10:49:11]

The parameter is not for such cases. The mime sniffer requires us to allocate at
least 2048 bytes buffer, and 128 is for cases where mojo allocates a smaller
buffer.

+
+}  // namespace

[mmenke, 2016/06/13 20:56:58]

How about a test that fails the CheckForSufficientResource?  May have to keep on
creating and starting hung requests until we get that error.

Can create jobs that hang on the first body read with: 
net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(net::URLRequestFailedJob::READ_SYNC,
net::ERR_IO_PENDING);

[mmenke, 2016/06/13 20:56:58]

Can we make BeginWriteDataRaw return sync/async/errors?

[mmenke, 2016/06/13 20:56:59]

Should have tests that check each of the strings in completion_status() when
they have two different values, to make sure they're passed along correctly.

[yhirano, 2016/07/12 10:49:11]

These are tested in mojo_async_resource_handler_unittest.cc.

+
+}  // namespace content