Use mojo for Chrome Loading, Part 1

content/browser/loader/mojo_async_resource_handler_unittest.cc

Index: content/browser/loader/mojo_async_resource_handler_unittest.cc
diff --git a/content/browser/loader/mojo_async_resource_handler_unittest.cc b/content/browser/loader/mojo_async_resource_handler_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2ffd95e1dd3593d818e963017b36e799d0a619ae
--- /dev/null
+++ b/content/browser/loader/mojo_async_resource_handler_unittest.cc
@@ -0,0 +1,850 @@
+// 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/mojo_async_resource_handler.h"
+
+#include <string.h>
+
+#include <utility>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/scoped_vector.h"
+#include "base/memory/weak_ptr.h"
+#include "base/run_loop.h"
+#include "content/browser/loader/mojo_async_resource_handler_test_util.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
+#include "content/browser/loader/resource_request_info_impl.h"
+#include "content/common/resource_request_completion_status.h"
+#include "content/common/url_loader.mojom.h"
+#include "content/public/browser/appcache_service.h"
+#include "content/public/browser/navigation_data.h"
+#include "content/public/browser/resource_context.h"
+#include "content/public/browser/resource_controller.h"
+#include "content/public/browser/resource_dispatcher_host_delegate.h"
+#include "content/public/browser/resource_throttle.h"
+#include "content/public/browser/stream_info.h"
+#include "content/public/common/resource_response.h"
+#include "content/public/common/resource_type.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/system/data_pipe.h"
+#include "net/base/auth.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/ssl/client_cert_store.h"
+#include "net/test/url_request/url_request_failed_job.h"
+#include "net/url_request/url_request.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_status.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/page_transition_types.h"
+
+namespace content {
+namespace {
+
+class FakeResourceDispatcherHostDelegate
+    : public ResourceDispatcherHostDelegate {
+ public:
+  FakeResourceDispatcherHostDelegate() {}
+  ~FakeResourceDispatcherHostDelegate() override {}
+
+  bool ShouldBeginRequest(const std::string& method,
+                          const GURL& url,
+                          ResourceType resource_type,
+                          ResourceContext* resource_context) override {
+    ADD_FAILURE() << "ShouldBeginRequest should not be called.";
+    return false;
+  }
+
+  void RequestBeginning(net::URLRequest* request,
+                        ResourceContext* resource_context,
+                        AppCacheService* appcache_service,
+                        ResourceType resource_type,
+                        ScopedVector<ResourceThrottle>* throttles) override {
+    ADD_FAILURE() << "RequestBeginning should not be called.";
+  }
+
+  void DownloadStarting(net::URLRequest* request,
+                        ResourceContext* resource_context,
+                        int child_id,
+                        int route_id,
+                        bool is_content_initiated,
+                        bool must_download,
+                        ScopedVector<ResourceThrottle>* throttles) override {
+    ADD_FAILURE() << "DownloadStarting should not be called.";
+  }
+
+  ResourceDispatcherHostLoginDelegate* CreateLoginDelegate(
+      net::AuthChallengeInfo* auth_info,
+      net::URLRequest* request) override {
+    ADD_FAILURE() << "CreateLoginDelegate should not be called.";
+    return nullptr;
+  }
+
+  bool HandleExternalProtocol(
+      const GURL& url,
+      int child_id,
+      const ResourceRequestInfo::WebContentsGetter& web_contents_getter,
+      bool is_main_frame,
+      ui::PageTransition page_transition,
+      bool has_user_gesture,
+      ResourceContext* resource_context) override {
+    ADD_FAILURE() << "HandleExternalProtocol should not be called.";
+    return false;
+  }
+
+  bool ShouldForceDownloadResource(const GURL& url,
+                                   const std::string& mime_type) override {
+    ADD_FAILURE() << "ShouldForceDownloadResource should not be called.";
+    return false;
+  }
+
+  bool ShouldInterceptResourceAsStream(net::URLRequest* request,
+                                       const base::FilePath& plugin_path,
+                                       const std::string& mime_type,
+                                       GURL* origin,
+                                       std::string* payload) override {
+    ADD_FAILURE() << "ShouldInterceptResourceAsStream should not be called.";
+    return false;
+  }
+
+  void OnStreamCreated(net::URLRequest* request,
+                       std::unique_ptr<content::StreamInfo> stream) override {
+    ADD_FAILURE() << "OnStreamCreated should not be called.";
+  }
+
+  void OnResponseStarted(net::URLRequest* request,
+                         ResourceContext* resource_context,
+                         ResourceResponse* response) override {
+    is_on_response_started_called_ = true;
+  }
+
+  void OnRequestRedirected(const GURL& redirect_url,
+                           net::URLRequest* request,
+                           ResourceContext* resource_context,
+                           ResourceResponse* response) override {
+    ADD_FAILURE() << "OnRequestRedirected should not be called.";
+  }
+
+  void RequestComplete(net::URLRequest* url_request) override {
+    ADD_FAILURE() << "RequestComplete should not be called.";
+  }
+
+  bool ShouldEnableLoFiMode(
+      const net::URLRequest& url_request,
+      content::ResourceContext* resource_context) override {
+    ADD_FAILURE() << "ShouldEnableLoFiMode should not be called.";
+    return false;
+  }
+
+  NavigationData* GetNavigationData(net::URLRequest* request) const override {
+    ADD_FAILURE() << "GetNavigationData should not be called.";
+    return nullptr;
+  }
+
+  std::unique_ptr<net::ClientCertStore> CreateClientCertStore(
+      ResourceContext* resource_context) override {
+    ADD_FAILURE() << "CreateClientCertStore should not be called.";
+    return nullptr;
+  }
+
+  bool is_on_response_started_called() const {
+    return is_on_response_started_called_;
+  }
+
+ private:
+  bool is_on_response_started_called_ = false;

[mmenke, 2016/07/25 22:03:54]

is -> was?

Or better, make this an integer, and call it on_response_started_called_count_,
so can catch unexpected calls.  Could even check in the destructor that it
hasn't been changed since the last call to the accessor call, if we want to be
really paranoid.

Or could just add an EXPECT_FALSE in OnResponseStarted, to make sure it's not
called twice.  (Destructor check would still potentially be useful in that case)

[yhirano, 2016/07/27 16:13:28]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(FakeResourceDispatcherHostDelegate);

[mmenke, 2016/07/25 22:03:54]

include base/macros.h

[yhirano, 2016/07/27 16:13:29]

My understanding is mojo_async_resource_handler.h is the related header of this
file in terms of [1] (it's included at the top of the file). Include directives
in the related file is not needed to be repeated [2]. As base/macros.h is
included in mojo_async_resource_handler.h, I think we don't need to include it
here.

1: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

[mmenke, 2016/07/27 16:49:36]

Hrm...I've always considered foo.h the related header for only foo.cc (Well,
include foo.h first in foo_unittest.cc, but otherwise, treat it like a normal
header.)

+};
+
+class FakeResourceController : public ResourceController {
+ public:
+  FakeResourceController() {}
+  ~FakeResourceController() override {}
+
+  void Cancel() override { ADD_FAILURE() << "Cancel should not be called."; }
+
+  void CancelAndIgnore() override {
+    ADD_FAILURE() << "CancelAndIgnore should not be called.";
+  }
+
+  void CancelWithError(int error_code) override {
+    is_cancel_with_error_called_ = true;
+    error_ = error_code;
+    if (quit_closure_)
+      quit_closure_.Run();
+  }
+
+  void Resume() override {
+    ++num_resume_calls_;
+    if (quit_closure_)
+      quit_closure_.Run();
+  }
+
+  void set_quit_closure(const base::Closure& quit_closure) {
+    quit_closure_ = quit_closure;
+  }
+
+  bool is_cancel_with_error_called() const {
+    return is_cancel_with_error_called_;
+  }
+  int error() const { return error_; }
+  int num_resume_calls() const { return num_resume_calls_; }
+
+ private:
+  bool is_cancel_with_error_called_ = false;
+  int error_ = net::OK;
+  int num_resume_calls_ = 0;
+  base::Closure quit_closure_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeResourceController);
+};
+
+class MojoAsyncResourceHandlerWithCustomDataPipeOperations
+    : public MojoAsyncResourceHandler {
+ public:
+  MojoAsyncResourceHandlerWithCustomDataPipeOperations(
+      net::URLRequest* request,
+      ResourceDispatcherHostImpl* rdh,
+      mojo::InterfaceRequest<mojom::URLLoader> mojo_request,
+      mojom::URLLoaderClientPtr url_loader_client)
+      : MojoAsyncResourceHandler(request,
+                                 rdh,
+                                 std::move(mojo_request),
+                                 std::move(url_loader_client)) {}
+  ~MojoAsyncResourceHandlerWithCustomDataPipeOperations() override {}
+
+  void set_begin_write_expectation(MojoResult begin_write_expectation) {
+    is_begin_write_expectation_set_ = true;
+    begin_write_expectation_ = begin_write_expectation;
+  }
+  void set_end_write_expectation(MojoResult end_write_expectation) {
+    is_end_write_expectation_set_ = true;
+    end_write_expectation_ = end_write_expectation;
+  }
+
+ private:
+  MojoResult BeginWrite(void** data, uint32_t* available) override {
+    if (is_begin_write_expectation_set_)
+      return begin_write_expectation_;
+    return MojoAsyncResourceHandler::BeginWrite(data, available);
+  }
+  MojoResult EndWrite(uint32_t written) override {
+    if (is_end_write_expectation_set_)
+      return end_write_expectation_;
+    return MojoAsyncResourceHandler::EndWrite(written);
+  }
+
+  bool is_begin_write_expectation_set_ = false;
+  bool is_end_write_expectation_set_ = false;
+  MojoResult begin_write_expectation_ = MOJO_RESULT_UNKNOWN;
+  MojoResult end_write_expectation_ = MOJO_RESULT_UNKNOWN;
+
+  DISALLOW_COPY_AND_ASSIGN(
+      MojoAsyncResourceHandlerWithCustomDataPipeOperations);
+};
+
+class MojoAsyncResourceHandlerTestBase {
+ public:
+  MojoAsyncResourceHandlerTestBase()
+      : thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP),
+        browser_context_(new TestBrowserContext()) {
+    MojoAsyncResourceHandler::SetAllocationSizeForTesting(32 * 1024);
+
+    // Calling this function creates a request context.
+    browser_context_->GetResourceContext()->GetRequestContext();
+    base::RunLoop().RunUntilIdle();
+
+    url_request_delegate_.reset(new net::TestDelegate());
+    net::URLRequestContext* request_context =
+        browser_context_->GetResourceContext()->GetRequestContext();
+    request_ = request_context->CreateRequest(
+        net::URLRequestFailedJob::GetMockHttpUrl(net::ERR_TIMED_OUT),
+        net::DEFAULT_PRIORITY, url_request_delegate_.get());
+    ResourceRequestInfo::AllocateForTesting(
+        request_.get(), RESOURCE_TYPE_XHR,
+        browser_context_->GetResourceContext(), 2, 0, 0, true, false, false,
+        true, false);
+    handler_.reset(new MojoAsyncResourceHandlerWithCustomDataPipeOperations(
+        request_.get(), &rdh_, nullptr,
+        url_loader_client_.CreateInterfacePtrAndBind()));
+    handler_->SetController(&resource_controller_);
+  }
+
+  virtual ~MojoAsyncResourceHandlerTestBase() {
+    rdh_.SetDelegate(nullptr);

[mmenke, 2016/07/25 22:03:54]

Should we just make rdh_delegate_ part of the test fixture, and always set it
up?  May be worth checking it gets called (And, just as importantly, not called
unexpectedly) in a few more tests....

[yhirano, 2016/07/27 16:13:28]

Done.

+    net::URLRequestFilter::GetInstance()->ClearHandlers();
+    MojoAsyncResourceHandler::SetAllocationSizeForTesting(
+        MojoAsyncResourceHandler::kDefaultAllocationSize);
+  }
+
+  void RunUntilNextNotification() {
+    base::RunLoop run_loop;
+    url_loader_client_.set_quit_closure(run_loop.QuitClosure());
+    resource_controller_.set_quit_closure(run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
+  TestBrowserThreadBundle thread_bundle_;
+  ResourceDispatcherHostImpl rdh_;
+  FakeURLLoaderClient url_loader_client_;
+  FakeResourceController resource_controller_;
+  std::unique_ptr<TestBrowserContext> browser_context_;
+  std::unique_ptr<net::TestDelegate> url_request_delegate_;
+  std::unique_ptr<net::URLRequest> request_;
+  std::unique_ptr<MojoAsyncResourceHandlerWithCustomDataPipeOperations>
+      handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(MojoAsyncResourceHandlerTestBase);
+};
+
+class MojoAsyncResourceHandlerTest : public MojoAsyncResourceHandlerTestBase,
+                                     public ::testing::Test {};
+
+// This test class is parameterized with MojoAsyncResourceHandler's allocation
+// size.
+class MojoAsyncResourceHandlerWithAllocationSizeTest
+    : public MojoAsyncResourceHandlerTestBase,
+      public ::testing::TestWithParam<size_t> {
+ protected:
+  MojoAsyncResourceHandlerWithAllocationSizeTest() {
+    MojoAsyncResourceHandler::SetAllocationSizeForTesting(GetParam());
+  }
+};
+
+TEST_F(MojoAsyncResourceHandlerTest, InFlightRequests) {
+  EXPECT_EQ(0, rdh_.num_in_flight_requests_for_testing());
+  handler_ = nullptr;
+  EXPECT_EQ(0, rdh_.num_in_flight_requests_for_testing());
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, OnResponseStarted) {
+  scoped_refptr<ResourceResponse> response = new ResourceResponse();
+  FakeResourceDispatcherHostDelegate rdh_delegate;
+  response->head.content_length = 99;
+  response->head.request_start =
+      base::TimeTicks::UnixEpoch() + base::TimeDelta::FromDays(14);
+  response->head.response_start =
+      base::TimeTicks::UnixEpoch() + base::TimeDelta::FromDays(28);
+
+  bool defer = false;
+
+  base::TimeTicks now1 = base::TimeTicks::Now();

[mmenke, 2016/07/25 22:03:54]

Call OnWillStart?

+  rdh_.SetDelegate(&rdh_delegate);
+  ASSERT_TRUE(handler_->OnResponseStarted(response.get(), &defer));
+  rdh_.SetDelegate(nullptr);
+  base::TimeTicks now2 = base::TimeTicks::Now();
+
+  EXPECT_FALSE(defer);
+  EXPECT_EQ(request_->creation_time(), response->head.request_start);
+  EXPECT_LE(now1, response->head.response_start);
+  EXPECT_LE(response->head.response_start, now2);
+  EXPECT_TRUE(rdh_delegate.is_on_response_started_called());
+
+  RunUntilNextNotification();

[mmenke, 2016/07/25 22:03:54]

What do you think of more specific methods for these? 
url_loader_client_.RunUntilResponseReceived(), etc?

Not a big ran of just running until some miscellaneous something happens in one
of two classes.

[yhirano, 2016/07/27 16:13:29]

Done.

+  EXPECT_TRUE(url_loader_client_.has_received_response());
+  EXPECT_EQ(response->head.request_start,
+            url_loader_client_.response_head().request_start);
+  EXPECT_EQ(response->head.response_start,
+            url_loader_client_.response_head().response_start);
+  EXPECT_EQ(99, url_loader_client_.response_head().content_length);
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, OnWillStart) {

[mmenke, 2016/07/25 22:03:53]

Seems like this should be first - it's always called before other methods.

[yhirano, 2016/07/27 16:13:29]

Done.

+  bool defer = false;
+  EXPECT_TRUE(handler_->OnWillStart(request_->url(), &defer));
+  EXPECT_FALSE(defer);
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, OnBeforeNetworkStart) {
+  bool defer = false;
+  EXPECT_TRUE(handler_->OnBeforeNetworkStart(request_->url(), &defer));

[mmenke, 2016/07/25 22:03:54]

Now this method really has been removed, honest!  :)

[yhirano, 2016/07/27 16:13:28]

Done.

+  EXPECT_FALSE(defer);
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, OnWillReadAndInFlightRequests) {
+  EXPECT_EQ(0, rdh_.num_in_flight_requests_for_testing());
+  scoped_refptr<net::IOBuffer> buffer;
+  int buf_size;
+  EXPECT_TRUE(handler_->OnWillRead(&buffer, &buf_size, -1));

[mmenke, 2016/07/25 22:03:54]

Just calling this method seems weird - normally we call OnWillStart,
OnResponseStarted, and then OnWillRead.

Or with mime sniffing, we call OnWillStart, OnWillRead, OnResponseStarted.

Should test both variants (And preferably run to completion, with at least one
successful read, through both paths).

Testing the actual implementation of the API minimizes this code's dependence on
the MojoHandler's implementation details, instead, we're depending more on
whether it implements the spec.

[yhirano, 2016/07/27 16:13:28]

Having more tests sounds a good idea. Done.

I think it's OK to call OnWillRead without calling OnWillStart in this file,
though. Actually tests in this file already extremely depend on
MojoAsyncResourceHandler's implementation (e.g., a certain BeginWriteData
failure leads to a certain result or side-effect). Additionally, to keep high
code coverage, it is important to keep tests simple, more focused on each code
fragment.

[mmenke, 2016/07/27 16:49:36]

I really think we should mimic real world usage as closely as we can - it makes
these tests less fragile when modifying / refactoring AsyncMojoResourceHandler. 
To put it bluntly, as the one most likely to either review refactoring of this
file or make them himself, I'm not going to sign off on this CL unless we follow
the expected order of ResourceHandler calls more closely.

[yhirano, 2016/07/28 14:50:46]

Done.

+  EXPECT_EQ(1, rdh_.num_in_flight_requests_for_testing());
+  handler_ = nullptr;
+  EXPECT_EQ(0, rdh_.num_in_flight_requests_for_testing());
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, OnWillReadWithInsufficientResource) {
+  rdh_.set_max_num_in_flight_requests_per_process(0);
+
+  scoped_refptr<net::IOBuffer> buffer;
+  int buf_size = 0;
+  EXPECT_FALSE(handler_->OnWillRead(&buffer, &buf_size, -1));

[mmenke, 2016/07/25 22:03:53]

Not going to comment for all of these, but think they should all go through
OnWillStart+OnResponseStarted.

+  EXPECT_FALSE(buffer);
+  EXPECT_EQ(0, buf_size);
+  EXPECT_EQ(1, rdh_.num_in_flight_requests_for_testing());
+  EXPECT_TRUE(resource_controller_.is_cancel_with_error_called());
+  EXPECT_EQ(net::ERR_INSUFFICIENT_RESOURCES, resource_controller_.error());
+  handler_ = nullptr;
+  EXPECT_EQ(0, rdh_.num_in_flight_requests_for_testing());
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, OnWillReadAndOnReadCompleted) {
+  bool defer = false;
+  scoped_refptr<net::IOBuffer> buffer;
+  int buf_size;
+
+  ASSERT_TRUE(handler_->OnWillRead(&buffer, &buf_size, -1));
+  ASSERT_TRUE(buffer);
+  // The buffer size that the mime sniffer requires implicitly.
+  ASSERT_GE(buf_size, 2 * 1024);
+
+  RunUntilNextNotification();
+  ASSERT_TRUE(url_loader_client_.response_body().is_valid());
+
+  buffer->data()[0] = 'A';
+  buffer->data()[1] = 'B';
+  ASSERT_TRUE(handler_->OnReadCompleted(2, &defer));
+  EXPECT_FALSE(defer);
+
+  std::string contents;

[mmenke, 2016/07/25 22:03:54]

include <string>?

[yhirano, 2016/07/27 16:13:28]

It's included in mojo_async_resource_handler.h.

+  do {
+    char buffer[16];

[mmenke, 2016/07/25 22:03:53]

There's another local called buffer outside this loop - should avoid identical
variable names.

[yhirano, 2016/07/27 16:13:28]

Done.

+    uint32_t read = sizeof(buffer);

[mmenke, 2016/07/25 22:03:54]

include stdint.h?

[mmenke, 2016/07/25 22:03:54]

sizeof => arraysize?

[yhirano, 2016/07/27 16:13:28]

sizeof is more preferable here, as the argument represents the size of the
region.

[yhirano, 2016/07/27 16:13:28]

It's included in mojo_async_resource_handler.h.

+    MojoResult result =
+        mojo::ReadDataRaw(url_loader_client_.response_body(), buffer, &read,
+                          MOJO_READ_DATA_FLAG_NONE);
+    if (result == MOJO_RESULT_SHOULD_WAIT)

[mmenke, 2016/07/25 22:03:54]

Is there any spec on what we have to do if we get this result?  It seems weird
to not even spin the message loop if we get it.

[yhirano, 2016/07/27 16:13:28]

In production code, we use mojo::Watcher to be notified when the handle becomes
readable, but we cannot use it here because we need to stay in the test case.
spinning the message loop is a good idea.

+      continue;
+    contents.append(buffer, read);
+  } while (contents.size() < 2);

[mmenke, 2016/07/25 22:03:53]

optional:  Suggest switching to a while() loop - they're much more common, and
as a result, easier for the average Chrome dev to follow.

[yhirano, 2016/07/27 16:13:28]

Done.

+  EXPECT_EQ("AB", contents);
+}
+
+TEST_F(MojoAsyncResourceHandlerTest,
+       OnWillReadAndOnReadCompletedWithInsufficientInitialCapacity) {
+  MojoAsyncResourceHandler::SetAllocationSizeForTesting(2);
+
+  bool defer = false;
+  scoped_refptr<net::IOBuffer> buffer;
+  int buf_size;
+
+  ASSERT_TRUE(handler_->OnWillRead(&buffer, &buf_size, -1));
+  ASSERT_TRUE(buffer);
+  // The buffer size that the mime sniffer requires implicitly.
+  ASSERT_GE(buf_size, 2 * 1024);
+
+  RunUntilNextNotification();
+  ASSERT_TRUE(url_loader_client_.response_body().is_valid());
+
+  const std::string data("abcdefgh");
+  strcpy(buffer->data(), data.c_str());
+  ASSERT_TRUE(handler_->OnReadCompleted(data.size(), &defer));
+  EXPECT_TRUE(defer);
+
+  std::string contents;
+  do {
+    // This is needed for Resume to be called.
+    base::RunLoop().RunUntilIdle();
+    char buffer[16];

[mmenke, 2016/07/25 22:03:53]

There's another local called buffer outside this loop - should avoid identical
variable names.

[yhirano, 2016/07/27 16:13:29]

Done.

+    uint32_t read = sizeof(buffer);
+    MojoResult result =
+        mojo::ReadDataRaw(url_loader_client_.response_body(), buffer, &read,
+                          MOJO_READ_DATA_FLAG_NONE);
+    if (result == MOJO_RESULT_SHOULD_WAIT)
+      continue;
+    ASSERT_EQ(MOJO_RESULT_OK, result);
+    contents.append(buffer, read);
+  } while (contents.size() < data.size());

[mmenke, 2016/07/25 22:03:53]

Again, suggest a while loop

[yhirano, 2016/07/27 16:13:29]

Done.

+  EXPECT_EQ(data, contents);
+  EXPECT_EQ(0, resource_controller_.num_resume_calls());
+}
+
+TEST_F(MojoAsyncResourceHandlerTest,
+       IOBufferFromOnWillReadShouldRemainValidEvenIfHandlerIsGone) {
+  scoped_refptr<net::IOBuffer> io_buffer;
+  int buf_size;
+
+  ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+  ASSERT_TRUE(io_buffer);
+  // The io_buffer size that the mime sniffer requires implicitly.
+  ASSERT_GE(buf_size, 2 * 1024);
+
+  handler_ = nullptr;
+  url_loader_client_.Unbind();
+  base::RunLoop().RunUntilIdle();
+
+  // Hopefully ASAN checks this operation's validity.
+  io_buffer->data()[0] = 'A';
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, OnResponseCompleted) {
+  scoped_refptr<ResourceResponse> response = new ResourceResponse();
+  bool defer = false;
+  ASSERT_TRUE(handler_->OnResponseStarted(response.get(), &defer));
+  EXPECT_FALSE(defer);
+  RunUntilNextNotification();
+  EXPECT_TRUE(url_loader_client_.has_received_response());
+
+  handler_->GetRequestInfoForTesting()->set_was_ignored_by_handler(false);
+  net::URLRequestStatus status(net::URLRequestStatus::SUCCESS, net::OK);
+  std::string security_info = "info0";
+
+  base::TimeTicks now1 = base::TimeTicks::Now();
+  handler_->OnResponseCompleted(status, security_info, &defer);
+  base::TimeTicks now2 = base::TimeTicks::Now();
+  EXPECT_FALSE(defer);
+
+  RunUntilNextNotification();
+  EXPECT_TRUE(url_loader_client_.has_received_completion());
+  EXPECT_EQ(net::OK, url_loader_client_.completion_status().error_code);
+  EXPECT_FALSE(url_loader_client_.completion_status().was_ignored_by_handler);
+  EXPECT_EQ("info0", url_loader_client_.completion_status().security_info);
+  EXPECT_LE(now1, url_loader_client_.completion_status().completion_time);
+  EXPECT_LE(url_loader_client_.completion_status().completion_time, now2);
+  EXPECT_EQ(request_->GetTotalReceivedBytes(),
+            url_loader_client_.completion_status().encoded_data_length);
+}
+
+// This test case sets different status values from OnResponseCompleted.
+TEST_F(MojoAsyncResourceHandlerTest, OnResponseCompleted2) {
+  scoped_refptr<ResourceResponse> response = new ResourceResponse();
+  bool defer = false;
+  ASSERT_TRUE(handler_->OnResponseStarted(response.get(), &defer));
+  EXPECT_FALSE(defer);
+
+  handler_->GetRequestInfoForTesting()->set_was_ignored_by_handler(true);
+  net::URLRequestStatus status(net::URLRequestStatus::CANCELED,
+                               net::ERR_ABORTED);
+  std::string security_info = "info1";
+
+  base::TimeTicks now1 = base::TimeTicks::Now();
+  handler_->OnResponseCompleted(status, security_info, &defer);
+  base::TimeTicks now2 = base::TimeTicks::Now();
+  EXPECT_FALSE(defer);
+
+  RunUntilNextNotification();
+  EXPECT_TRUE(url_loader_client_.has_received_completion());
+  EXPECT_EQ(net::ERR_ABORTED,
+            url_loader_client_.completion_status().error_code);
+  EXPECT_TRUE(url_loader_client_.completion_status().was_ignored_by_handler);
+  EXPECT_EQ("info1", url_loader_client_.completion_status().security_info);
+  EXPECT_LE(now1, url_loader_client_.completion_status().completion_time);
+  EXPECT_LE(url_loader_client_.completion_status().completion_time, now2);
+  EXPECT_EQ(request_->GetTotalReceivedBytes(),
+            url_loader_client_.completion_status().encoded_data_length);
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, OnResponseCompletedWithCanceledTimedOut) {
+  net::URLRequestStatus status(net::URLRequestStatus::CANCELED,
+                               net::ERR_TIMED_OUT);
+  bool defer = false;
+
+  handler_->OnResponseCompleted(status, "security_info", &defer);
+  EXPECT_FALSE(defer);
+
+  RunUntilNextNotification();
+  EXPECT_TRUE(url_loader_client_.has_received_completion());
+  EXPECT_EQ(net::ERR_TIMED_OUT,
+            url_loader_client_.completion_status().error_code);
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, OnResponseCompletedWithFailedTimedOut) {
+  net::URLRequestStatus status(net::URLRequestStatus::FAILED,
+                               net::ERR_TIMED_OUT);
+  bool defer = false;
+
+  handler_->OnResponseCompleted(status, "security_info", &defer);
+  EXPECT_FALSE(defer);
+
+  RunUntilNextNotification();
+  EXPECT_TRUE(url_loader_client_.has_received_completion());
+  EXPECT_EQ(net::ERR_TIMED_OUT,
+            url_loader_client_.completion_status().error_code);
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, ResponseCompletionShouldCloseDataPipe) {
+  scoped_refptr<ResourceResponse> response = new ResourceResponse();
+  bool defer = false;
+  ASSERT_TRUE(handler_->OnResponseStarted(response.get(), &defer));
+  EXPECT_FALSE(defer);
+  RunUntilNextNotification();
+  EXPECT_TRUE(url_loader_client_.has_received_response());
+
+  scoped_refptr<net::IOBuffer> io_buffer;
+  int buf_size;
+  ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+  RunUntilNextNotification();
+  ASSERT_TRUE(url_loader_client_.response_body().is_valid());
+  ASSERT_TRUE(handler_->OnReadCompleted(0, &defer));
+  EXPECT_FALSE(defer);
+
+  net::URLRequestStatus status(net::URLRequestStatus::SUCCESS, net::OK);
+  handler_->OnResponseCompleted(status, "security_info", &defer);
+  EXPECT_FALSE(defer);
+
+  RunUntilNextNotification();
+  EXPECT_TRUE(url_loader_client_.has_received_completion());
+  EXPECT_EQ(net::OK, url_loader_client_.completion_status().error_code);
+
+  // This is needed because |*io_buffer| may keep the data producer alive.
+  io_buffer = nullptr;
+
+  while (true) {
+    char buffer[16];
+    uint32_t read = sizeof(buffer);
+    MojoResult result =
+        mojo::ReadDataRaw(url_loader_client_.response_body(), buffer, &read,
+                          MOJO_READ_DATA_FLAG_NONE);
+    if (result == MOJO_RESULT_FAILED_PRECONDITION)
+      break;
+    ASSERT_EQ(result, MOJO_RESULT_SHOULD_WAIT);
+  }
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, ResponseErrorDuringBodyTransmission) {

[mmenke, 2016/07/25 22:03:54]

Should we have a failure like this, but where the failure is without the
OnReadCompleted call?  I think that's a more interesting difference than with
the above test.

Could also do a cancellation in the middle of a completion deferal, and make
sure we stop listening to learn the pipe is ready for more data.

[yhirano, 2016/07/27 16:13:28]

Done.
BeginWriteFailsOnResume checks it, I think.

[mmenke, 2016/07/27 21:38:21]

I don't think it does - in that test, we fail when we try to get memory from
mojo.  What I'm thinking of is we get told to wait for memory to be available. 
Then the request is cancelled (For some external navigation).  We should make
sure we stop listening for the mojo ready notification.  Otherwise, if we get
one and start trying to read Bad Things may happen.

[yhirano, 2016/07/28 14:50:45]

Hmm, is CancelWhileWaiting what you want?

I have a question: Is it allowed to call OnResponseCompleted even when the
handler is in the deferred state (i.e., the last call set |*defer| true and
Resume has not been called since then)? It's not clear to me from
ResourceHandler's comments.

[mmenke, 2016/07/29 20:01:14]

Looks like it.
The ResourceHandler API is underspecified, and random ResourceHandlers do random
weird things, unfortunately.  Regardless of whether it's allowed or not, it
happens.  There's no code to hold back a cancellation message while waiting on a
deferred request, and we destroy requests after cancelling them when a renderer
goes away.  Other random things can cancel them at any point, too, so I don't
think it's safe to assume deletion happens synchronously after a cancellation.

+  scoped_refptr<ResourceResponse> response = new ResourceResponse();
+  bool defer = false;
+  ASSERT_TRUE(handler_->OnResponseStarted(response.get(), &defer));
+  EXPECT_FALSE(defer);
+  RunUntilNextNotification();
+  EXPECT_TRUE(url_loader_client_.has_received_response());
+
+  scoped_refptr<net::IOBuffer> io_buffer;
+  int buf_size;
+  ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+  RunUntilNextNotification();
+  ASSERT_TRUE(url_loader_client_.response_body().is_valid());
+  ASSERT_GT(buf_size, 0);
+  memset(io_buffer->data(), 'a', buf_size);
+  ASSERT_TRUE(handler_->OnReadCompleted(buf_size, &defer));
+
+  defer = false;
+  net::URLRequestStatus status(net::URLRequestStatus::FAILED, net::ERR_FAILED);
+  handler_->OnResponseCompleted(status, "security_info", &defer);
+  EXPECT_FALSE(defer);
+
+  RunUntilNextNotification();
+  EXPECT_TRUE(url_loader_client_.has_received_completion());
+  EXPECT_EQ(net::ERR_FAILED, url_loader_client_.completion_status().error_code);
+
+  // This is needed because |*io_buffer| may keep the data producer alive.
+  io_buffer = nullptr;
+
+  std::string actual;
+  while (true) {
+    char buf[16];
+    uint32_t read = sizeof(buf);
+    MojoResult result = mojo::ReadDataRaw(url_loader_client_.response_body(),
+                                          buf, &read, MOJO_READ_DATA_FLAG_NONE);
+    if (result == MOJO_RESULT_FAILED_PRECONDITION)
+      break;
+    if (result == MOJO_RESULT_SHOULD_WAIT)
+      continue;
+    EXPECT_EQ(MOJO_RESULT_OK, result);
+    actual.append(buf, read);
+  }
+  EXPECT_EQ(std::string(buf_size, 'a'), actual);
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, BeginWriteFailsOnWillRead) {
+  handler_->set_begin_write_expectation(MOJO_RESULT_UNKNOWN);
+  scoped_refptr<net::IOBuffer> io_buffer;
+  int buf_size = 0;
+  ASSERT_FALSE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+  EXPECT_FALSE(resource_controller_.is_cancel_with_error_called());
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, BeginWriteReturnsShouldWaitOnWillRead) {
+  handler_->set_begin_write_expectation(MOJO_RESULT_SHOULD_WAIT);

[mmenke, 2016/07/25 22:03:54]

Seems like we should simulate both the case where the handler is only ready
after the read completes, and the handler is ready before the read completes,
and make sure the consumer gets the data in both cases.

[yhirano, 2016/07/27 16:13:28]

Sorry, what do you mean by "handler is ready"?

[mmenke, 2016/07/27 16:49:36]

Erm...  the mojo channel thing is ready for data.

We try to get a buffer from mojo, mojo is tells us to come back later.  We then
read from the network stack into our own memory buffer buffer.  Then that
completes.

Now, we try to get a buffer from mojo again.  Mojo either gives us memory, or
tells us to come back later and we start watching mojo.  We should test both
cases.

[yhirano, 2016/07/28 14:50:46]

Added BeginWriteReturnsShouldWaitOnWillReadAndThenReturnsOK.

+  scoped_refptr<net::IOBuffer> io_buffer;
+  int buf_size = 0;
+  ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+  EXPECT_TRUE(io_buffer);
+  EXPECT_GT(buf_size, 0);
+}
+
+TEST_F(MojoAsyncResourceHandlerTest,
+       EndWriteFailsOnWillReadWithInsufficientInitialCapacity) {
+  MojoAsyncResourceHandler::SetAllocationSizeForTesting(2);
+  handler_->set_end_write_expectation(MOJO_RESULT_UNKNOWN);
+  scoped_refptr<net::IOBuffer> io_buffer;
+  int buf_size = 0;
+  ASSERT_FALSE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+}
+
+TEST_F(MojoAsyncResourceHandlerTest, EndWriteFailsOnReadCompleted) {
+  scoped_refptr<net::IOBuffer> io_buffer;
+  int buf_size = 0;
+  bool defer = false;
+  ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+
+  handler_->set_end_write_expectation(MOJO_RESULT_SHOULD_WAIT);
+  ASSERT_FALSE(handler_->OnReadCompleted(buf_size, &defer));
+}
+
+TEST_F(MojoAsyncResourceHandlerTest,
+       EndWriteFailsOnReadCompletedWithInsufficientInitialCapacity) {
+  MojoAsyncResourceHandler::SetAllocationSizeForTesting(2);
+  scoped_refptr<net::IOBuffer> io_buffer;
+  int buf_size = 0;
+  bool defer = false;
+  ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+
+  handler_->set_end_write_expectation(MOJO_RESULT_SHOULD_WAIT);
+  ASSERT_FALSE(handler_->OnReadCompleted(buf_size, &defer));
+}
+
+TEST_F(MojoAsyncResourceHandlerTest,
+       EndWriteFailsOnResumeWithInsufficientInitialCapacity) {
+  MojoAsyncResourceHandler::SetAllocationSizeForTesting(8);
+  scoped_refptr<net::IOBuffer> io_buffer;
+  int buf_size = 0;
+  ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+  RunUntilNextNotification();
+  ASSERT_TRUE(url_loader_client_.response_body().is_valid());
+
+  while (true) {
+    bool defer = false;
+    ASSERT_TRUE(handler_->OnReadCompleted(buf_size, &defer));
+    ASSERT_GE(buf_size, 0);
+    if (defer)
+      break;
+    ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+  }
+
+  while (true) {
+    char buf[16];
+    uint32_t read = sizeof(buf);
+    MojoResult result = mojo::ReadDataRaw(url_loader_client_.response_body(),
+                                          buf, &read, MOJO_READ_DATA_FLAG_NONE);
+    if (result == MOJO_RESULT_SHOULD_WAIT)
+      break;
+    ASSERT_EQ(MOJO_RESULT_OK, result);
+  }
+
+  handler_->set_end_write_expectation(MOJO_RESULT_SHOULD_WAIT);
+  RunUntilNextNotification();
+  EXPECT_FALSE(url_loader_client_.has_received_completion());
+  EXPECT_TRUE(resource_controller_.is_cancel_with_error_called());
+  EXPECT_EQ(net::ERR_FAILED, resource_controller_.error());
+}
+
+TEST_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
+       OnWillReadWithLongContents) {
+  bool defer = false;
+  scoped_refptr<net::IOBuffer> io_buffer;
+  int buf_size = 0;
+
+  ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+  ASSERT_TRUE(io_buffer);
+  // The io_buffer size that the mime sniffer requires implicitly.
+  ASSERT_GE(buf_size, 2 * 1024);
+  std::string expected;
+  for (int i = 0; i < 3 * buf_size + 2; ++i)
+    expected += ('A' + i % 26);
+
+  ASSERT_TRUE(handler_->OnReadCompleted(0, &defer));
+  ASSERT_FALSE(defer);
+
+  RunUntilNextNotification();
+  ASSERT_TRUE(url_loader_client_.response_body().is_valid());
+
+  size_t written = 0;
+  std::string actual;
+  while (actual.size() < expected.size()) {
+    while (written < expected.size() && !defer) {
+      ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+      const size_t to_be_written =
+          std::min(static_cast<size_t>(buf_size), expected.size() - written);
+      memcpy(io_buffer->data(), &expected[written], to_be_written);
+      ASSERT_TRUE(handler_->OnReadCompleted(to_be_written, &defer));
+      written += to_be_written;
+    }
+
+    char buf[16];
+    uint32_t read = sizeof(buf);
+    MojoResult result = mojo::ReadDataRaw(url_loader_client_.response_body(),
+                                          buf, &read, MOJO_READ_DATA_FLAG_NONE);
+    if (result != MOJO_RESULT_SHOULD_WAIT) {
+      ASSERT_EQ(MOJO_RESULT_OK, result);
+      actual.append(buf, read);
+    }
+    int resume_count = resource_controller_.num_resume_calls();
+    base::RunLoop().RunUntilIdle();
+    // Continue writing if controller->Resume() is called.
+    defer = (resume_count == resource_controller_.num_resume_calls());
+  }
+  EXPECT_EQ(expected, actual);
+}
+
+TEST_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
+       BeginWriteFailsOnReadCompleted) {
+  scoped_refptr<net::IOBuffer> io_buffer;
+  int buf_size = 0;
+  bool defer = false;
+  ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+
+  handler_->set_begin_write_expectation(MOJO_RESULT_UNKNOWN);
+  ASSERT_FALSE(handler_->OnReadCompleted(buf_size, &defer));
+}
+
+TEST_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
+       BeginWriteReturnsShouldWaitOnReadCompleted) {
+  scoped_refptr<net::IOBuffer> io_buffer;
+  int buf_size = 0;
+  bool defer = false;
+  ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+
+  handler_->set_begin_write_expectation(MOJO_RESULT_SHOULD_WAIT);
+  ASSERT_TRUE(handler_->OnReadCompleted(buf_size, &defer));
+  EXPECT_TRUE(defer);
+}
+
+TEST_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
+       BeginWriteFailsOnResume) {
+  bool defer = false;
+  int buf_size = 0;
+  scoped_refptr<net::IOBuffer> io_buffer;
+
+  ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+  ASSERT_TRUE(handler_->OnReadCompleted(0, &defer));
+  ASSERT_FALSE(defer);
+  RunUntilNextNotification();
+  ASSERT_TRUE(url_loader_client_.response_body().is_valid());
+
+  while (!defer) {
+    ASSERT_TRUE(handler_->OnWillRead(&io_buffer, &buf_size, -1));
+    ASSERT_TRUE(handler_->OnReadCompleted(buf_size, &defer));
+  }
+  handler_->set_begin_write_expectation(MOJO_RESULT_UNKNOWN);
+
+  while (!resource_controller_.is_cancel_with_error_called()) {
+    char buf[256];
+    uint32_t read = sizeof(buf);
+    MojoResult result = mojo::ReadDataRaw(url_loader_client_.response_body(),
+                                          buf, &read, MOJO_READ_DATA_FLAG_NONE);
+    ASSERT_TRUE(result == MOJO_RESULT_OK || result == MOJO_RESULT_SHOULD_WAIT);
+    base::RunLoop().RunUntilIdle();
+  }
+
+  EXPECT_FALSE(url_loader_client_.has_received_completion());
+  EXPECT_EQ(net::ERR_FAILED, resource_controller_.error());
+  EXPECT_EQ(0, resource_controller_.num_resume_calls());
+}
+
+INSTANTIATE_TEST_CASE_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
+                        MojoAsyncResourceHandlerWithAllocationSizeTest,
+                        ::testing::Values(8, 32 * 2014));
+}  // namespace
+}  // namespace content