Skip to content

Commit

Permalink
service worker: Remove ServiceWorkerNavigationLoader's fallback case.
Browse files Browse the repository at this point in the history 
This is a refactoring CL only. Before this CL, we created a
ServiceWorkerNavigationLoader always, and then called methods on it
about whether to forward to a service worker or fall back to network.
But there is no need to create the loader just to fall back to network.
This CL changes things to only create the loader once we decided to
forward to a service worker.

As for the linked bugs, much of the complexity was probably due to the
old non-S13nSW code path, and I encountered this while trying to move
things to the UI thread.

Bug: 926114, 824858
Change-Id: I320ebcd9401c283653bc58ce0c2049c62c1e5139
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1690572
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#675890}
  • Loading branch information
@mfalken
mfalken authored and Commit Bot committed Jul 10, 2019
1 parent 5f15de1 commit 46a16ef
Show file tree
Hide file tree
Showing 8 changed files with 194 additions and 418 deletions.
1 change: 0 additions & 1 deletion build/check_gn_headers_whitelist.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ components/viz/viz_export.h
components/wifi/wifi_export.h
components/wifi/wifi_service.h
content/browser/background_fetch/background_fetch_constants.h
content/browser/service_worker/service_worker_response_type.h
content/common/mac/attributed_string_coder.h
content/public/browser/context_factory.h
content/public/browser/media_observer.h
Expand Down
239 changes: 107 additions & 132 deletions content/browser/service_worker/service_worker_controllee_request_handler.cc
View file

Large diffs are not rendered by default.

14 changes: 9 additions & 5 deletions content/browser/service_worker/service_worker_controllee_request_handler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#define CONTENT_BROWSER_SERVICE_WORKER_SERVICE_WORKER_CONTROLLEE_REQUEST_HANDLER_H_

#include <stdint.h>

#include <memory>
#include <string>

Expand All @@ -18,6 +19,7 @@
#include "content/common/content_export.h"
#include "content/common/service_worker/service_worker_types.h"
#include "content/public/common/resource_type.h"
#include "services/network/public/cpp/resource_request.h"
#include "services/network/public/mojom/fetch_api.mojom.h"
#include "third_party/blink/public/mojom/fetch/fetch_api_request.mojom.h"
#include "url/gurl.h"
Expand Down Expand Up @@ -68,15 +70,12 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler final
ActivateWaitingVersion);
class ScopedDisallowSetControllerRegistration;

