Skip to content

Commit

Permalink
service worker: Add MainResourceRequestDestination UMA.
Browse files Browse the repository at this point in the history
ServicifiedServiceWorker has a lower count of main resource requests
that result in a fetch event dispatch. Add UMA to try to debug the
issue.

Bug: 866335
Change-Id: Iad5906f550722d073f0bfc6c788668aef18abd25
Reviewed-on: https://chromium-review.googlesource.com/1163353
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581129}
  • Loading branch information
mfalken authored and Commit Bot committed Aug 7, 2018
1 parent 510548e commit b145a45
Show file tree
Hide file tree
Showing 11 changed files with 211 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,36 @@ class ServiceWorkerControlleeRequestHandler::
DISALLOW_COPY_AND_ASSIGN(ScopedDisallowSetControllerRegistration);
};

class ServiceWorkerControlleeRequestHandler::MainResourceRequestTracker {
public:
MainResourceRequestTracker() = default;

~MainResourceRequestTracker() {
if (recorded_destination_)
return;
RecordDestination(
will_dispatch_fetch_
? ServiceWorkerMetrics::MainResourceRequestDestination::
kAbortedWhileDispatchingFetchEvent
: ServiceWorkerMetrics::MainResourceRequestDestination::
kAbortedWithoutDispatchingFetchEvent);
}

void RecordDestination(
ServiceWorkerMetrics::MainResourceRequestDestination destination) {
CHECK(!recorded_destination_);
recorded_destination_ = true;
ServiceWorkerMetrics::RecordMainResourceRequestDestination(destination);
}

void WillDispatchFetchEvent() { will_dispatch_fetch_ = true; }

private:
bool recorded_destination_ = false;
bool will_dispatch_fetch_ = false;
DISALLOW_COPY_AND_ASSIGN(MainResourceRequestTracker);
};

