Make ResourceHandler::OnWillRead able to complete asynchronously.

content/browser/loader/detachable_resource_handler_unittest.cc

+// Copyright 2017 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/detachable_resource_handler.h"
+
+#include <string>
+
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/weak_ptr.h"
+#include "base/single_thread_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "content/browser/loader/mock_resource_loader.h"
+#include "content/browser/loader/resource_controller.h"
+#include "content/browser/loader/test_resource_handler.h"
+#include "content/public/browser/resource_request_info.h"
+#include "content/public/common/resource_response.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "net/base/net_errors.h"
+#include "net/url_request/redirect_info.h"
+#include "net/url_request/url_request_context.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 "url/gurl.h"
+
+namespace content {
+
+namespace {
+
+// Full response body.
+const char kResponseBody[] = "Nifty response body.";
+// Two separate reads allow for testing cancellation in the middle of one read,
+// and between reads.
+const char kFirstBodyRead[] = "Nifty";
+const char kSecondBodyRead[] = " response body.";
+
+enum class DetachPhase {
+  DETACHED_FROM_CREATION,
+  ON_WILL_START,
+  REQUEST_REDIRECTED,
+  ON_RESPONSE_STARTED,
+  FIRST_ON_WILL_READ,
+  FIRST_ON_READ_COMPLETED,
+  SECOND_ON_WILL_READ,
+  SECOND_ON_READ_COMPLETED,
+  ON_READ_EOF,
+  ON_RESPONSE_COMPLETED,
+  NEVER_DETACH,
+};
+
+class DetachableResourceHandlerTest
+    : public testing::TestWithParam<DetachPhase> {

[Charlie Harrison, 2017/02/16 21:25:04]

Very clever

+ public:
+  DetachableResourceHandlerTest()
+      : request_(context_.CreateRequest(GURL("http://foo/"),
+                                        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
+                                            PREVIEWS_OFF);  // previews_state
+
+    std::unique_ptr<TestResourceHandler> test_handler;
+    if (GetParam() != DetachPhase::DETACHED_FROM_CREATION) {
+      test_handler = base::MakeUnique<TestResourceHandler>();
+      test_handler_ = test_handler->GetWeakPtr();
+    }
+    // TODO(mmenke):  This file currently has no timeout tests.  Should it?

[Charlie Harrison, 2017/02/16 21:25:04]

You have a bunch of double spaces here and in a few other places. git cl format
should (I think) fix this for you nowadays.

[mmenke, 2017/03/08 19:16:07]

It doesn't.  And you're supposed to use double-spaces with fixed-width fonts,
dangit!  My general feeling is this doesn't matter enough to be worth being
consistent - you'll see both single and double space comments all over the
network stack.  However, the header of this file uses single spaces, so I'll
switch.

The Google style guide used to explicitly allow both, now it's silent on the
subject, though some of its examples do use double spacing.

[Charlie Harrison, 2017/03/08 21:12:46]

OK. It doesn't matter to me as long as it's consistent. If net stack code uses
double spaces I don't care one way or another :)

+    detachable_handler_ = base::MakeUnique<DetachableResourceHandler>(
+        request_.get(), base::TimeDelta::FromMinutes(30),
+        std::move(test_handler));
+    mock_loader_ =
+        base::MakeUnique<MockResourceLoader>(detachable_handler_.get());
+  }
+
+ protected:
+  TestBrowserThreadBundle thread_bundle_;
+  net::TestURLRequestContext context_;
+  std::unique_ptr<net::URLRequest> request_;
+
+  base::WeakPtr<TestResourceHandler> test_handler_;
+
+  std::unique_ptr<DetachableResourceHandler> detachable_handler_;
+  std::unique_ptr<MockResourceLoader> mock_loader_;
+};
+
+// Tests where ResourceHandler completes synchronously.  Handler is detached
+// just before the phase indicated by the DetachPhase parameter.
+TEST_P(DetachableResourceHandlerTest, Sync) {
+  if (GetParam() == DetachPhase::ON_WILL_START) {

[Charlie Harrison, 2017/02/16 21:25:04]

One way to condense these tests would be to do something like:

void MaybeDetachAt(param) {
  if (GetParam() == param) {
    detachable_handler_->Detach();
    ASSERT_FALSE(test_handler_);
  }
}

bool WasDetachedBy(param) {
  if (GetParam() <= param) {
    EXPECT_FALSE(test_handler_);
    return true;
  }
  EXPECT_TRUE(test_handler_);
  return false;
}

then each leg of the test will be condensed to:

MaybeDetachAt(DetachPhase::ON_WILL_START);
ASSERT_EQ(MockResourceLoader::Status::IDLE,
          mock_loader_->OnWillStart(request_->url()));
if (!WasDetachedBy(DetachPhase::ON_WILL_START)) {
  EXPECT_EQ(1, test_handler_->on_will_start_called())
  EXPECT_EQ(0, test_handler_->on_request_redirected_called());
}


WDYT? I will have to look at async test to see if they can be combined...

[mmenke, 2017/03/08 19:16:06]

Done.  Not sure it makes the tests any easier to read, though.

[Charlie Harrison, 2017/03/08 21:12:45]

Thanks. For me these tests are now much easier to read.

+    detachable_handler_->Detach();
+    ASSERT_FALSE(test_handler_);
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE,
+            mock_loader_->OnWillStart(request_->url()));
+  if (GetParam() > DetachPhase::ON_WILL_START) {
+    EXPECT_EQ(1, test_handler_->on_will_start_called());
+    EXPECT_EQ(0, test_handler_->on_request_redirected_called());
+  } else {
+    EXPECT_FALSE(test_handler_);
+  }
+
+  if (GetParam() == DetachPhase::REQUEST_REDIRECTED) {
+    detachable_handler_->Detach();
+    ASSERT_FALSE(test_handler_);
+  }
+  ASSERT_EQ(
+      MockResourceLoader::Status::IDLE,
+      mock_loader_->OnRequestRedirected(
+          net::RedirectInfo(), make_scoped_refptr(new ResourceResponse())));
+  if (GetParam() > DetachPhase::REQUEST_REDIRECTED) {
+    EXPECT_EQ(1, test_handler_->on_request_redirected_called());
+    EXPECT_EQ(0, test_handler_->on_response_started_called());
+  } else {
+    EXPECT_FALSE(test_handler_);
+  }
+
+  if (GetParam() == DetachPhase::ON_RESPONSE_STARTED) {
+    detachable_handler_->Detach();
+    ASSERT_FALSE(test_handler_);
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE,
+            mock_loader_->OnResponseStarted(
+                make_scoped_refptr(new ResourceResponse())));
+  if (GetParam() > DetachPhase::ON_RESPONSE_STARTED) {
+    EXPECT_EQ(1, test_handler_->on_request_redirected_called());
+    EXPECT_EQ(1, test_handler_->on_response_started_called());
+    EXPECT_EQ(0, test_handler_->on_will_read_called());
+  } else {
+    EXPECT_FALSE(test_handler_);
+  }
+
+  if (GetParam() == DetachPhase::FIRST_ON_WILL_READ) {
+    detachable_handler_->Detach();
+    ASSERT_FALSE(test_handler_);
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
+  if (GetParam() > DetachPhase::FIRST_ON_WILL_READ) {
+    EXPECT_EQ(1, test_handler_->on_will_read_called());
+    EXPECT_EQ(0, test_handler_->on_read_completed_called());
+  } else {
+    EXPECT_FALSE(test_handler_);
+  }
+
+  if (GetParam() == DetachPhase::FIRST_ON_READ_COMPLETED) {
+    detachable_handler_->Detach();
+    ASSERT_FALSE(test_handler_);
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE,
+            mock_loader_->OnReadCompleted(kFirstBodyRead));
+  if (GetParam() > DetachPhase::FIRST_ON_READ_COMPLETED) {
+    EXPECT_EQ(1, test_handler_->on_read_completed_called());
+    EXPECT_EQ(kFirstBodyRead, test_handler_->body());
+  } else {
+    EXPECT_FALSE(test_handler_);
+  }
+
+  if (GetParam() == DetachPhase::SECOND_ON_WILL_READ) {
+    detachable_handler_->Detach();
+    ASSERT_FALSE(test_handler_);
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
+  if (GetParam() > DetachPhase::SECOND_ON_WILL_READ) {
+    EXPECT_EQ(2, test_handler_->on_will_read_called());
+    EXPECT_EQ(1, test_handler_->on_read_completed_called());
+  } else {
+    EXPECT_FALSE(test_handler_);
+  }
+
+  if (GetParam() == DetachPhase::SECOND_ON_READ_COMPLETED) {
+    detachable_handler_->Detach();
+    ASSERT_FALSE(test_handler_);
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE,
+            mock_loader_->OnReadCompleted(kSecondBodyRead));
+  if (GetParam() > DetachPhase::SECOND_ON_READ_COMPLETED) {
+    EXPECT_EQ(2, test_handler_->on_will_read_called());
+    EXPECT_EQ(2, test_handler_->on_read_completed_called());
+    EXPECT_EQ(kResponseBody, test_handler_->body());
+  } else {
+    EXPECT_FALSE(test_handler_);
+  }
+
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
+  if (GetParam() > DetachPhase::SECOND_ON_READ_COMPLETED) {
+    EXPECT_EQ(3, test_handler_->on_will_read_called());
+    EXPECT_EQ(2, test_handler_->on_read_completed_called());
+    EXPECT_EQ(0, test_handler_->on_response_completed_called());
+  } else {
+    EXPECT_FALSE(test_handler_);
+  }
+
+  if (GetParam() == DetachPhase::ON_READ_EOF) {
+    detachable_handler_->Detach();
+    ASSERT_FALSE(test_handler_);
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE,
+            mock_loader_->OnReadCompleted(""));
+  if (GetParam() > DetachPhase::ON_READ_EOF) {
+    EXPECT_EQ(3, test_handler_->on_read_completed_called());
+    EXPECT_EQ(1, test_handler_->on_read_eof_called());
+    EXPECT_EQ(0, test_handler_->on_response_completed_called());
+  } else {
+    EXPECT_FALSE(test_handler_);
+  }
+
+  if (GetParam() == DetachPhase::ON_RESPONSE_COMPLETED) {
+    detachable_handler_->Detach();
+    ASSERT_FALSE(test_handler_);
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE,
+            mock_loader_->OnResponseCompleted(
+                net::URLRequestStatus::FromError(net::OK)));
+  if (GetParam() > DetachPhase::ON_RESPONSE_COMPLETED) {
+    EXPECT_EQ(1, test_handler_->on_response_completed_called());
+    EXPECT_EQ(kResponseBody, test_handler_->body());
+  } else {
+    EXPECT_FALSE(test_handler_);
+  }
+}
+
+// Tests where ResourceHandler completes asynchronously. Handler is detached
+// during the phase indicated by the DetachPhase parameter. Async casees where

[Charlie Harrison, 2017/02/16 21:25:04]

s/casees/cases

[mmenke, 2017/03/08 19:16:07]

Done.

+// the handler is detached between phases are similar enough to the sync tests
+// that they wouldn't provide meaningfully better test coverage.
+//
+// Before the handler is detached, all calls complete asynchronously.
+// Afterwards, they all complete synchronously.
+TEST_P(DetachableResourceHandlerTest, Async) {
+  if (GetParam() != DetachPhase::DETACHED_FROM_CREATION) {
+    test_handler_->set_defer_on_will_start(true);
+    test_handler_->set_defer_on_request_redirected(true);
+    test_handler_->set_defer_on_response_started(true);
+    test_handler_->set_defer_on_will_read(true);
+    test_handler_->set_defer_on_read_completed(true);
+    test_handler_->set_defer_on_read_eof(true);
+    // Note:  Can't set |defer_on_response_completed|, since the
+    // DetachableResourceHandler DCHECKs when the next handler tried to defer
+    // the ERR_ABORTED message it sends downstream.
+  }
+
+  if (GetParam() < DetachPhase::ON_WILL_START) {
+    ASSERT_EQ(MockResourceLoader::Status::IDLE,
+              mock_loader_->OnWillStart(request_->url()));
+  } else {
+    ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+              mock_loader_->OnWillStart(request_->url()));
+    EXPECT_EQ(1, test_handler_->on_will_start_called());
+    EXPECT_EQ(0, test_handler_->on_request_redirected_called());
+    if (GetParam() == DetachPhase::ON_WILL_START) {
+      detachable_handler_->Detach();
+      ASSERT_FALSE(test_handler_);
+    } else {
+      test_handler_->Resume();
+    }
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
+
+  if (GetParam() < DetachPhase::REQUEST_REDIRECTED) {

[Charlie Harrison, 2017/02/16 21:25:04]

I don't think we could use the helper methods I described by the sync case, but
these could probably be condensed as well in a similar way. Up to you.

I think I would be happier if you pulled out the calls to the loader outside the
conditions, e.g.
auto status = mock_loader_->OnRequestRedirected(net::RedirectInfo(),
make_scoped_refptr(new ResourceResponse())));
if (GetParam() < REQUEST_REDIRECTED) {
  ASSERT_EQ(IDLE, status);
}
...

[mmenke, 2017/03/08 19:16:07]

Done.
Done.

+    EXPECT_FALSE(test_handler_);
+    ASSERT_EQ(
+        MockResourceLoader::Status::IDLE,
+        mock_loader_->OnRequestRedirected(
+            net::RedirectInfo(), make_scoped_refptr(new ResourceResponse())));
+  } else {
+    ASSERT_EQ(
+        MockResourceLoader::Status::CALLBACK_PENDING,
+        mock_loader_->OnRequestRedirected(
+            net::RedirectInfo(), make_scoped_refptr(new ResourceResponse())));
+    EXPECT_EQ(1, test_handler_->on_request_redirected_called());
+    EXPECT_EQ(0, test_handler_->on_response_started_called());
+    if (GetParam() == DetachPhase::REQUEST_REDIRECTED) {
+      detachable_handler_->Detach();
+      ASSERT_FALSE(test_handler_);
+    } else {
+      test_handler_->Resume();
+    }
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
+
+  if (GetParam() < DetachPhase::ON_RESPONSE_STARTED) {
+    EXPECT_FALSE(test_handler_);
+    ASSERT_EQ(MockResourceLoader::Status::IDLE,
+              mock_loader_->OnResponseStarted(
+                  make_scoped_refptr(new ResourceResponse())));
+  } else {
+    ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+              mock_loader_->OnResponseStarted(
+                  make_scoped_refptr(new ResourceResponse())));
+    EXPECT_EQ(1, test_handler_->on_request_redirected_called());
+    EXPECT_EQ(1, test_handler_->on_response_started_called());
+    EXPECT_EQ(0, test_handler_->on_will_read_called());
+    if (GetParam() == DetachPhase::ON_RESPONSE_STARTED) {
+      detachable_handler_->Detach();
+      ASSERT_FALSE(test_handler_);
+    } else {
+      test_handler_->Resume();
+    }
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
+
+  if (GetParam() < DetachPhase::FIRST_ON_WILL_READ) {
+    EXPECT_FALSE(test_handler_);
+    ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
+  } else {
+    ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+              mock_loader_->OnWillRead());
+    EXPECT_EQ(1, test_handler_->on_will_read_called());
+    EXPECT_EQ(0, test_handler_->on_read_completed_called());
+    if (GetParam() == DetachPhase::FIRST_ON_WILL_READ) {
+      detachable_handler_->Detach();
+      ASSERT_FALSE(test_handler_);
+    } else {
+      test_handler_->Resume();
+    }
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
+
+  if (GetParam() < DetachPhase::FIRST_ON_READ_COMPLETED) {
+    EXPECT_FALSE(test_handler_);
+    ASSERT_EQ(MockResourceLoader::Status::IDLE,
+              mock_loader_->OnReadCompleted(kFirstBodyRead));
+  } else {
+    ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+              mock_loader_->OnReadCompleted(kFirstBodyRead));
+    EXPECT_EQ(1, test_handler_->on_read_completed_called());
+    EXPECT_EQ(kFirstBodyRead, test_handler_->body());
+    if (GetParam() == DetachPhase::FIRST_ON_READ_COMPLETED) {
+      detachable_handler_->Detach();
+      ASSERT_FALSE(test_handler_);
+    } else {
+      test_handler_->Resume();
+    }
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
+
+  if (GetParam() < DetachPhase::SECOND_ON_WILL_READ) {
+    EXPECT_FALSE(test_handler_);
+    ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
+  } else {
+    test_handler_->set_defer_on_will_read(true);
+    ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+              mock_loader_->OnWillRead());
+    EXPECT_EQ(2, test_handler_->on_will_read_called());
+    EXPECT_EQ(1, test_handler_->on_read_completed_called());
+    if (GetParam() == DetachPhase::SECOND_ON_WILL_READ) {
+      detachable_handler_->Detach();
+      ASSERT_FALSE(test_handler_);
+    } else {
+      test_handler_->Resume();
+    }
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
+
+  if (GetParam() < DetachPhase::SECOND_ON_READ_COMPLETED) {
+    EXPECT_FALSE(test_handler_);
+    ASSERT_EQ(MockResourceLoader::Status::IDLE,
+              mock_loader_->OnReadCompleted(kSecondBodyRead));
+  } else {
+    test_handler_->set_defer_on_read_completed(true);
+    ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+              mock_loader_->OnReadCompleted(kSecondBodyRead));
+    EXPECT_EQ(2, test_handler_->on_will_read_called());
+    EXPECT_EQ(2, test_handler_->on_read_completed_called());
+    EXPECT_EQ(kResponseBody, test_handler_->body());
+    if (GetParam() == DetachPhase::SECOND_ON_READ_COMPLETED) {
+      detachable_handler_->Detach();
+      ASSERT_FALSE(test_handler_);
+    } else {
+      test_handler_->Resume();
+    }
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
+
+  // Test doesn't check detaching on thie third OnWillRead call.

[Charlie Harrison, 2017/02/16 21:25:04]

s/thie/the

[mmenke, 2017/03/08 19:16:07]

Done.

+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->OnWillRead());
+  if (GetParam() > DetachPhase::SECOND_ON_READ_COMPLETED) {
+    EXPECT_EQ(3, test_handler_->on_will_read_called());
+    EXPECT_EQ(2, test_handler_->on_read_completed_called());
+    EXPECT_EQ(0, test_handler_->on_response_completed_called());
+  } else {
+    EXPECT_FALSE(test_handler_);
+  }
+
+  if (GetParam() < DetachPhase::ON_READ_EOF) {
+    EXPECT_FALSE(test_handler_);
+    ASSERT_EQ(MockResourceLoader::Status::IDLE,
+              mock_loader_->OnReadCompleted(""));
+  } else {
+    test_handler_->set_defer_on_read_completed(true);
+    ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+              mock_loader_->OnReadCompleted(""));
+    EXPECT_EQ(3, test_handler_->on_read_completed_called());
+    EXPECT_EQ(1, test_handler_->on_read_eof_called());
+    EXPECT_EQ(0, test_handler_->on_response_completed_called());
+    if (GetParam() == DetachPhase::ON_READ_EOF) {
+      detachable_handler_->Detach();
+      ASSERT_FALSE(test_handler_);
+    } else {
+      test_handler_->Resume();
+    }
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
+
+  if (GetParam() < DetachPhase::ON_RESPONSE_COMPLETED) {
+    EXPECT_FALSE(test_handler_);
+    ASSERT_EQ(MockResourceLoader::Status::IDLE,
+              mock_loader_->OnResponseCompleted(
+                  net::URLRequestStatus::FromError(net::OK)));
+  } else {
+    test_handler_->set_defer_on_response_completed(true);
+    ASSERT_EQ(MockResourceLoader::Status::CALLBACK_PENDING,
+              mock_loader_->OnResponseCompleted(
+                  net::URLRequestStatus::FromError(net::OK)));
+    EXPECT_EQ(1, test_handler_->on_response_completed_called());
+    EXPECT_EQ(kResponseBody, test_handler_->body());
+    if (GetParam() == DetachPhase::ON_RESPONSE_COMPLETED) {
+      detachable_handler_->Detach();
+      ASSERT_FALSE(test_handler_);
+    } else {
+      test_handler_->Resume();
+    }
+  }
+  ASSERT_EQ(MockResourceLoader::Status::IDLE, mock_loader_->status());
+}
+
+INSTANTIATE_TEST_CASE_P(/* No prefix needed*/,
+                        DetachableResourceHandlerTest,
+                        testing::Values(DetachPhase::DETACHED_FROM_CREATION,
+                                        DetachPhase::ON_WILL_START,
+                                        DetachPhase::REQUEST_REDIRECTED,
+                                        DetachPhase::ON_RESPONSE_STARTED,
+                                        DetachPhase::FIRST_ON_WILL_READ,
+                                        DetachPhase::FIRST_ON_READ_COMPLETED,
+                                        DetachPhase::SECOND_ON_WILL_READ,
+                                        DetachPhase::SECOND_ON_READ_COMPLETED,
+                                        DetachPhase::ON_READ_EOF,
+                                        DetachPhase::ON_RESPONSE_COMPLETED,
+                                        DetachPhase::NEVER_DETACH));
+
+}  // namespace
+
+}  // namespace content