// TODO(falken): Remove the "MainResource" names, they are redundant as this
// handler is for main resources only.
void PrepareForMainResource(const GURL& url, const GURL& site_for_cookies);
void DidLookupRegistrationForMainResource(
void ContinueWithRegistration(
std::unique_ptr<ScopedDisallowSetControllerRegistration>
disallow_controller,
blink::ServiceWorkerStatusCode status,
scoped_refptr<ServiceWorkerRegistration> registration);
void ContinueWithInScopeMainResourceRequest(
void ContinueWithActivatedVersion(
scoped_refptr<ServiceWorkerRegistration> registration,
scoped_refptr<ServiceWorkerVersion> version,
std::unique_ptr<ScopedDisallowSetControllerRegistration>
Expand Down Expand Up @@ -105,6 +104,8 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler final
// that job, except for timing information.
void ClearJob();

void CompleteWithoutLoader();

// Schedules a service worker update to occur shortly after the page and its
// initial subresources load, if this handler was for a navigation.
void MaybeScheduleUpdate();
Expand All @@ -118,6 +119,9 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler final
bool force_update_started_;
base::TimeTicks registration_lookup_start_time_;

LoaderCallback loader_callback_;
FallbackCallback fallback_callback_;

base::WeakPtrFactory<ServiceWorkerControlleeRequestHandler> weak_factory_;

DISALLOW_COPY_AND_ASSIGN(ServiceWorkerControlleeRequestHandler);
Expand Down
80 changes: 30 additions & 50 deletions content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#include "content/browser/service_worker/service_worker_context_core.h"
#include "content/browser/service_worker/service_worker_provider_host.h"
#include "content/browser/service_worker/service_worker_registration.h"
#include "content/browser/service_worker/service_worker_response_type.h"
#include "content/browser/service_worker/service_worker_test_utils.h"
#include "content/common/service_worker/service_worker_types.h"
#include "content/public/browser/resource_context.h"
Expand Down Expand Up @@ -58,16 +57,17 @@ class ServiceWorkerControlleeRequestHandlerTest : public testing::Test {
test->provider_host_,
type)) {}

ServiceWorkerNavigationLoader* MaybeCreateLoader() {
void MaybeCreateLoader() {
network::ResourceRequest resource_request;
resource_request.url = request_->url();
resource_request.resource_type = static_cast<int>(resource_type_);
resource_request.headers = request()->extra_request_headers();
handler_->MaybeCreateLoader(resource_request, nullptr, nullptr,
base::DoNothing(), base::DoNothing());
return handler_->loader();
}

ServiceWorkerNavigationLoader* loader() { return handler_->loader(); }

void ResetHandler() { handler_.reset(nullptr); }

net::URLRequest* request() const { return request_.get(); }
Expand Down Expand Up @@ -185,16 +185,12 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, Basic) {
// Conduct a main resource load.
ServiceWorkerRequestTestResources test_resources(
this, GURL("https://host/scope/doc"), ResourceType::kMainFrame);
ServiceWorkerNavigationLoader* loader = test_resources.MaybeCreateLoader();

EXPECT_FALSE(loader->ShouldFallbackToNetwork());
EXPECT_FALSE(loader->ShouldForwardToServiceWorker());
EXPECT_FALSE(version_->HasControllee());
test_resources.MaybeCreateLoader();
EXPECT_FALSE(test_resources.loader());

base::RunLoop().RunUntilIdle();

EXPECT_FALSE(loader->ShouldFallbackToNetwork());
EXPECT_TRUE(loader->ShouldForwardToServiceWorker());
EXPECT_TRUE(test_resources.loader());
EXPECT_TRUE(version_->HasControllee());
histogram_tester.ExpectTotalCount(
"ServiceWorker.LookupRegistration.MainResource.Time.Exists", 1);
Expand All @@ -212,8 +208,8 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DoesNotExist) {
// Conduct a main resource load.
ServiceWorkerRequestTestResources test_resources(
this, GURL("https://host/scope/doc"), ResourceType::kMainFrame);
ServiceWorkerNavigationLoader* loader = test_resources.MaybeCreateLoader();
EXPECT_FALSE(loader);
test_resources.MaybeCreateLoader();
EXPECT_FALSE(test_resources.loader());

histogram_tester.ExpectTotalCount(
"ServiceWorker.LookupRegistration.MainResource.Time.DoesNotExist", 1);
Expand All @@ -233,8 +229,8 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, Error) {
// Conduct a main resource load.
ServiceWorkerRequestTestResources test_resources(
this, GURL("https://host/scope/doc"), ResourceType::kMainFrame);
ServiceWorkerNavigationLoader* loader = test_resources.MaybeCreateLoader();
EXPECT_FALSE(loader);
test_resources.MaybeCreateLoader();
EXPECT_FALSE(test_resources.loader());

histogram_tester.ExpectTotalCount(
"ServiceWorker.LookupRegistration.MainResource.Time.Error", 1);
Expand All @@ -259,17 +255,13 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DisallowServiceWorker) {
// Conduct a main resource load.
ServiceWorkerRequestTestResources test_resources(
this, GURL("https://host/scope/doc"), ResourceType::kMainFrame);
ServiceWorkerNavigationLoader* loader = test_resources.MaybeCreateLoader();
ASSERT_TRUE(loader);
test_resources.MaybeCreateLoader();
EXPECT_FALSE(test_resources.loader());

EXPECT_FALSE(loader->ShouldFallbackToNetwork());
EXPECT_FALSE(loader->ShouldForwardToServiceWorker());
EXPECT_FALSE(version_->HasControllee());
base::RunLoop().RunUntilIdle();

// Verify we did not use the worker.
EXPECT_TRUE(loader->ShouldFallbackToNetwork());
EXPECT_FALSE(loader->ShouldForwardToServiceWorker());
EXPECT_FALSE(test_resources.loader());
EXPECT_FALSE(version_->HasControllee());

SetBrowserClientForTesting(old_browser_client);
Expand All @@ -290,17 +282,13 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, InsecureContext) {
// Conduct a main resource load.
ServiceWorkerRequestTestResources test_resources(
this, GURL("https://host/scope/doc"), ResourceType::kMainFrame);
ServiceWorkerNavigationLoader* loader = test_resources.MaybeCreateLoader();
ASSERT_TRUE(loader);

EXPECT_FALSE(loader->ShouldFallbackToNetwork());
EXPECT_FALSE(loader->ShouldForwardToServiceWorker());
test_resources.MaybeCreateLoader();
EXPECT_FALSE(test_resources.loader());
EXPECT_FALSE(version_->HasControllee());
base::RunLoop().RunUntilIdle();

// Verify we did not use the worker.
EXPECT_TRUE(loader->ShouldFallbackToNetwork());
EXPECT_FALSE(loader->ShouldForwardToServiceWorker());
EXPECT_FALSE(test_resources.loader());
EXPECT_FALSE(version_->HasControllee());
}

Expand All @@ -317,18 +305,14 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, ActivateWaitingVersion) {
// Conduct a main resource load.
ServiceWorkerRequestTestResources test_resources(
this, GURL("https://host/scope/doc"), ResourceType::kMainFrame);
ServiceWorkerNavigationLoader* loader = test_resources.MaybeCreateLoader();
ASSERT_TRUE(loader);

EXPECT_FALSE(loader->ShouldFallbackToNetwork());
EXPECT_FALSE(loader->ShouldForwardToServiceWorker());
test_resources.MaybeCreateLoader();
EXPECT_FALSE(test_resources.loader());
EXPECT_FALSE(version_->HasControllee());

base::RunLoop().RunUntilIdle();

EXPECT_EQ(ServiceWorkerVersion::ACTIVATED, version_->status());
EXPECT_FALSE(loader->ShouldFallbackToNetwork());
EXPECT_TRUE(loader->ShouldForwardToServiceWorker());
EXPECT_TRUE(test_resources.loader());
EXPECT_TRUE(version_->HasControllee());

test_resources.ResetHandler();
Expand All @@ -346,15 +330,15 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, InstallingRegistration) {
// Conduct a main resource load.
ServiceWorkerRequestTestResources test_resources(
this, GURL("https://host/scope/doc"), ResourceType::kMainFrame);
ServiceWorkerNavigationLoader* job = test_resources.MaybeCreateLoader();
test_resources.MaybeCreateLoader();

base::RunLoop().RunUntilIdle();

// The handler should have fallen back to network and destroyed the job. The
// provider host should not be controlled. However it should add the
// registration as a matching registration so it can be used for .ready and
// claim().
EXPECT_FALSE(job);
EXPECT_FALSE(test_resources.loader());
EXPECT_FALSE(version_->HasControllee());
EXPECT_FALSE(provider_host_->controller());
EXPECT_EQ(registration_.get(), provider_host_->MatchRegistration());
Expand All @@ -377,19 +361,15 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DeletedProviderHost) {
// Conduct a main resource load.
ServiceWorkerRequestTestResources test_resources(
this, GURL("https://host/scope/doc"), ResourceType::kMainFrame);
ServiceWorkerNavigationLoader* loader = test_resources.MaybeCreateLoader();
ASSERT_TRUE(loader);

EXPECT_FALSE(loader->ShouldFallbackToNetwork());
EXPECT_FALSE(loader->ShouldForwardToServiceWorker());
test_resources.MaybeCreateLoader();
EXPECT_FALSE(test_resources.loader());

// Shouldn't crash if the ProviderHost is deleted prior to completion of
// the database lookup.
context()->RemoveProviderHost(provider_host_->provider_id());
EXPECT_FALSE(provider_host_.get());
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(loader->ShouldFallbackToNetwork());
EXPECT_FALSE(loader->ShouldForwardToServiceWorker());
EXPECT_FALSE(test_resources.loader());
}

#if BUILDFLAG(ENABLE_OFFLINE_PAGES)
Expand All @@ -409,9 +389,9 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, FallbackWithOfflineHeader) {
// Sets an offline header to indicate force loading offline page.
test_resources.request()->SetExtraRequestHeaderByName(
"X-Chrome-offline", "reason=download", true);
ServiceWorkerNavigationLoader* loader = test_resources.MaybeCreateLoader();

EXPECT_FALSE(loader);
test_resources.MaybeCreateLoader();
base::RunLoop().RunUntilIdle();
EXPECT_FALSE(test_resources.loader());
}

