Relax ad-hoc assumptions on InterceptingResourceHandler

content/browser/loader/intercepting_resource_handler_unittest.cc

 // 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/intercepting_resource_handler.h"
 
 #include <stdint.h>
 
 #include <memory>
+#include <string>
 
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/public/browser/resource_controller.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/common/resource_response.h"
 #include "content/public/common/webplugininfo.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "net/base/net_errors.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_status.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
 namespace content {
 
 namespace {
 
+class TestResourceController : public ResourceController {
+ public:
+  TestResourceController() = default;
+  void Cancel() override {}
+  void CancelAndIgnore() override {}
+  void CancelWithError(int error_code) override {}
+  void Resume() override { ++resume_calls_; }
+
+  int resume_calls() const { return resume_calls_; }
+
+ private:
+  int resume_calls_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(TestResourceController);
+};
+
 class TestResourceHandler : public ResourceHandler {
  public:
-  // A test version of a ResourceHandler. |request_status| and
-  // |final_bytes_read| will be updated when the response is complete with the
-  // final status of the request received by the handler, and the total bytes
-  // the handler saw.
+  // A test version of a ResourceHandler. |request_status| will be updated when
+  // the response is complete with the final status of the request received by
+  // the handler and |body| will be updated on each OnReadCompleted call.
   explicit TestResourceHandler(net::URLRequestStatus* request_status,
-                               size_t* final_bytes_read)
+                               std::string* body)
       : TestResourceHandler(request_status,
-                            final_bytes_read,
+                            body,
+                            2048 /* buffer_size */,
                             true /* on_response_started_result */,
                             true /* on_will_read_result */,
-                            true /* on_read_completed_result */) {}
+                            true /* on_read_completed_result */,
+                            false /* defer_on_response_started */,
+                            false /* defer_on_read_completed */,
+                            false /* defer_on_response_completed */) {}

[mmenke, 2016/10/12 20:10:46]

mime sniffer tests do:

on_blah_result
defer_on_blah
on_blump_result
defer_on_blump

etc.  Given how closely related the tests are, I don't think we should be using
a different pattern between the two...  On the other hand, this pattern does
make sense, if we want the 2 argument consumer.  This does seem like a painful
refactor, but as-is, working on both sets of tests seems really confusing. 
Maybe fix it in a followup, and make the two test fixtures share a
TestResourceHandler?

[mmenke, 2016/10/12 20:10:47]

Should this just call the next constructor down?

[yhirano, 2016/10/13 11:36:21]

Done.

[yhirano, 2016/10/13 11:36:22]

Added a TODO comment.

 
   // This constructor allows to specify return values for OnResponseStarted,
   // OnWillRead and OnReadCompleted.
   TestResourceHandler(net::URLRequestStatus* request_status,
-                      size_t* final_bytes_read,
+                      std::string* body,
                       bool on_response_started_result,
                       bool on_will_read_result,
                       bool on_read_completed_result)
+      : TestResourceHandler(request_status,
+                            body,
+                            2048 /* buffer_size */,
+                            on_response_started_result,
+                            on_will_read_result,
+                            on_read_completed_result,
+                            false /* defer_on_response_started */,
+                            false /* defer_on_response_completed */,
+                            false /* defer_on_read_completed */) {}
+
+  TestResourceHandler(net::URLRequestStatus* request_status,
+                      std::string* body,
+                      size_t buffer_size,
+                      bool on_response_started_result,
+                      bool on_will_read_result,
+                      bool on_read_completed_result,
+                      bool defer_on_response_started,
+                      bool defer_on_read_completed,
+                      bool defer_on_response_completed)
       : ResourceHandler(nullptr),
-        buffer_(new net::IOBuffer(2048)),
         request_status_(request_status),
-        final_bytes_read_(final_bytes_read),
+        body_(body),
+        buffer_(new net::IOBuffer(buffer_size)),
+        buffer_size_(buffer_size),
         on_response_started_result_(on_response_started_result),
         on_will_read_result_(on_will_read_result),
         on_read_completed_result_(on_read_completed_result),
-        bytes_read_(0),
-        is_completed_(false) {
-    memset(buffer_->data(), '\0', 2048);
+        defer_on_response_started_(defer_on_response_started),
+        defer_on_read_completed_(defer_on_read_completed),
+        defer_on_response_completed_(defer_on_response_completed) {
+    memset(buffer_->data(), '\0', buffer_size_);
   }
 
   ~TestResourceHandler() override {}
 
   void SetController(ResourceController* controller) override {}
 
   bool OnRequestRedirected(const net::RedirectInfo& redirect_info,
                            ResourceResponse* response,
                            bool* defer) override {
     NOTREACHED();
     return false;
   }
 
   bool OnResponseStarted(ResourceResponse* response, bool* defer) override {
     EXPECT_FALSE(is_completed_);
-    return on_response_started_result_;
+    if (!on_response_started_result_)
+      return false;
+    *defer = defer_on_response_started_;
+    defer_on_response_started_ = false;
+    return true;
   }
 
   bool OnWillStart(const GURL& url, bool* defer) override {
     EXPECT_FALSE(is_completed_);
     return true;
   }
 
   bool OnWillRead(scoped_refptr<net::IOBuffer>* buf,
                   int* buf_size,
                   int min_size) override {
     EXPECT_FALSE(is_completed_);
     *buf = buffer_;
-    *buf_size = 2048;
-    memset(buffer_->data(), '\0', 2048);
+    *buf_size = buffer_size_;
+    memset(buffer_->data(), '\0', buffer_size_);
     return on_will_read_result_;
   }
 
   bool OnReadCompleted(int bytes_read, bool* defer) override {
     EXPECT_FALSE(is_completed_);
-    EXPECT_LT(bytes_read, 2048);
-    bytes_read_ += bytes_read;
-    return on_read_completed_result_;
+    EXPECT_LE(static_cast<size_t>(bytes_read), buffer_size_);
+    body_->append(buffer_->data(), bytes_read);
+    if (!on_read_completed_result_)
+      return false;
+    *defer = defer_on_read_completed_;
+    defer_on_read_completed_ = false;
+    return true;
   }
 
   void OnResponseCompleted(const net::URLRequestStatus& status,
                            bool* defer) override {
     EXPECT_FALSE(is_completed_);
     is_completed_ = true;
     *request_status_ = status;
-    *final_bytes_read_ = bytes_read_;
+    *defer = defer_on_response_completed_;
+    defer_on_response_completed_ = false;
   }
 
   void OnDataDownloaded(int bytes_downloaded) override { NOTREACHED(); }
 
   scoped_refptr<net::IOBuffer> buffer() const { return buffer_; }
 
-  size_t bytes_read() const { return bytes_read_; }
-
  private:
+  net::URLRequestStatus* request_status_;
+  std::string* body_;
   scoped_refptr<net::IOBuffer> buffer_;
-  net::URLRequestStatus* request_status_;
-  size_t* final_bytes_read_;
-  bool on_response_started_result_;
-  bool on_will_read_result_;
-  bool on_read_completed_result_;
-  size_t bytes_read_;
-  bool is_completed_;
+  const size_t buffer_size_;
+  const bool on_response_started_result_;
+  const bool on_will_read_result_;
+  const bool on_read_completed_result_;
+  bool defer_on_response_started_;
+  bool defer_on_read_completed_;
+  bool defer_on_response_completed_;
+  bool is_completed_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(TestResourceHandler);
 };
 
 class InterceptingResourceHandlerTest : public testing::Test {
  public:
   InterceptingResourceHandlerTest() {}
 
  private:
   TestBrowserThreadBundle thread_bundle_;
@@ skipping 11 matching lines @@
                                           0,        // render_process_id
                                           0,        // render_view_id
                                           0,        // render_frame_id
                                           true,     // is_main_frame
                                           false,    // parent_is_main_frame
                                           true,     // allow_download
                                           true,     // is_async
                                           false);   // is_using_lofi
 
   net::URLRequestStatus old_handler_status;
-  size_t old_handler_final_bytes_read = 0;
-  std::unique_ptr<TestResourceHandler> old_handler(new TestResourceHandler(
-      &old_handler_status, &old_handler_final_bytes_read));
+  std::string old_handler_body;
+  std::unique_ptr<TestResourceHandler> old_handler(
+      new TestResourceHandler(&old_handler_status, &old_handler_body));
   TestResourceHandler* old_test_handler = old_handler.get();
   scoped_refptr<net::IOBuffer> old_buffer = old_handler.get()->buffer();
   std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
       new InterceptingResourceHandler(std::move(old_handler), request.get()));
 
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
 
   // Simulate the MimeSniffingResourceHandler buffering the data.
   scoped_refptr<net::IOBuffer> read_buffer;
   int buf_size = 0;
   bool defer = false;
   EXPECT_TRUE(intercepting_handler->OnWillStart(GURL(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1));
 
   const std::string kData = "The data";
   EXPECT_NE(kData, std::string(old_test_handler->buffer()->data()));
 
-  ASSERT_EQ(read_buffer.get(), old_test_handler->buffer());
+  ASSERT_NE(read_buffer.get(), old_test_handler->buffer());
   ASSERT_GT(static_cast<size_t>(buf_size), kData.length());
   memcpy(read_buffer->data(), kData.c_str(), kData.length());
 
   // The response is received. The handler should not change.
   EXPECT_TRUE(intercepting_handler->OnResponseStarted(response.get(), &defer));
   EXPECT_FALSE(defer);
 
   // The read is replayed by the MimeSniffingResourceHandler. The data should
   // have been received by the old intercepting_handler.
   EXPECT_TRUE(intercepting_handler->OnReadCompleted(kData.length(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_EQ(kData, std::string(old_test_handler->buffer()->data()));
 
   // Make sure another read behave as expected.
   buf_size = 0;
   EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1));
   ASSERT_EQ(read_buffer.get(), old_test_handler->buffer());
 
   const std::string kData2 = "Data 2";
   EXPECT_NE(kData, std::string(old_test_handler->buffer()->data()));
   ASSERT_GT(static_cast<size_t>(buf_size), kData2.length());
   memcpy(read_buffer->data(), kData2.c_str(), kData2.length());
 
   EXPECT_TRUE(intercepting_handler->OnReadCompleted(kData2.length(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_EQ(kData2, std::string(old_test_handler->buffer()->data()));
-  EXPECT_EQ(kData.length() + kData2.length(), old_test_handler->bytes_read());
+  EXPECT_EQ(kData + kData2, old_handler_body);
 }
 
 // Tests that the data received is transmitted to the newly created
 // ResourceHandler.
 TEST_F(InterceptingResourceHandlerTest, HandlerSwitchNoPayload) {
   net::URLRequestContext context;
   std::unique_ptr<net::URLRequest> request(context.CreateRequest(
       GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr));
   ResourceRequestInfo::AllocateForTesting(request.get(),
                                           RESOURCE_TYPE_MAIN_FRAME,
                                           nullptr,  // context
                                           0,        // render_process_id
                                           0,        // render_view_id
                                           0,        // render_frame_id
                                           true,     // is_main_frame
                                           false,    // parent_is_main_frame
                                           true,     // allow_download
                                           true,     // is_async
                                           false);   // is_using_lofi
 
   net::URLRequestStatus old_handler_status;
-  size_t old_handler_final_bytes_read = 0;
-  std::unique_ptr<TestResourceHandler> old_handler(new TestResourceHandler(
-      &old_handler_status, &old_handler_final_bytes_read));
+  std::string old_handler_body;
+  std::unique_ptr<TestResourceHandler> old_handler(
+      new TestResourceHandler(&old_handler_status, &old_handler_body));
   scoped_refptr<net::IOBuffer> old_buffer = old_handler.get()->buffer();
   std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
       new InterceptingResourceHandler(std::move(old_handler), request.get()));
 
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
 
   // Simulate the MimeSniffingResourceHandler buffering the data.
   scoped_refptr<net::IOBuffer> read_buffer;
   int buf_size = 0;
   bool defer = false;
   EXPECT_TRUE(intercepting_handler->OnWillStart(GURL(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1));
 
   const std::string kData = "The data";
-  ASSERT_EQ(read_buffer.get(), old_buffer.get());
+  ASSERT_NE(read_buffer.get(), old_buffer.get());
   ASSERT_GT(static_cast<size_t>(buf_size), kData.length());
   memcpy(read_buffer->data(), kData.c_str(), kData.length());
 
   // Simulate the MimeSniffingResourceHandler asking the
   // InterceptingResourceHandler to switch to a new handler.
   net::URLRequestStatus new_handler_status;
-  size_t new_handler_final_bytes_read = 0;
+  std::string new_handler_body;
   std::unique_ptr<TestResourceHandler> new_handler_scoped(
-      new TestResourceHandler(&new_handler_status,
-                              &new_handler_final_bytes_read));
+      new TestResourceHandler(&new_handler_status, &new_handler_body));
   TestResourceHandler* new_test_handler = new_handler_scoped.get();
   intercepting_handler->UseNewHandler(std::move(new_handler_scoped),
                                       std::string());
 
   // The response is received. The new ResourceHandler should be used handle
   // the download.
   EXPECT_TRUE(intercepting_handler->OnResponseStarted(response.get(), &defer));
   EXPECT_FALSE(defer);
 
   EXPECT_FALSE(old_handler_status.is_success());
   EXPECT_EQ(net::ERR_ABORTED, old_handler_status.error());
-  EXPECT_EQ(0ul, old_handler_final_bytes_read);
+  EXPECT_EQ(std::string(), old_handler_body);
 
   // It should not have received the download data yet.
   EXPECT_NE(kData, std::string(new_test_handler->buffer()->data()));
 
   // The read is replayed by the MimeSniffingResourceHandler. The data should
   // have been received by the new handler.
   EXPECT_TRUE(intercepting_handler->OnReadCompleted(kData.length(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_EQ(kData, std::string(new_test_handler->buffer()->data()));
 
   // Make sure another read behaves as expected.
   buf_size = 0;
   EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1));
   ASSERT_EQ(read_buffer.get(), new_test_handler->buffer());
   ASSERT_GT(static_cast<size_t>(buf_size), kData.length());
 
   const std::string kData2 = "Data 2";
   EXPECT_NE(kData2, std::string(new_test_handler->buffer()->data()));
   memcpy(read_buffer->data(), kData2.c_str(), kData2.length());
 
   EXPECT_TRUE(intercepting_handler->OnReadCompleted(kData2.length(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_EQ(kData2, std::string(new_test_handler->buffer()->data()));
-  EXPECT_EQ(kData.length() + kData2.length(), new_test_handler->bytes_read());
+  EXPECT_EQ(kData + kData2, new_handler_body);
 }
 
 // Tests that the data received is transmitted to the newly created
 // ResourceHandler and the specified payload to the old ResourceHandler.
 TEST_F(InterceptingResourceHandlerTest, HandlerSwitchWithPayload) {
   net::URLRequestContext context;
   std::unique_ptr<net::URLRequest> request(context.CreateRequest(
       GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr));
   ResourceRequestInfo::AllocateForTesting(request.get(),
                                           RESOURCE_TYPE_MAIN_FRAME,
                                           nullptr,  // context
                                           0,        // render_process_id
                                           0,        // render_view_id
                                           0,        // render_frame_id
                                           true,     // is_main_frame
                                           false,    // parent_is_main_frame
                                           true,     // allow_download
                                           true,     // is_async
                                           false);   // is_using_lofi
 
   net::URLRequestStatus old_handler_status;
-  size_t old_handler_final_bytes_read = 0;
+  std::string old_handler_body;
   std::unique_ptr<TestResourceHandler> old_handler_scoped(
-      new TestResourceHandler(&old_handler_status,
-                              &old_handler_final_bytes_read));
+      new TestResourceHandler(&old_handler_status, &old_handler_body));
   TestResourceHandler* old_handler = old_handler_scoped.get();
   scoped_refptr<net::IOBuffer> old_buffer = old_handler->buffer();
   std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
       new InterceptingResourceHandler(std::move(old_handler_scoped),
                                       request.get()));
 
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
 
   // Simulate the MimeSniffingResourceHandler buffering the data.
   scoped_refptr<net::IOBuffer> read_buffer;
   int buf_size = 0;
   bool defer = false;
   EXPECT_TRUE(intercepting_handler->OnWillStart(GURL(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1));
 
   const std::string kData = "The data";
-  ASSERT_EQ(read_buffer.get(), old_buffer.get());
+  ASSERT_NE(read_buffer.get(), old_buffer.get());
   ASSERT_GT(static_cast<size_t>(buf_size), kData.length());
   memcpy(read_buffer->data(), kData.c_str(), kData.length());
 
   // Simulate the MimeSniffingResourceHandler asking the
   // InterceptingResourceHandler to switch to a new handler.
   const std::string kPayload = "The payload";
   net::URLRequestStatus new_handler_status;
-  size_t new_handler_final_bytes_read = 0;
+  std::string new_handler_body;
   std::unique_ptr<TestResourceHandler> new_handler_scoped(
-      new TestResourceHandler(&new_handler_status,
-                              &new_handler_final_bytes_read));
+      new TestResourceHandler(&new_handler_status, &new_handler_body));
   TestResourceHandler* new_test_handler = new_handler_scoped.get();
   intercepting_handler->UseNewHandler(std::move(new_handler_scoped), kPayload);
 
   // The old handler should not have received the payload yet.
-  ASSERT_FALSE(old_handler->bytes_read());
+  ASSERT_EQ("", old_handler_body);

[mmenke, 2016/10/12 20:10:46]

std::string()?  I'm fine wither way, as long as we're consistent, but you're
using std::string() in a bunch of other tests in this file.

[yhirano, 2016/10/13 11:36:22]

Done.

   EXPECT_NE(kPayload, std::string(old_buffer->data()));

[mmenke, 2016/10/12 20:10:46]

Do we still need these old_buffer?  They seem redundant.

[yhirano, 2016/10/13 11:36:21]

Done.

 
   // The response is received. The new ResourceHandler should be used to handle
   // the download.
   EXPECT_TRUE(intercepting_handler->OnResponseStarted(response.get(), &defer));
   EXPECT_FALSE(defer);
 
   // The old handler should have received the payload.
-  ASSERT_TRUE(old_handler_final_bytes_read == kPayload.size());
+  EXPECT_EQ(kPayload, old_handler_body);
   EXPECT_EQ(kPayload, std::string(old_buffer->data()));
 
   EXPECT_TRUE(old_handler_status.is_success());
   EXPECT_EQ(net::OK, old_handler_status.error());
 
   // It should not have received the download data yet.
   EXPECT_NE(kData, std::string(new_test_handler->buffer()->data()));
 
   // The read is replayed by the MimeSniffingResourceHandler. The data should
   // have been received by the new handler.
   EXPECT_TRUE(intercepting_handler->OnReadCompleted(kData.length(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_EQ(kData, std::string(new_test_handler->buffer()->data()));
 
   // Make sure another read behave as expected.
   buf_size = 0;
   const std::string kData2 = "Data 2";
   EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1));
   ASSERT_EQ(read_buffer.get(), new_test_handler->buffer());
   ASSERT_GT(static_cast<size_t>(buf_size), kData.length());
 
   EXPECT_NE(kData2, std::string(new_test_handler->buffer()->data()));
   memcpy(read_buffer->data(), kData2.c_str(), kData2.length());
 
   EXPECT_TRUE(intercepting_handler->OnReadCompleted(kData2.length(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_EQ(kData2, std::string(new_test_handler->buffer()->data()));
-  EXPECT_EQ(kData.length() + kData2.length(), new_test_handler->bytes_read());
+  EXPECT_EQ(kData + kData2, new_handler_body);
 }
 
 // Tests that the handler behaves properly if the old handler fails will read.
 TEST_F(InterceptingResourceHandlerTest, OldHandlerFailsWillRead) {
   net::URLRequestContext context;
   std::unique_ptr<net::URLRequest> request(context.CreateRequest(
       GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr));
   ResourceRequestInfo::AllocateForTesting(request.get(),
                                           RESOURCE_TYPE_MAIN_FRAME,
                                           nullptr,  // context
                                           0,        // render_process_id
                                           0,        // render_view_id
                                           0,        // render_frame_id
                                           true,     // is_main_frame
                                           false,    // parent_is_main_frame
                                           true,     // allow_download
                                           true,     // is_async
                                           false);   // is_using_lofi
 
   net::URLRequestStatus old_handler_status;
-  size_t old_handler_final_bytes_read = 0;
-  std::unique_ptr<TestResourceHandler> old_handler(new TestResourceHandler(
-      &old_handler_status, &old_handler_final_bytes_read,
-      true,    // on_response_started
-      false,   // on_will_read
-      true));  // on_read_completed
+  std::string old_handler_body;
+  std::unique_ptr<TestResourceHandler> old_handler(
+      new TestResourceHandler(&old_handler_status, &old_handler_body,
+                              true,    // on_response_started
+                              false,   // on_will_read
+                              true));  // on_read_completed
   scoped_refptr<net::IOBuffer> old_buffer = old_handler.get()->buffer();
   std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
       new InterceptingResourceHandler(std::move(old_handler), request.get()));
 
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
 
   // Simulate the MimeSniffingResourceHandler buffering the data. The old
   // handler should tell the caller to fail.
   scoped_refptr<net::IOBuffer> read_buffer;
   int buf_size = 0;
@@ skipping 15 matching lines @@
                                           0,        // render_process_id
                                           0,        // render_view_id
                                           0,        // render_frame_id
                                           true,     // is_main_frame
                                           false,    // parent_is_main_frame
                                           true,     // allow_download
                                           true,     // is_async
                                           false);   // is_using_lofi
 
   net::URLRequestStatus old_handler_status;
-  size_t old_handler_final_bytes_read = 0;
-  std::unique_ptr<TestResourceHandler> old_handler(new TestResourceHandler(
-      &old_handler_status, &old_handler_final_bytes_read));
+  std::string old_handler_body;
+  std::unique_ptr<TestResourceHandler> old_handler(
+      new TestResourceHandler(&old_handler_status, &old_handler_body));
   scoped_refptr<net::IOBuffer> old_buffer = old_handler.get()->buffer();
   std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
       new InterceptingResourceHandler(std::move(old_handler), request.get()));
 
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
 
   // Simulate the MimeSniffingResourceHandler buffering the data.
   scoped_refptr<net::IOBuffer> read_buffer;
   int buf_size = 0;
   bool defer = false;
   EXPECT_TRUE(intercepting_handler->OnWillStart(GURL(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1));
 
   const char kData[] = "The data";
-  ASSERT_EQ(read_buffer.get(), old_buffer.get());
+  ASSERT_NE(read_buffer.get(), old_buffer.get());
   ASSERT_GT(static_cast<size_t>(buf_size), sizeof(kData));
   memcpy(read_buffer->data(), kData, sizeof(kData));
 
   // Simulate the MimeSniffingResourceHandler asking the
   // InterceptingResourceHandler to switch to a new handler.
   net::URLRequestStatus new_handler_status;
-  size_t new_handler_final_bytes_read = 0;
-  std::unique_ptr<TestResourceHandler> new_handler(new TestResourceHandler(
-      &new_handler_status, &new_handler_final_bytes_read,
-      false,   // on_response_started
-      true,    // on_will_read
-      true));  // on_read_completed
+  std::string new_handler_body;
+  std::unique_ptr<TestResourceHandler> new_handler(
+      new TestResourceHandler(&new_handler_status, &new_handler_body,
+                              false,   // on_response_started
+                              true,    // on_will_read
+                              true));  // on_read_completed
   intercepting_handler->UseNewHandler(std::move(new_handler), std::string());
 
   // The response is received. The new ResourceHandler should tell us to fail.
   EXPECT_FALSE(intercepting_handler->OnResponseStarted(response.get(), &defer));
   EXPECT_FALSE(defer);
 }
 
 // Tests that the handler behaves properly if the new handler fails will read.
 TEST_F(InterceptingResourceHandlerTest, NewHandlerFailsWillRead) {
   net::URLRequestContext context;
   std::unique_ptr<net::URLRequest> request(context.CreateRequest(
       GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr));
   ResourceRequestInfo::AllocateForTesting(request.get(),
                                           RESOURCE_TYPE_MAIN_FRAME,
                                           nullptr,  // context
                                           0,        // render_process_id
                                           0,        // render_view_id
                                           0,        // render_frame_id
                                           true,     // is_main_frame
                                           false,    // parent_is_main_frame
                                           true,     // allow_download
                                           true,     // is_async
                                           false);   // is_using_lofi
 
   net::URLRequestStatus old_handler_status;
-  size_t old_handler_final_bytes_read = 0;
-  std::unique_ptr<TestResourceHandler> old_handler(new TestResourceHandler(
-      &old_handler_status, &old_handler_final_bytes_read));
+  std::string old_handler_body;
+  std::unique_ptr<TestResourceHandler> old_handler(
+      new TestResourceHandler(&old_handler_status, &old_handler_body));
   scoped_refptr<net::IOBuffer> old_buffer = old_handler.get()->buffer();
   std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
       new InterceptingResourceHandler(std::move(old_handler), request.get()));
 
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
 
   // Simulate the MimeSniffingResourceHandler buffering the data.
   scoped_refptr<net::IOBuffer> read_buffer;
   int buf_size = 0;
   bool defer = false;
   EXPECT_TRUE(intercepting_handler->OnWillStart(GURL(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1));
 
   const char kData[] = "The data";
-  ASSERT_EQ(read_buffer.get(), old_buffer.get());
+  ASSERT_NE(read_buffer.get(), old_buffer.get());
   ASSERT_GT(static_cast<size_t>(buf_size), sizeof(kData));
   memcpy(read_buffer->data(), kData, sizeof(kData));
 
   // Simulate the MimeSniffingResourceHandler asking the
   // InterceptingResourceHandler to switch to a new handler.
   net::URLRequestStatus new_handler_status;
-  size_t new_handler_final_bytes_read = 0;
-  std::unique_ptr<TestResourceHandler> new_handler(new TestResourceHandler(
-      &new_handler_status, &new_handler_final_bytes_read,
-      true,    // on_response_started
-      false,   // on_will_read
-      true));  // on_read_completed
+  std::string new_handler_body;
+  std::unique_ptr<TestResourceHandler> new_handler(
+      new TestResourceHandler(&new_handler_status, &new_handler_body,
+                              true,    // on_response_started
+                              false,   // on_will_read
+                              true));  // on_read_completed
   intercepting_handler->UseNewHandler(std::move(new_handler), std::string());
 
   // The response is received. The new handler should not have been asked to
   // read yet.
   EXPECT_TRUE(intercepting_handler->OnResponseStarted(response.get(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_EQ(net::URLRequestStatus::CANCELED, old_handler_status.status());
   EXPECT_EQ(net::ERR_ABORTED, old_handler_status.error());
 
   // The read is replayed by the MimeSniffingResourceHandler. The new
@@ skipping 14 matching lines @@
                                           0,        // render_process_id
                                           0,        // render_view_id
                                           0,        // render_frame_id
                                           true,     // is_main_frame
                                           false,    // parent_is_main_frame
                                           true,     // allow_download
                                           true,     // is_async
                                           false);   // is_using_lofi
 
   net::URLRequestStatus old_handler_status;
-  size_t old_handler_final_bytes_read = 0;
-  std::unique_ptr<TestResourceHandler> old_handler(new TestResourceHandler(
-      &old_handler_status, &old_handler_final_bytes_read));
+  std::string old_handler_body;
+  std::unique_ptr<TestResourceHandler> old_handler(
+      new TestResourceHandler(&old_handler_status, &old_handler_body));
   scoped_refptr<net::IOBuffer> old_buffer = old_handler.get()->buffer();
   std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
       new InterceptingResourceHandler(std::move(old_handler), request.get()));
 
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
 
   // Simulate the MimeSniffingResourceHandler buffering the data.
   scoped_refptr<net::IOBuffer> read_buffer;
   int buf_size = 0;
   bool defer = false;
   EXPECT_TRUE(intercepting_handler->OnWillStart(GURL(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1));
 
   const char kData[] = "The data";
-  ASSERT_EQ(read_buffer.get(), old_buffer.get());
+  ASSERT_NE(read_buffer.get(), old_buffer.get());
   ASSERT_GT(static_cast<size_t>(buf_size), sizeof(kData));
   memcpy(read_buffer->data(), kData, sizeof(kData));
 
   // Simulate the MimeSniffingResourceHandler asking the
   // InterceptingResourceHandler to switch to a new handler.
   net::URLRequestStatus new_handler_status;
-  size_t new_handler_final_bytes_read = 0;
-  std::unique_ptr<TestResourceHandler> new_handler(new TestResourceHandler(
-      &new_handler_status, &new_handler_final_bytes_read,
-      true,     // on_response_started
-      true,     // on_will_read
-      false));  // on_read_completed
+  std::string new_handler_body;
+  std::unique_ptr<TestResourceHandler> new_handler(
+      new TestResourceHandler(&new_handler_status, &new_handler_body,
+                              true,     // on_response_started
+                              true,     // on_will_read
+                              false));  // on_read_completed
   intercepting_handler->UseNewHandler(std::move(new_handler), std::string());
 
   // The response is received.
   EXPECT_TRUE(intercepting_handler->OnResponseStarted(response.get(), &defer));
   EXPECT_FALSE(defer);
   EXPECT_EQ(net::URLRequestStatus::CANCELED, old_handler_status.status());
   EXPECT_EQ(net::ERR_ABORTED, old_handler_status.error());
 
   // The read is replayed by the MimeSniffingResourceHandler. The new handler
   // should tell the caller to fail.
   EXPECT_FALSE(intercepting_handler->OnReadCompleted(sizeof(kData), &defer));
   EXPECT_FALSE(defer);
 }
 
+// The old handler and the new handler set |defer| to true.

[mmenke, 2016/10/12 20:10:46]

I'd mention where they set it to true (old handler in OnReadCompleted, new one
in OnResponseStarted and OnReadCompleted)

[yhirano, 2016/10/13 11:36:21]

Done.

+TEST_F(InterceptingResourceHandlerTest, DeferredOperations) {
+  net::URLRequestContext context;
+  std::unique_ptr<net::URLRequest> request(context.CreateRequest(
+      GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr));
+  ResourceRequestInfo::AllocateForTesting(request.get(),
+                                          RESOURCE_TYPE_MAIN_FRAME,
+                                          nullptr,  // context
+                                          0,        // render_process_id
+                                          0,        // render_view_id
+                                          0,        // render_frame_id
+                                          true,     // is_main_frame
+                                          false,    // parent_is_main_frame
+                                          true,     // allow_download
+                                          true,     // is_async
+                                          false);   // is_using_lofi
+
+  std::unique_ptr<TestResourceController> resource_controller =
+      base::MakeUnique<TestResourceController>();
+  net::URLRequestStatus old_handler_status = {net::URLRequestStatus::IO_PENDING,
+                                              0};
+  std::string old_handler_body;
+  std::unique_ptr<TestResourceHandler> old_handler(
+      new TestResourceHandler(&old_handler_status, &old_handler_body,
+                              10,       // buffer_size
+                              true,     // on_response_started
+                              true,     // on_will_read
+                              true,     // on_read_completed
+                              false,    // defer_on_response_started
+                              true,     // defer_on_read_completed
+                              false));  // defer_on_response_completed
+  scoped_refptr<net::IOBuffer> old_buffer = old_handler.get()->buffer();
+  std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
+      new InterceptingResourceHandler(std::move(old_handler), request.get()));
+  intercepting_handler->SetController(resource_controller.get());
+
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+
+  // Simulate the MimeSniffingResourceHandler buffering the data.
+  scoped_refptr<net::IOBuffer> read_buffer;
+  int buf_size = 0;
+  bool defer = false;
+  EXPECT_TRUE(intercepting_handler->OnWillStart(GURL(), &defer));
+  EXPECT_FALSE(defer);
+  EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1));
+
+  const char kData[] = "The data";
+  ASSERT_NE(read_buffer.get(), old_buffer.get());
+  ASSERT_GT(static_cast<size_t>(buf_size), strlen(kData));
+  memcpy(read_buffer->data(), kData, strlen(kData));
+
+  // Simulate the MimeSniffingResourceHandler asking the
+  // InterceptingResourceHandler to switch to a new handler.
+  net::URLRequestStatus new_handler_status = {net::URLRequestStatus::IO_PENDING,
+                                              0};
+
+  std::string new_handler_body;
+  const std::string kPayload = "The long long long long long payload";
+  ASSERT_GT(kPayload.size(), static_cast<size_t>(buf_size));
+  std::unique_ptr<TestResourceHandler> new_handler(
+      new TestResourceHandler(&new_handler_status, &new_handler_body,
+                              1,       // buffer_size
+                              true,    // on_response_started
+                              true,    // on_will_read
+                              true,    // on_read_completed
+                              true,    // defer_on_response_started
+                              true,    // defer_on_read_completed
+                              true));  // defer_on_response_completed
+  intercepting_handler->UseNewHandler(std::move(new_handler), kPayload);
+  // The response is received.
+  ASSERT_TRUE(intercepting_handler->OnResponseStarted(response.get(), &defer));
+  ASSERT_TRUE(defer);
+
+  EXPECT_EQ("The long l", old_handler_body);

[mmenke, 2016/10/12 20:10:47]

Think this needs a comment - think that goes for most code blocks below this
point.

[yhirano, 2016/10/13 11:36:22]

Done.

+  EXPECT_EQ(std::string(), new_handler_body);
+
+  intercepting_handler->Resume();
+  EXPECT_EQ(0, resource_controller->resume_calls());
+  EXPECT_EQ(kPayload, old_handler_body);
+  EXPECT_EQ(std::string(), new_handler_body);
+
+  intercepting_handler->Resume();
+  EXPECT_EQ(1, resource_controller->resume_calls());
+  EXPECT_EQ(kPayload, old_handler_body);
+  EXPECT_EQ(std::string(), new_handler_body);
+
+  defer = false;
+  ASSERT_TRUE(intercepting_handler->OnReadCompleted(strlen(kData), &defer));
+  ASSERT_TRUE(defer);
+
+  EXPECT_EQ(kPayload, old_handler_body);
+  EXPECT_EQ("T", new_handler_body);
+
+  intercepting_handler->Resume();
+  EXPECT_EQ(2, resource_controller->resume_calls());
+  EXPECT_EQ(kPayload, old_handler_body);
+  EXPECT_EQ(kData, new_handler_body);
+
+  EXPECT_EQ(old_handler_status.status(), net::URLRequestStatus::SUCCESS);

[mmenke, 2016/10/12 20:10:46]

Should we check this two blocks up, when it's actually set?

[yhirano, 2016/10/13 11:36:22]

Done.

+  EXPECT_EQ(new_handler_status.status(), net::URLRequestStatus::IO_PENDING);
+
+  defer = false;
+  intercepting_handler->OnResponseCompleted({net::URLRequestStatus::SUCCESS, 0},
+                                            &defer);
+  ASSERT_TRUE(defer);
+  EXPECT_EQ(old_handler_status.status(), net::URLRequestStatus::SUCCESS);
+  EXPECT_EQ(new_handler_status.status(), net::URLRequestStatus::SUCCESS);
+}

[mmenke, 2016/10/12 20:10:47]

Per comment, I'd like to see two cancellation (UnexpectedOnResponseCompleted
called) tests (I don't think we have them yet)

One while the old handler is deferring reading the response, one while the new
handler is deferring OnResponseStarted.  Main thing to check is the lower layer
resource handlers gets informed of the call.  Full correct behavior depends on
the next ResourceHandler implementation, which we can't check.

[yhirano, 2016/10/13 11:36:21]

I don't fully understand when we need to expect OnResponseCompleted. Let me
check.

1. I'm sure once a ResourceHandler receives OnResponseCompleted it will never
receive any message including OnResponseCompleted. That means deferred
OnResponseCompleted cannot be canceled.

2. I'm not sure if it's ok to call OnResponseCompleted on a deferred
ResourceHandler.

If 2 is true (i.e., we need to expect OnResponseCompleted when deferred), I
think it's difficult to throw away the assumption that the old handler's
OnResponseComplete never sets |defer| which I removed between PS10 - PS11 based
on [A]. The old handler's OnResonseCompleted cannot be canceled, so we need to
take care of it in InterceptingResourceHandler, which needs more states.
I'm also worried about the cancellation timing: Currently we fix the order of
OnResponseCompleted (PS10 called the old handler's OnResponseCompleted first,
PS12 calls the new handler's OnResponseCompleted first). But we need to call
deferred ResourceHandler's OnResponseCompleted immediately - otherwise it will
be very hard to control the deferred state.

In short, If 2 is true, we need that assumption, I believe.

WDYT?

A:
https://codereview.chromium.org/2327463002/diff/240001/content/browser/loader...

[mmenke, 2016/10/13 17:35:47]

This is true.  I meant two tests with cancellation, not a test with two
cancellations.
This does happen.  There's nothing stopping a request from being cancelled
during the OnResponseCompleted call.  It could be another ResourceHandler that
does it (Even if it's not the ResourceHandler's turn, we have nothing to prevent
it), or there could be something else that happens.  If we enter suspend mode,
for instance, we'll get a notification of cancellation, or if the renderer
suddenly cancels the request (Maybe due to a new navigation?), we'll see a
cancel in the middle of a deferred OnResponseCompleted call.

It's precisely this case (And the same for OnReadCompleted) that I want to test,
because it's a weird case.
I'm not following this.
I don't understand what you mean by "calling deferred ResourceHandler's
OnResponseCompleted".

[yhirano, 2016/10/14 04:06:04]

I wrote slides for explanation:
https://docs.google.com/a/chromium.org/presentation/d/1eZa6oOaU-BoRAqLB3JN3jU...

+
+TEST_F(InterceptingResourceHandlerTest, DeferredCompletion) {
+  net::URLRequestContext context;
+  std::unique_ptr<net::URLRequest> request(context.CreateRequest(
+      GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr));
+  ResourceRequestInfo::AllocateForTesting(request.get(),
+                                          RESOURCE_TYPE_MAIN_FRAME,
+                                          nullptr,  // context
+                                          0,        // render_process_id
+                                          0,        // render_view_id
+                                          0,        // render_frame_id
+                                          true,     // is_main_frame
+                                          false,    // parent_is_main_frame
+                                          true,     // allow_download
+                                          true,     // is_async
+                                          false);   // is_using_lofi
+
+  std::unique_ptr<TestResourceController> resource_controller =
+      base::MakeUnique<TestResourceController>();
+  net::URLRequestStatus old_handler_status = {net::URLRequestStatus::IO_PENDING,
+                                              0};
+  std::string old_handler_body;
+  std::unique_ptr<TestResourceHandler> old_handler(
+      new TestResourceHandler(&old_handler_status, &old_handler_body,
+                              2048,     // buffer_size
+                              true,     // on_response_started
+                              true,     // on_will_read
+                              false,    // on_read_completed
+                              false,    // defer_on_response_started
+                              false,    // defer_on_read_completed
+                              false));  // defer_on_response_completed
+
+  std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
+      new InterceptingResourceHandler(std::move(old_handler), request.get()));
+  intercepting_handler->SetController(resource_controller.get());
+
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+
+  // Simulate the MimeSniffingResourceHandler buffering the data.
+  scoped_refptr<net::IOBuffer> read_buffer;
+  int buf_size = 0;
+  bool defer = false;
+  EXPECT_TRUE(intercepting_handler->OnWillStart(GURL(), &defer));
+  EXPECT_FALSE(defer);
+  EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1));
+
+  const char kData[] = "The data";
+  ASSERT_GT(static_cast<size_t>(buf_size), strlen(kData));
+  memcpy(read_buffer->data(), kData, strlen(kData));
+
+  // Simulate the MimeSniffingResourceHandler asking the
+  // InterceptingResourceHandler to switch to a new handler.
+  net::URLRequestStatus new_handler_status = {net::URLRequestStatus::IO_PENDING,
+                                              0};
+
+  std::string new_handler_body;
+  const std::string kPayload = "The payload";
+  std::unique_ptr<TestResourceHandler> new_handler(
+      new TestResourceHandler(&new_handler_status, &new_handler_body,
+                              1,       // buffer_size
+                              true,    // on_response_started
+                              true,    // on_will_read
+                              true,    // on_read_completed
+                              false,   // defer_on_response_started
+                              false,   // defer_on_read_completed
+                              true));  // defer_on_response_completed
+  intercepting_handler->UseNewHandler(std::move(new_handler), kPayload);
+
+  // The response is received.
+  ASSERT_FALSE(intercepting_handler->OnResponseStarted(response.get(), &defer));
+  ASSERT_FALSE(defer);
+
+  EXPECT_EQ(net::URLRequestStatus::IO_PENDING, old_handler_status.status());
+  EXPECT_EQ(net::URLRequestStatus::IO_PENDING, new_handler_status.status());
+
+  intercepting_handler->OnResponseCompleted(
+      {net::URLRequestStatus::CANCELED, net::ERR_FAILED}, &defer);
+  ASSERT_TRUE(defer);
+  EXPECT_EQ(0, resource_controller->resume_calls());
+  EXPECT_EQ(net::URLRequestStatus::CANCELED, old_handler_status.status());
+  EXPECT_EQ(net::URLRequestStatus::CANCELED, new_handler_status.status());
+}
+
 }  // namespace
 
 }  // namespace content