When InterceptingResourceHandler swaps in a handler, call OnWillStart.

The new ResourceHandler's OnWillStart was never being called, which
violates the ResourceHandler's API contract, and has caused at least one
crash previously (Though that fixed by another CL).

BUG=650717

Review-Url: https://codereview.chromium.org/2436163002
Cr-Commit-Position: refs/heads/master@{#427183}

content/browser/loader/intercepting_resource_handler.cc

@@ -8,6 +8,7 @@
 #include "base/strings/string_util.h"
 #include "content/public/common/resource_response.h"
 #include "net/base/io_buffer.h"
+#include "net/url_request/url_request.h"
 
 namespace content {
 
@@ -175,7 +176,10 @@ bool InterceptingResourceHandler::DoLoop(bool* defer) {
       case State::PASS_THROUGH:
         NOTREACHED();
         break;
-      case State::NOTIFYING_ON_RESPONSE_STARTED_TO_NEW_HANDLER:
+      case State::SENDING_ON_WILL_START_TO_NEW_HANDLER:
+        result = SendOnResponseStartedToNewHandler(defer);
+        break;
+      case State::SENDING_ON_RESPONSE_STARTED_TO_NEW_HANDLER:
         if (first_read_buffer_double_) {
           // OnWillRead has been called, so copying the data from
           // |first_read_buffer_double_| to |first_read_buffer_| will be needed
@@ -239,7 +243,13 @@ bool InterceptingResourceHandler::SendPayloadToOldHandler(bool* defer) {
   DCHECK(!*defer);
 
   next_handler_ = std::move(new_handler_);
-  state_ = State::NOTIFYING_ON_RESPONSE_STARTED_TO_NEW_HANDLER;
+  state_ = State::SENDING_ON_WILL_START_TO_NEW_HANDLER;
+  return next_handler_->OnWillStart(request()->url(), defer);
+}
+
+bool InterceptingResourceHandler::SendOnResponseStartedToNewHandler(
+    bool* defer) {
+  state_ = State::SENDING_ON_RESPONSE_STARTED_TO_NEW_HANDLER;
   return next_handler_->OnResponseStarted(response_.get(), defer);
 }
 

content/browser/loader/intercepting_resource_handler.h

@@ -81,11 +81,17 @@ class CONTENT_EXPORT InterceptingResourceHandler
     // Resume().
     SENDING_PAYLOAD_TO_OLD_HANDLER,
 
-    // The InterceptingResourcHandler is notifying OnResponseStarted to the new
-    // handler and waiting for its completion via Resume(). After the
-    // completion, the InterceptingResourcHandler will transit to
+    // The InterceptingResourcHandler is calling the new handler's
+    // OnResponseStarted method and waiting for its completion via Resume().
+    // After completion, the InterceptingResourceHandler will transition to
+    // SENDING_ON_RESPONSE_STARTED_TO_NEW_HANDLER on success.
+    SENDING_ON_WILL_START_TO_NEW_HANDLER,
+
+    // The InterceptingResourcHandler is calling the new handler's
+    // OnResponseStarted method and waiting for its completion via Resume().
+    // After completion, the InterceptingResourceHandler will transition to
     // WAITING_FOR_ON_READ_COMPLETED on success.
-    NOTIFYING_ON_RESPONSE_STARTED_TO_NEW_HANDLER,
+    SENDING_ON_RESPONSE_STARTED_TO_NEW_HANDLER,
 
     // The InterceptingResourcHandler is waiting for OnReadCompleted to be
     // called.
@@ -110,7 +116,7 @@ class CONTENT_EXPORT InterceptingResourceHandler
   // The return value and |defer| has the same meaning as DoLoop.
   bool SendPayloadToOldHandler(bool* defer);
   bool SendFirstReadBufferToNewHandler(bool* defer);
-  bool SendCompletionToOldHandler(bool* defer);
+  bool SendOnResponseStartedToNewHandler(bool* defer);
 
   State state_ = State::STARTING;
 

content/browser/loader/intercepting_resource_handler_unittest.cc

@@ -58,50 +58,8 @@ class TestResourceHandler : public ResourceHandler {
   // the handler and |body| will be updated on each OnReadCompleted call.
   explicit TestResourceHandler(net::URLRequestStatus* request_status,
                                std::string* body)
-      : TestResourceHandler(request_status,
-                            body,
-                            true /* on_response_started_result */,
-                            true /* on_will_read_result */,
-                            true /* on_read_completed_result */) {}
-
-  // This constructor allows to specify return values for OnResponseStarted,
-  // OnWillRead and OnReadCompleted.
-  TestResourceHandler(net::URLRequestStatus* request_status,
-                      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),
-        request_status_(request_status),
-        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),
-        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_);
+      : ResourceHandler(nullptr), request_status_(request_status), body_(body) {
+    SetBufferSize(2048);
   }
 
   ~TestResourceHandler() override {}
@@ -116,7 +74,11 @@ class TestResourceHandler : public ResourceHandler {
   }
 
   bool OnResponseStarted(ResourceResponse* response, bool* defer) override {
-    EXPECT_FALSE(is_completed_);
+    EXPECT_TRUE(will_start_called_);
+    EXPECT_FALSE(response_started_called_);
+    EXPECT_FALSE(response_completed_called_);
+    response_started_called_ = true;
+
     if (!on_response_started_result_)
       return false;
     *defer = defer_on_response_started_;
@@ -125,14 +87,23 @@ class TestResourceHandler : public ResourceHandler {
   }
 
   bool OnWillStart(const GURL& url, bool* defer) override {
-    EXPECT_FALSE(is_completed_);
+    EXPECT_FALSE(response_started_called_);
+    EXPECT_FALSE(will_start_called_);
+    EXPECT_FALSE(response_completed_called_);
+    will_start_called_ = true;
+
+    if (!on_will_start_result_)
+      return false;
+
+    *defer = defer_on_will_start_;
     return true;
   }
 
   bool OnWillRead(scoped_refptr<net::IOBuffer>* buf,
                   int* buf_size,
                   int min_size) override {
-    EXPECT_FALSE(is_completed_);
+    EXPECT_FALSE(response_completed_called_);
+
     *buf = buffer_;
     *buf_size = buffer_size_;
     memset(buffer_->data(), '\0', buffer_size_);
@@ -140,7 +111,10 @@ class TestResourceHandler : public ResourceHandler {
   }
 
   bool OnReadCompleted(int bytes_read, bool* defer) override {
-    EXPECT_FALSE(is_completed_);
+    EXPECT_TRUE(will_start_called_);
+    EXPECT_TRUE(response_started_called_);
+    EXPECT_FALSE(response_completed_called_);
+
     EXPECT_LE(static_cast<size_t>(bytes_read), buffer_size_);
     body_->append(buffer_->data(), bytes_read);
     if (!on_read_completed_result_)
@@ -152,8 +126,8 @@ class TestResourceHandler : public ResourceHandler {
 
   void OnResponseCompleted(const net::URLRequestStatus& status,
                            bool* defer) override {
-    EXPECT_FALSE(is_completed_);
-    is_completed_ = true;
+    EXPECT_FALSE(response_completed_called_);
+    response_completed_called_ = true;
     *request_status_ = status;
     *defer = defer_on_response_completed_;
     defer_on_response_completed_ = false;
@@ -163,18 +137,59 @@ class TestResourceHandler : public ResourceHandler {
 
   scoped_refptr<net::IOBuffer> buffer() const { return buffer_; }
 
+  // Sets the size of the read buffer returned by OnWillRead. Releases reference
+  // to previous read buffer.
+  void SetBufferSize(int buffer_size) {
+    buffer_ = new net::IOBuffer(buffer_size);
+    buffer_size_ = buffer_size;
+    memset(buffer_->data(), '\0', buffer_size);
+  }
+
+  void set_on_will_start_result(bool on_will_start_result) {
+    on_will_start_result_ = on_will_start_result;
+  }
+  void set_on_response_started_result(bool on_response_started_result) {
+    on_response_started_result_ = on_response_started_result;
+  }
+  void set_on_will_read_result(bool on_will_read_result) {
+    on_will_read_result_ = on_will_read_result;
+  }
+  void set_on_read_completed_result(bool on_read_completed_result) {
+    on_read_completed_result_ = on_read_completed_result;
+  }
+
+  void set_defer_on_will_start(bool defer_on_will_start) {
+    defer_on_will_start_ = defer_on_will_start;
+  }
+  void set_defer_on_response_started(bool defer_on_response_started) {
+    defer_on_response_started_ = defer_on_response_started;
+  }
+  void set_defer_on_read_completed(bool defer_on_read_completed) {
+    defer_on_read_completed_ = defer_on_read_completed;
+  }
+  void set_defer_on_response_completed(bool defer_on_response_completed) {
+    defer_on_response_completed_ = defer_on_response_completed;
+  }
+
  private:
   net::URLRequestStatus* request_status_;
   std::string* body_;
   scoped_refptr<net::IOBuffer> buffer_;
-  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;
+  size_t buffer_size_;
+
+  bool on_will_start_result_ = true;
+  bool on_response_started_result_ = true;
+  bool on_will_read_result_ = true;
+  bool on_read_completed_result_ = true;
+
+  bool defer_on_will_start_ = false;
+  bool defer_on_response_started_ = false;
+  bool defer_on_read_completed_ = false;
+  bool defer_on_response_completed_ = false;
+
+  bool will_start_called_ = false;
+  bool response_started_called_ = false;
+  bool response_completed_called_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(TestResourceHandler);
 };
@@ -453,10 +468,8 @@ TEST_F(InterceptingResourceHandlerTest, OldHandlerFailsWillRead) {
   net::URLRequestStatus old_handler_status;
   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
+      new TestResourceHandler(&old_handler_status, &old_handler_body));
+  old_handler->set_on_will_read_result(false);
   std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
       new InterceptingResourceHandler(std::move(old_handler), request.get()));
 
@@ -472,6 +485,61 @@ TEST_F(InterceptingResourceHandlerTest, OldHandlerFailsWillRead) {
   EXPECT_FALSE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1));
 }
 
+// Tests that the handler behaves properly if the new handler fails in
+// OnWillStart.
+TEST_F(InterceptingResourceHandlerTest, NewHandlerFailsOnWillStart) {
+  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;
+  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_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;
+  std::string new_handler_body;
+  std::unique_ptr<TestResourceHandler> new_handler(
+      new TestResourceHandler(&new_handler_status, &new_handler_body));
+  new_handler->set_on_will_start_result(false);
+  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 response
 // started.
 TEST_F(InterceptingResourceHandlerTest, NewHandlerFailsResponseStarted) {
@@ -518,10 +586,8 @@ TEST_F(InterceptingResourceHandlerTest, NewHandlerFailsResponseStarted) {
   net::URLRequestStatus new_handler_status;
   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
+      new TestResourceHandler(&new_handler_status, &new_handler_body));
+  new_handler->set_on_response_started_result(false);
   intercepting_handler->UseNewHandler(std::move(new_handler), std::string());
 
   // The response is received. The new ResourceHandler should tell us to fail.
@@ -574,10 +640,8 @@ TEST_F(InterceptingResourceHandlerTest, NewHandlerFailsWillRead) {
   net::URLRequestStatus new_handler_status;
   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
+      new TestResourceHandler(&new_handler_status, &new_handler_body));
+  new_handler->set_on_will_read_result(false);
   intercepting_handler->UseNewHandler(std::move(new_handler), std::string());
 
   // The response is received. The new handler should not have been asked to
@@ -639,10 +703,8 @@ TEST_F(InterceptingResourceHandlerTest, NewHandlerFailsReadCompleted) {
   net::URLRequestStatus new_handler_status;
   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
+      new TestResourceHandler(&new_handler_status, &new_handler_body));
+  new_handler->set_on_read_completed_result(false);
   intercepting_handler->UseNewHandler(std::move(new_handler), std::string());
 
   // The response is received.
@@ -682,14 +744,10 @@ TEST_F(InterceptingResourceHandlerTest, DeferredOperations) {
                                               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
+      new TestResourceHandler(&old_handler_status, &old_handler_body));
+  old_handler->SetBufferSize(10);
+  old_handler->set_defer_on_read_completed(true);
+
   scoped_refptr<net::IOBuffer> old_buffer = old_handler.get()->buffer();
   std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
       new InterceptingResourceHandler(std::move(old_handler), request.get()));
@@ -719,16 +777,16 @@ TEST_F(InterceptingResourceHandlerTest, DeferredOperations) {
   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
+      new TestResourceHandler(&new_handler_status, &new_handler_body));
+  new_handler->SetBufferSize(1);
+  new_handler->set_defer_on_will_start(true);
+  new_handler->set_defer_on_response_started(true);
+  new_handler->set_defer_on_read_completed(true);
+  new_handler->set_defer_on_response_completed(true);
   intercepting_handler->UseNewHandler(std::move(new_handler), kPayload);
-  // The response is received.
+
+  // The response is received, and then deferred by the old handler's
+  // OnReadCompleted method.
   ASSERT_TRUE(intercepting_handler->OnResponseStarted(response.get(), &defer));
   ASSERT_TRUE(defer);
 
@@ -739,6 +797,7 @@ TEST_F(InterceptingResourceHandlerTest, DeferredOperations) {
   EXPECT_EQ(old_handler_status.status(), net::URLRequestStatus::IO_PENDING);
   EXPECT_EQ(new_handler_status.status(), net::URLRequestStatus::IO_PENDING);
 
+  // Run until the new handler's OnWillStart method defers the request.
   intercepting_handler->Resume();
   EXPECT_EQ(0, resource_controller->resume_calls());
   EXPECT_EQ(kPayload, old_handler_body);
@@ -746,9 +805,18 @@ TEST_F(InterceptingResourceHandlerTest, DeferredOperations) {
   EXPECT_EQ(old_handler_status.status(), net::URLRequestStatus::SUCCESS);
   EXPECT_EQ(new_handler_status.status(), net::URLRequestStatus::IO_PENDING);
 
+  // Run until the new handler's OnResponseStarted method defers the request.
+  intercepting_handler->Resume();
+  EXPECT_EQ(0, resource_controller->resume_calls());
+  EXPECT_EQ(std::string(), new_handler_body);
+  EXPECT_EQ(old_handler_status.status(), net::URLRequestStatus::SUCCESS);
+  EXPECT_EQ(new_handler_status.status(), net::URLRequestStatus::IO_PENDING);
+
+  // Resuming should finally call back into the ResourceController.
   intercepting_handler->Resume();
   EXPECT_EQ(1, resource_controller->resume_calls());
 
+  // Data is read, the new handler defers completion of the read.
   defer = false;
   ASSERT_TRUE(intercepting_handler->OnReadCompleted(strlen(kData), &defer));
   ASSERT_TRUE(defer);
@@ -864,14 +932,10 @@ TEST_F(InterceptingResourceHandlerTest, CancelNewHandler) {
   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
-                              true,    // defer_on_response_started
-                              false,   // defer_on_read_completed
-                              true));  // defer_on_response_completed
+      new TestResourceHandler(&new_handler_status, &new_handler_body));
+  new_handler->SetBufferSize(1);
+  new_handler->set_defer_on_response_started(true);
+  new_handler->set_defer_on_response_completed(true);
   intercepting_handler->UseNewHandler(std::move(new_handler), kPayload);
 
   // The response is received.
@@ -914,14 +978,8 @@ TEST_F(InterceptingResourceHandlerTest, CancelBothHandlers) {
                                               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
-                              true,     // on_read_completed
-                              false,    // defer_on_response_started
-                              true,     // defer_on_read_completed
-                              false));  // defer_on_response_completed
+      new TestResourceHandler(&old_handler_status, &old_handler_body));
+  old_handler->set_defer_on_read_completed(true);
 
   std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
       new InterceptingResourceHandler(std::move(old_handler), request.get()));
@@ -949,14 +1007,9 @@ TEST_F(InterceptingResourceHandlerTest, CancelBothHandlers) {
   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
+      new TestResourceHandler(&new_handler_status, &new_handler_body));
+  new_handler->SetBufferSize(1);
+  new_handler->set_defer_on_response_completed(true);
   intercepting_handler->UseNewHandler(std::move(new_handler), kPayload);
 
   // The response is received.