TEST_F(ServiceWorkerControlleeRequestHandlerTest, FallbackWithNoOfflineHeader) {
Expand All @@ -430,9 +410,9 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, FallbackWithNoOfflineHeader) {
// Empty offline header value should not cause fallback.
test_resources.request()->SetExtraRequestHeaderByName("X-Chrome-offline", "",
true);
ServiceWorkerNavigationLoader* loader = test_resources.MaybeCreateLoader();

EXPECT_TRUE(loader);
test_resources.MaybeCreateLoader();
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(test_resources.loader());
}
#endif // BUILDFLAG(ENABLE_OFFLINE_PAGE)

Expand Down
67 changes: 11 additions & 56 deletions content/browser/service_worker/service_worker_navigation_loader.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,34 +78,25 @@ class ServiceWorkerNavigationLoader::StreamWaiter
};

ServiceWorkerNavigationLoader::ServiceWorkerNavigationLoader(
NavigationLoaderInterceptor::LoaderCallback callback,
NavigationLoaderInterceptor::FallbackCallback fallback_callback,
Delegate* delegate,
const network::ResourceRequest& tentative_resource_request,
base::WeakPtr<ServiceWorkerProviderHost> provider_host,
scoped_refptr<URLLoaderFactoryGetter> url_loader_factory_getter)
: loader_callback_(std::move(callback)),
fallback_callback_(std::move(fallback_callback)),
: fallback_callback_(std::move(fallback_callback)),
delegate_(delegate),
provider_host_(std::move(provider_host)),
url_loader_factory_getter_(std::move(url_loader_factory_getter)),
binding_(this),
weak_factory_(this) {
TRACE_EVENT_WITH_FLOW1(
TRACE_EVENT_WITH_FLOW0(
"ServiceWorker",
"ServiceWorkerNavigationLoader::ServiceWorkerNavigationloader", this,
TRACE_EVENT_FLAG_FLOW_OUT, "url", tentative_resource_request.url.spec());
"ServiceWorkerNavigationLoader::ServiceWorkerNavigationLoader", this,
TRACE_EVENT_FLAG_FLOW_OUT);

DCHECK(delegate_);
DCHECK(ServiceWorkerUtils::IsMainResourceType(
static_cast<ResourceType>(tentative_resource_request.resource_type)));

response_head_.load_timing.request_start = base::TimeTicks::Now();
response_head_.load_timing.request_start_time = base::Time::Now();

// Beware that the final resource request may change due to throttles, so
// don't save |tentative_resource_request| here. We'll get the real one in
// StartRequest.
}