ServiceWorkerControlleeRequestHandler::ServiceWorkerControlleeRequestHandler(
base::WeakPtr<ServiceWorkerContextCore> context,
base::WeakPtr<ServiceWorkerProviderHost> provider_host,
Expand Down Expand Up @@ -333,6 +363,8 @@ void ServiceWorkerControlleeRequestHandler::PrepareForMainResource(
DCHECK(!JobWasCanceled());
DCHECK(context_);
DCHECK(provider_host_);
tracker_ = std::make_unique<MainResourceRequestTracker>();

TRACE_EVENT_ASYNC_BEGIN1(
"ServiceWorker",
"ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
Expand Down Expand Up @@ -368,6 +400,9 @@ void ServiceWorkerControlleeRequestHandler::
return;

if (status != blink::ServiceWorkerStatusCode::kOk) {
tracker_->RecordDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::
kNetworkBecauseNoRegistration);
url_job_->FallbackToNetwork();
TRACE_EVENT_ASYNC_END1(
"ServiceWorker",
Expand All @@ -378,6 +413,9 @@ void ServiceWorkerControlleeRequestHandler::
DCHECK(registration);

if (!provider_host_) {
tracker_->RecordDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::
kNetworkBecauseNoProvider);
url_job_->FallbackToNetwork();
TRACE_EVENT_ASYNC_END1(
"ServiceWorker",
Expand All @@ -388,6 +426,9 @@ void ServiceWorkerControlleeRequestHandler::
provider_host_->AddMatchingRegistration(registration.get());

if (!context_) {
tracker_->RecordDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::
kNetworkBecauseNoContext);
url_job_->FallbackToNetwork();
TRACE_EVENT_ASYNC_END1(
"ServiceWorker",
Expand All @@ -399,6 +440,9 @@ void ServiceWorkerControlleeRequestHandler::
if (!GetContentClient()->browser()->AllowServiceWorker(
registration->pattern(), provider_host_->topmost_frame_url(),
resource_context_, provider_host_->web_contents_getter())) {
tracker_->RecordDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::
kNetworkBecauseNotAllowed);
url_job_->FallbackToNetwork();
TRACE_EVENT_ASYNC_END1(
"ServiceWorker",
Expand All @@ -410,6 +454,9 @@ void ServiceWorkerControlleeRequestHandler::
if (!provider_host_->IsContextSecureForServiceWorker()) {
// TODO(falken): Figure out a way to surface in the page's DevTools
// console that the service worker was blocked for security.
tracker_->RecordDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::
kNetworkBecauseNotSecure);
url_job_->FallbackToNetwork();
TRACE_EVENT_ASYNC_END1(
"ServiceWorker",
Expand Down Expand Up @@ -441,6 +488,9 @@ void ServiceWorkerControlleeRequestHandler::
scoped_refptr<ServiceWorkerVersion> active_version =
registration->active_version();
if (!active_version) {
tracker_->RecordDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::
kNetworkBecauseNoActiveVersion);
url_job_->FallbackToNetwork();
TRACE_EVENT_ASYNC_END1(
"ServiceWorker",
Expand Down Expand Up @@ -481,6 +531,8 @@ void ServiceWorkerControlleeRequestHandler::
// The job may have been canceled before this was invoked. In that
// case, |url_job_| can't be used, so return.
if (JobWasCanceled()) {
tracker_->RecordDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::kJobWasCancelled);
TRACE_EVENT_ASYNC_END1(
"ServiceWorker",
"ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
Expand All @@ -489,6 +541,9 @@ void ServiceWorkerControlleeRequestHandler::
}

if (!provider_host_) {
tracker_->RecordDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::
kNetworkBecauseNoProviderAfterContinuing);
url_job_->FallbackToNetwork();
TRACE_EVENT_ASYNC_END1(
"ServiceWorker",
Expand Down Expand Up @@ -517,6 +572,9 @@ void ServiceWorkerControlleeRequestHandler::
// retries.
// 3) If the provider host does not have an active version, just fail the
// load.
tracker_->RecordDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::
kNetworkBecauseNoActiveVersionAfterContinuing);
url_job_->FallbackToNetwork();
TRACE_EVENT_ASYNC_END2(
"ServiceWorker",
Expand Down Expand Up @@ -547,6 +605,11 @@ void ServiceWorkerControlleeRequestHandler::
}
bool is_forwarded =
MaybeForwardToServiceWorker(url_job_.get(), active_version.get());
if (!is_forwarded) {
tracker_->RecordDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::
kNetworkBecauseNoFetchEventHandler);
}
TRACE_EVENT_ASYNC_END1(
"ServiceWorker",
"ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
Expand Down Expand Up @@ -681,6 +744,18 @@ void ServiceWorkerControlleeRequestHandler::MainResourceLoadFailed() {
provider_host_->NotifyControllerLost();
}

void ServiceWorkerControlleeRequestHandler::ReportDestination(
ServiceWorkerMetrics::MainResourceRequestDestination destination) {
DCHECK(is_main_resource_load_);
tracker_->RecordDestination(destination);
}

void ServiceWorkerControlleeRequestHandler::
WillDispatchFetchEventForMainResource() {
DCHECK(is_main_resource_load_);
tracker_->WillDispatchFetchEvent();
}

void ServiceWorkerControlleeRequestHandler::ClearJob() {
url_job_.reset();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler
FRIEND_TEST_ALL_PREFIXES(ServiceWorkerControlleeRequestHandlerTest,
ActivateWaitingVersion);
class ScopedDisallowSetControllerRegistration;
class MainResourceRequestTracker;

// For main resource case.
void PrepareForMainResource(const GURL& url, const GURL& site_for_cookies);
Expand Down Expand Up @@ -134,6 +135,9 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler
bool RequestStillValid(
ServiceWorkerMetrics::URLRequestJobResult* result) override;
void MainResourceLoadFailed() override;
void ReportDestination(ServiceWorkerMetrics::MainResourceRequestDestination
destination) override;
void WillDispatchFetchEventForMainResource() override;

// Sets |job_| to nullptr, and clears all extra response info associated with
// that job, except for timing information.
Expand Down Expand Up @@ -165,6 +169,8 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler
// next intercept opportunity, for main frame requests.
bool use_network_;

std::unique_ptr<MainResourceRequestTracker> tracker_;

base::WeakPtrFactory<ServiceWorkerControlleeRequestHandler> weak_factory_;

DISALLOW_COPY_AND_ASSIGN(ServiceWorkerControlleeRequestHandler);
Expand Down
6 changes: 6 additions & 0 deletions content/browser/service_worker/service_worker_metrics.cc
Original file line number Diff line number Diff line change
Expand Up @@ -951,4 +951,10 @@ void ServiceWorkerMetrics::RecordRegisteredOriginCount(size_t origin_count) {
UMA_HISTOGRAM_COUNTS_1M("ServiceWorker.RegisteredOriginCount", origin_count);
}

void ServiceWorkerMetrics::RecordMainResourceRequestDestination(
MainResourceRequestDestination destination) {
UMA_HISTOGRAM_ENUMERATION("ServiceWorker.MainResourceRequestDestination",
destination);
}

} // namespace content
43 changes: 43 additions & 0 deletions content/browser/service_worker/service_worker_metrics.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,46 @@ enum class EmbeddedWorkerStatus;

class ServiceWorkerMetrics {
public:
// Used for UMA. Append-only.
enum class MainResourceRequestDestination {
// The request was routed to the service worker. Fetch event dispatching
// possibly succeeded or failed.
// ServiceWorker.FetchEvent.MainResource.Status was logged with the result
// of the dispatch.
kServiceWorker = 0,

// The request was routed to network for the specified reason.
kNetworkBecauseNoActiveVersion = 1,
kNetworkBecauseNoActiveVersionAfterContinuing = 2,
kNetworkBecauseNoContext = 3,
kNetworkBecauseNoFetchEventHandler = 4,
kNetworkBecauseNoProvider = 5,
kNetworkBecauseNoProviderAfterContinuing = 6,
kNetworkBecauseNoRegistration = 7,
kNetworkBecauseNotAllowed = 8,
kNetworkBecauseNotSecure = 9,

// The loader couldn't dispatch the fetch event because there was no active
// worker.
kErrorNoActiveWorkerFromDelegate = 10,
// The loader couldn't dispatch the fetch event because the request body
// failed.
kErrorRequestBodyFailed = 11,

// The request was being routed to the service worker, but the handler was
// destroyed before the result of the fetch event dispatch was received.
kAbortedWhileDispatchingFetchEvent = 12,
// The handler was destroyed without dispatching a fetch event to the
// service
// worker.
kAbortedWithoutDispatchingFetchEvent = 13,

// The request was not routed because it was cancelled.
kJobWasCancelled = 14,

kMaxValue = 14,
};

// Used for UMA. Append-only.
enum ReadResponseResult {
READ_OK,
Expand Down Expand Up @@ -443,6 +483,9 @@ class ServiceWorkerMetrics {
// Records the number of origins with a registered service worker.
static void RecordRegisteredOriginCount(size_t origin_count);

static void RecordMainResourceRequestDestination(
MainResourceRequestDestination destination);

private:
DISALLOW_IMPLICIT_CONSTRUCTORS(ServiceWorkerMetrics);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,9 @@ void ServiceWorkerNavigationLoader::StartRequest() {
ServiceWorkerVersion* active_worker =
delegate_->GetServiceWorkerVersion(&result);
if (!active_worker) {
delegate_->ReportDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::
kErrorNoActiveWorkerFromDelegate);
ReturnNetworkError();
return;
}
Expand All @@ -193,6 +196,7 @@ void ServiceWorkerNavigationLoader::StartRequest() {
"there are. Add code here to clone the body before proceeding.";

// Dispatch the fetch event.
delegate_->WillDispatchFetchEventForMainResource();
fetch_dispatcher_ = std::make_unique<ServiceWorkerFetchDispatcher>(
std::move(request), std::string() /* request_body_blob_uuid */,
0 /* request_body_blob_size */, nullptr /* request_body_blob */,
Expand Down Expand Up @@ -291,6 +295,8 @@ void ServiceWorkerNavigationLoader::DidDispatchFetchEvent(
this, TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT, "status",
blink::ServiceWorkerStatusToString(status), "result",
ComposeFetchEventResultString(fetch_result, *response));
delegate_->ReportDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::kServiceWorker);
ServiceWorkerMetrics::RecordFetchEventStatus(true /* is_main_resource */,
status);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ class CONTENT_EXPORT ServiceWorkerNavigationLoader
void OnConnectionClosed();
void DeleteIfNeeded();

void ReportDestination(
ServiceWorkerMetrics::MainResourceRequestDestination destination);

ResponseType response_type_ = ResponseType::NOT_DETERMINED;
NavigationLoaderInterceptor::LoaderCallback loader_callback_;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ class CONTENT_EXPORT ServiceWorkerURLJobWrapper {
// Called to signal that loading failed, and that the resource being loaded
// was a main resource.
virtual void MainResourceLoadFailed() {}

virtual void ReportDestination(
ServiceWorkerMetrics::MainResourceRequestDestination destination) {}

virtual void WillDispatchFetchEventForMainResource() {}
};

// Non-S13nServiceWorker.
Expand Down
22 changes: 22 additions & 0 deletions content/browser/service_worker/service_worker_url_request_job.cc
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,7 @@ ServiceWorkerURLRequestJob::~ServiceWorkerURLRequestJob() {

if (!ShouldRecordResult())
return;

ServiceWorkerMetrics::URLRequestJobResult result =
ServiceWorkerMetrics::REQUEST_JOB_ERROR_KILLED;
if (response_body_type_ == STREAM)
Expand Down Expand Up @@ -668,6 +669,10 @@ void ServiceWorkerURLRequestJob::DidDispatchFetchEvent(
if (!did_navigation_preload_) {
fetch_dispatcher_.reset();
}
if (IsMainResourceLoad()) {
ReportDestination(
ServiceWorkerMetrics::MainResourceRequestDestination::kServiceWorker);
}
ServiceWorkerMetrics::RecordFetchEventStatus(IsMainResourceLoad(), status);

ServiceWorkerMetrics::URLRequestJobResult result =
Expand Down Expand Up @@ -950,6 +955,11 @@ void ServiceWorkerURLRequestJob::RequestBodyFileSizesResolved(bool success) {
if (!success) {
RecordResult(
ServiceWorkerMetrics::REQUEST_JOB_ERROR_REQUEST_BODY_BLOB_FAILED);
if (IsMainResourceLoad()) {
ReportDestination(ServiceWorkerMetrics::MainResourceRequestDestination::
kErrorRequestBodyFailed);
}

// TODO(falken): This and below should probably be NotifyStartError, not
// DeliverErrorResponse. But changing it causes
// ServiceWorkerURLRequestJobTest.DeletedProviderHostBeforeFetchEvent to
Expand All @@ -964,6 +974,10 @@ void ServiceWorkerURLRequestJob::RequestBodyFileSizesResolved(bool success) {
delegate_->GetServiceWorkerVersion(&result);
if (!active_worker) {
RecordResult(result);
if (IsMainResourceLoad()) {
ReportDestination(ServiceWorkerMetrics::MainResourceRequestDestination::
kErrorNoActiveWorkerFromDelegate);
}
DeliverErrorResponse();
return;
}
Expand All @@ -985,6 +999,8 @@ void ServiceWorkerURLRequestJob::RequestBodyFileSizesResolved(bool success) {
}

DCHECK(!fetch_dispatcher_);
if (IsMainResourceLoad())
delegate_->WillDispatchFetchEventForMainResource();
fetch_dispatcher_ = std::make_unique<ServiceWorkerFetchDispatcher>(
std::move(resource_request), blob_uuid, blob_size, std::move(blob),
client_id_, base::WrapRefCounted(active_worker), request()->net_log(),
Expand All @@ -1010,4 +1026,10 @@ void ServiceWorkerURLRequestJob::OnNavigationPreloadResponse() {
nav_preload_metrics_->ReportNavigationPreloadFinished();
}

void ServiceWorkerURLRequestJob::ReportDestination(
ServiceWorkerMetrics::MainResourceRequestDestination destination) {
DCHECK(IsMainResourceLoad());
delegate_->ReportDestination(destination);
}

} // namespace content
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,9 @@ class CONTENT_EXPORT ServiceWorkerURLRequestJob : public net::URLRequestJob {

void MaybeReportNavigationPreloadMetrics();

void ReportDestination(
ServiceWorkerMetrics::MainResourceRequestDestination destination);

// Not owned.
Delegate* delegate_;

Expand Down Expand Up @@ -337,6 +340,9 @@ class CONTENT_EXPORT ServiceWorkerURLRequestJob : public net::URLRequestJob {

std::unique_ptr<FileSizeResolver> file_size_resolver_;

bool started_fetch_dispatch_ = false;
bool reported_destination_ = false;

base::WeakPtrFactory<ServiceWorkerURLRequestJob> weak_factory_;

DISALLOW_COPY_AND_ASSIGN(ServiceWorkerURLRequestJob);
Expand Down
Loading

0 comments on commit b145a45

Please sign in to comment.