New issue
Advanced search Search tips

Issue 878667 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 715640



Sign in to add a comment

Fetch events for subresources won't be triggered once the service worker with DevTools requests termination

Project Member Reported by shimazu@chromium.org, Aug 29

Issue description

Context: https://chromium-review.googlesource.com/c/chromium/src/+/1195222#140

When a service worker becomes idle, ServiceWorkerTimeoutTimer requests the browser to terminate the service worker. If another fetch event is triggered to the service worker through SWSubresourceLoader, it's queued until the browser decides to terminate the worker (sending StopWorker message) or to keep the worker alive (just ignoring the termination request since there is inflight event in the Mojo pipe). 

However, when DevTool is attached to the worker, the message is silently ignored. In that case, there might be no inflight event. 
It results in indefinitely queued events.


 
Blocking: 715640
Labels: Target-70
Thanks for filing. Is it reasonable to target 70 for this? It seems it could result in strange behavior/skewed metrics.
Owner: shimazu@chromium.org
Yeah, that makes sense.
Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7e9a4a8064db0d57b77cb87e3af665bb4cd3c919

commit 7e9a4a8064db0d57b77cb87e3af665bb4cd3c919
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Wed Sep 05 05:55:40 2018

ServiceWorker: Send a result of RequestTermination back to the renderer

ServiceWorkerContextClient requests its termination when the worker gets idle.
However, when the browser knows DevTools is attached to the service worker, the
termination request is silently dropped. After that, even if
ServiceWorkerSubresourceLoader tries to trigger fetch events, those requests
will be queued indefinitely. This CL is to notify the renderer to run tasks
again when DevTools is attached.

Bug:  878667 
Change-Id: Iefdc4382489b68aae28aab57b5a7c96520795d14
Reviewed-on: https://chromium-review.googlesource.com/1194820
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588774}
[modify] https://crrev.com/7e9a4a8064db0d57b77cb87e3af665bb4cd3c919/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/7e9a4a8064db0d57b77cb87e3af665bb4cd3c919/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/7e9a4a8064db0d57b77cb87e3af665bb4cd3c919/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/7e9a4a8064db0d57b77cb87e3af665bb4cd3c919/content/browser/service_worker/service_worker_registration_unittest.cc
[modify] https://crrev.com/7e9a4a8064db0d57b77cb87e3af665bb4cd3c919/content/common/service_worker/embedded_worker.mojom
[modify] https://crrev.com/7e9a4a8064db0d57b77cb87e3af665bb4cd3c919/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/7e9a4a8064db0d57b77cb87e3af665bb4cd3c919/content/renderer/service_worker/service_worker_context_client.h

c#5 has landed in 71.0.3544.0. I'll request merge it to M70 after a few days.
Cc: swarnasree.mukkala@chromium.org shimazu@chromium.org susan.boorgula@chromium.org
 Issue 878422  has been merged into this issue.
Labels: Merge-Request-70
I'd like to request a merge of c#5 to M70.

It'll fix a bug where network requests from a page controlled by a service worker aren't handled when DevTools is attached. The bug affects debuggability a lot and it's worth merging it to M70.
So far I don't see crashes and alerts relevant to this.
Labels: -Merge-Request-70 Merge-Approved-70
Project Member

Comment 10 by sheriffbot@chromium.org, Sep 13

Cc: geo...@google.com
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 14

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f043ca4b1628b2002f241f3e6e8d81146857cda4

commit f043ca4b1628b2002f241f3e6e8d81146857cda4
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Fri Sep 14 04:23:55 2018

[Merge to M70] ServiceWorker: Send a result of RequestTermination back to the renderer

ServiceWorkerContextClient requests its termination when the worker gets idle.
However, when the browser knows DevTools is attached to the service worker, the
termination request is silently dropped. After that, even if
ServiceWorkerSubresourceLoader tries to trigger fetch events, those requests
will be queued indefinitely. This CL is to notify the renderer to run tasks
again when DevTools is attached.

Bug:  878667 
Change-Id: Iefdc4382489b68aae28aab57b5a7c96520795d14
Reviewed-on: https://chromium-review.googlesource.com/1194820
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#588774}(cherry picked from commit 7e9a4a8064db0d57b77cb87e3af665bb4cd3c919)
Reviewed-on: https://chromium-review.googlesource.com/1226535
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#398}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/f043ca4b1628b2002f241f3e6e8d81146857cda4/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/f043ca4b1628b2002f241f3e6e8d81146857cda4/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/f043ca4b1628b2002f241f3e6e8d81146857cda4/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/f043ca4b1628b2002f241f3e6e8d81146857cda4/content/browser/service_worker/service_worker_registration_unittest.cc
[modify] https://crrev.com/f043ca4b1628b2002f241f3e6e8d81146857cda4/content/common/service_worker/embedded_worker.mojom
[modify] https://crrev.com/f043ca4b1628b2002f241f3e6e8d81146857cda4/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/f043ca4b1628b2002f241f3e6e8d81146857cda4/content/renderer/service_worker/service_worker_context_client.h

Status: Fixed (was: Started)
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7bf1887dcf830f011222920693982db8f52ff9ad

commit 7bf1887dcf830f011222920693982db8f52ff9ad
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Wed Oct 10 06:13:13 2018

ServiceWorker: Triggers activation only when no event is handled

Currently we check only "HasNoWorkInBrowser()" when starting activation in
SWRegistration::IsReadyToActivate(). This CL is changing it to check tasks in
the renderer. Along with that, |idle_timer_fired_in_renderer_| kept in SWVersion
is renamed to |renderer_is_idle_| to make things clearer.

Bug:  879069 ,  878667 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ia57d5275af977d275f9a6eadabf82c97b74c2012
Reviewed-on: https://chromium-review.googlesource.com/c/1209134
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598221}
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/browser/service_worker/service_worker_job_unittest.cc
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/browser/service_worker/service_worker_navigation_loader_unittest.cc
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/browser/service_worker/service_worker_provider_host_unittest.cc
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/browser/service_worker/service_worker_registration.cc
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/browser/service_worker/service_worker_registration_unittest.cc
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/browser/service_worker/service_worker_url_request_job_unittest.cc
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/browser/service_worker/service_worker_version_unittest.cc
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/renderer/service_worker/service_worker_timeout_timer.cc
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/renderer/service_worker/service_worker_timeout_timer.h
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/content/renderer/service_worker/service_worker_timeout_timer_unittest.cc
[modify] https://crrev.com/7bf1887dcf830f011222920693982db8f52ff9ad/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService

Sign in to add a comment