ServiceWorkerNavigationLoader::~ServiceWorkerNavigationLoader() {
Expand All @@ -115,44 +106,6 @@ ServiceWorkerNavigationLoader::~ServiceWorkerNavigationLoader() {
TRACE_EVENT_FLAG_FLOW_IN);
}

void ServiceWorkerNavigationLoader::FallbackToNetwork() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
TRACE_EVENT_WITH_FLOW0(
"ServiceWorker", "ServiceWorkerNavigationLoader::FallbackToNetwork", this,
TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT);
// ServiceWorkerControlleeRequestHandler only calls this if this loader never
// intercepted the request. Fallback to network after interception uses
// |fallback_callback_| instead.
DCHECK_EQ(response_type_, ResponseType::NOT_DETERMINED);
response_type_ = ResponseType::FALLBACK_TO_NETWORK;

TransitionToStatus(Status::kCompleted);

std::move(loader_callback_).Run({});
}

void ServiceWorkerNavigationLoader::ForwardToServiceWorker() {
TRACE_EVENT_WITH_FLOW0(
"ServiceWorker", "ServiceWorkerNavigationLoader::ForwardToServiceWorker",
this, TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT);
DCHECK_EQ(status_, Status::kNotStarted);
DCHECK_EQ(response_type_, ResponseType::NOT_DETERMINED);

response_type_ = ResponseType::FORWARD_TO_SERVICE_WORKER;

std::move(loader_callback_)
.Run(base::BindOnce(&ServiceWorkerNavigationLoader::StartRequest,
weak_factory_.GetWeakPtr()));
}

bool ServiceWorkerNavigationLoader::ShouldFallbackToNetwork() {
return response_type_ == ResponseType::FALLBACK_TO_NETWORK;
}

bool ServiceWorkerNavigationLoader::ShouldForwardToServiceWorker() {
return response_type_ == ResponseType::FORWARD_TO_SERVICE_WORKER;
}

void ServiceWorkerNavigationLoader::DetachedFromRequest() {
delegate_ = nullptr;
DeleteIfNeeded();
Expand All @@ -167,6 +120,13 @@ void ServiceWorkerNavigationLoader::StartRequest(
const network::ResourceRequest& resource_request,
network::mojom::URLLoaderRequest request,
network::mojom::URLLoaderClientPtr client) {
TRACE_EVENT_WITH_FLOW1("ServiceWorker",
"ServiceWorkerNavigationLoader::StartRequest", this,
TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT,
"url", resource_request.url.spec());
DCHECK(ServiceWorkerUtils::IsMainResourceType(
static_cast<ResourceType>(resource_request.resource_type)));

resource_request_ = resource_request;
if (provider_host_ && provider_host_->fetch_request_window_id()) {
resource_request_.fetch_window_id =
Expand All @@ -182,13 +142,8 @@ void ServiceWorkerNavigationLoader::StartRequest(
base::Unretained(this)));
url_loader_client_ = std::move(client);

DCHECK_EQ(ResponseType::FORWARD_TO_SERVICE_WORKER, response_type_);
TransitionToStatus(Status::kStarted);

TRACE_EVENT_WITH_FLOW0("ServiceWorker",
"ServiceWorkerNavigationLoader::StartRequest", this,
TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT);

ServiceWorkerVersion* active_worker = delegate_->GetServiceWorkerVersion();
if (!active_worker) {
CommitCompleted(net::ERR_FAILED, "No active worker");
Expand Down
Loading

0 comments on commit 46a16ef

Please sign in to comment.