During activation, the exiting worker's in-flight requests should be handled somehow |
|||||
Issue descriptionWe either should: 1) wait for the requests to finish, or 2) terminate the worker and redispatch to the activating worker. The spec says to wait, but there may be some room for interpretation about what an in-flight request is (did browser send to renderer-side, is it added to the worker thread's event queue, is the worker handling the event?). We also need to decide what to do about new incoming requests. They can eiher be: 1) dispatched to the exiting worker 2) dispatched to the activating worker, once it's activated The spec seems to say they go to the activating worker.
,
Jun 18 2016
Spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/916
,
Jun 18 2016
The spec discussion is about the "waiting for events to finish" thing. Another thing we can do, which might alleviate broken page loads, is change the update trigger from "1 second after the last resource request finished" to "1 second after DOMContentLoaded". This would match the current spec. The current trigger is brittle since a slow network might not finish any requests within 1 second.
,
Jun 18 2016
Does the Inbox problem trigger in Firefox? That might be one way to tell if the domcontentloaded approach would help or not before implementing it. I don't have an Inbox account unfortunately.
,
Jun 18 2016
Unfortunately I don't have a reliable repro, just bug reports. I just realized DOMContentLoaded fires before images and other subresources finished loading, so it seems vulnerable to this issue too.
,
Jun 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0276ebb1eae96b10ade52b189ecbaa01ffc8009f commit 0276ebb1eae96b10ade52b189ecbaa01ffc8009f Author: falken <falken@chromium.org> Date: Mon Jun 20 08:25:47 2016 Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb Previously Doom() called StopIfIdle() rather than Stop(). Now (as of https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. There are two callsites that set status to REDUNDANT directly. This patch changes one to call Stop() first, as per spec. It's not necessary to call Doom() because the new version has already been stored which puts the old version's resources in the purged list. This is minor cleanup in anticipation of some changes for https://crbug.com/616331 BUG= 616331 , 448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html Review-Url: https://codereview.chromium.org/2077843002 Cr-Commit-Position: refs/heads/master@{#400636} [modify] https://crrev.com/0276ebb1eae96b10ade52b189ecbaa01ffc8009f/content/browser/service_worker/service_worker_register_job.cc [modify] https://crrev.com/0276ebb1eae96b10ade52b189ecbaa01ffc8009f/content/browser/service_worker/service_worker_version.cc
,
Jun 24 2016
Regarding Firefox, I made a kind of silly repro case (a worker that does setTimeout(10000) in onfetch for subresource requests) and could see broken images in both Chrome and Firefox.
,
Jun 30 2016
The spec issues are basically settled: https://github.com/slightlyoff/ServiceWorker/issues/916 One thing I'm concerned about implementation-wise is if we wait for in-flight requests to finish, we don't want to get stuck waiting for a fetch event that won't complete because the page closed. In fact we have an existing bug here. When the page closes, the URLRequest gets aborted, and SWURLRequestJob gets killed/destructed, so SWFetchDispatcher gets killed, so the fetch event callback never gets invoked. This means SWVersion treats it as an in-flight request until the 5 minute timeout kicks in. (Regardless of activation, that means we incorrectly terminate a worker that ever had a URLRequest get cancelled.) My plan is to first update callsites of StartRequest to ensure that FinishRequest is called.
,
Jul 4 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9ed9574ae81390911dd582a0d5a49a4c0430eafa commit 9ed9574ae81390911dd582a0d5a49a4c0430eafa Author: falken <falken@chromium.org> Date: Mon Jul 04 03:51:52 2016 service worker: Clean up the fetch event request if URLRequest gets cancelled. Before this patch, an inflight fetch event would never be finished, since the request being cancelled results in the URLRequestJob being destroyed, so the the response callback is never called. Eventually the event would timeout, but this is an error state that terminates the worker. It's better to listen for the response and finish the request cleanly. BUG= 616331 Review-Url: https://codereview.chromium.org/2114893002 Cr-Commit-Position: refs/heads/master@{#403632} [modify] https://crrev.com/9ed9574ae81390911dd582a0d5a49a4c0430eafa/content/browser/service_worker/service_worker_fetch_dispatcher.cc [modify] https://crrev.com/9ed9574ae81390911dd582a0d5a49a4c0430eafa/content/browser/service_worker/service_worker_fetch_dispatcher.h [modify] https://crrev.com/9ed9574ae81390911dd582a0d5a49a4c0430eafa/content/browser/service_worker/service_worker_url_request_job_unittest.cc [modify] https://crrev.com/9ed9574ae81390911dd582a0d5a49a4c0430eafa/content/browser/service_worker/service_worker_version.h
,
Jul 4 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6c74874b9c6969a2afc6f195c96a1a3b15cda890 commit 6c74874b9c6969a2afc6f195c96a1a3b15cda890 Author: falken <falken@chromium.org> Date: Mon Jul 04 08:48:36 2016 service worker: Refactor OnNoControllees OnNoControllees doesn't need to unset the flags. * If |is_uninstalling_| is set, the call to Clear() would unset it. * If |should_activate_when_ready_| is set, the call to ActivateWaitingVersion() would unset it. We also change Clear() to unset |should_activate_when_ready_| which makes sense for any callsite of Clear(). Minor simplification in anticipation of changes for https://crbug.com/616331 Refactoring only; no behavior change. BUG= 616331 Review-Url: https://codereview.chromium.org/2115393002 Cr-Commit-Position: refs/heads/master@{#403650} [modify] https://crrev.com/6c74874b9c6969a2afc6f195c96a1a3b15cda890/content/browser/service_worker/service_worker_registration.cc
,
Jul 12 2016
Patch under review here: https://codereview.chromium.org/2119143002/
,
Jul 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6c95b0ff8f694cda918e38c96387e5445713fc8e commit 6c95b0ff8f694cda918e38c96387e5445713fc8e Author: falken <falken@chromium.org> Date: Thu Jul 14 23:39:52 2016 service worker: Wait for inflight requests before activating This patch makes changes as per discussion at https://github.com/slightlyoff/ServiceWorker/issues/916 Namely, * The waiting worker gets promoted to active once there are no controllees and the incumbent worker has no in-flight events (fetch/sync/push/message etc). * skipWaiting bypasses the no controllees requirement, but we still wait until there are no in-flight events. * As usual, incoming fetch/sync/push events always go to the active worker. BUG= 616331 Review-Url: https://codereview.chromium.org/2119143002 Cr-Commit-Position: refs/heads/master@{#405628} [modify] https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e/content/browser/service_worker/service_worker_registration.cc [modify] https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e/content/browser/service_worker/service_worker_registration.h [modify] https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e/content/browser/service_worker/service_worker_registration_unittest.cc [modify] https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e/content/browser/service_worker/service_worker_storage.h [modify] https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e/content/browser/service_worker/service_worker_url_request_job_unittest.cc [modify] https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e/content/browser/service_worker/service_worker_version.cc [modify] https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e/content/browser/service_worker/service_worker_version.h [add] https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e/third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html [add] https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/mint-new-worker.php
,
Jul 20 2016
Requesting merge to M53 for: https://crrev.com/9ed9574ae81390911dd582a0d5a49a4c0430eafa https://crrev.com/6c74874b9c6969a2afc6f195c96a1a3b15cda890 https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e This fixes a major issue for a large site. These all are baked on canary with no problems.
,
Jul 20 2016
Your change meets the bar and is auto-approved for M53 (branch: 2785)
,
Jul 21 2016
Merge to M53 is complete.
,
Jul 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/802c9e886637e00ee90de9b139c233e26f0f56af commit 802c9e886637e00ee90de9b139c233e26f0f56af Author: Matt Falkenhagen <falken@chromium.org> Date: Thu Jul 21 05:42:53 2016 [M53] service worker: Clean up the fetch event request if URLRequest gets cancelled. Before this patch, an inflight fetch event would never be finished, since the request being cancelled results in the URLRequestJob being destroyed, so the the response callback is never called. Eventually the event would timeout, but this is an error state that terminates the worker. It's better to listen for the response and finish the request cleanly. BUG= 616331 Review-Url: https://codereview.chromium.org/2114893002 Cr-Commit-Position: refs/heads/master@{#403632} (cherry picked from commit 9ed9574ae81390911dd582a0d5a49a4c0430eafa) Review URL: https://codereview.chromium.org/2168013002 . Cr-Commit-Position: refs/branch-heads/2785@{#256} Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382} [modify] https://crrev.com/802c9e886637e00ee90de9b139c233e26f0f56af/content/browser/service_worker/service_worker_fetch_dispatcher.cc [modify] https://crrev.com/802c9e886637e00ee90de9b139c233e26f0f56af/content/browser/service_worker/service_worker_fetch_dispatcher.h [modify] https://crrev.com/802c9e886637e00ee90de9b139c233e26f0f56af/content/browser/service_worker/service_worker_url_request_job_unittest.cc [modify] https://crrev.com/802c9e886637e00ee90de9b139c233e26f0f56af/content/browser/service_worker/service_worker_version.h
,
Jul 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/09a50f12f80cb14568479cf33b10d55cdb6a54a2 commit 09a50f12f80cb14568479cf33b10d55cdb6a54a2 Author: Matt Falkenhagen <falken@chromium.org> Date: Thu Jul 21 05:47:25 2016 [M53] service worker: Refactor OnNoControllees OnNoControllees doesn't need to unset the flags. * If |is_uninstalling_| is set, the call to Clear() would unset it. * If |should_activate_when_ready_| is set, the call to ActivateWaitingVersion() would unset it. We also change Clear() to unset |should_activate_when_ready_| which makes sense for any callsite of Clear(). Minor simplification in anticipation of changes for https://crbug.com/616331 Refactoring only; no behavior change. BUG= 616331 Review-Url: https://codereview.chromium.org/2115393002 Cr-Commit-Position: refs/heads/master@{#403650} (cherry picked from commit 6c74874b9c6969a2afc6f195c96a1a3b15cda890) Review URL: https://codereview.chromium.org/2170723003 . Cr-Commit-Position: refs/branch-heads/2785@{#257} Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382} [modify] https://crrev.com/09a50f12f80cb14568479cf33b10d55cdb6a54a2/content/browser/service_worker/service_worker_registration.cc
,
Jul 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2d740ca87cb68f2fbc558a34eb1f1e8328dd6470 commit 2d740ca87cb68f2fbc558a34eb1f1e8328dd6470 Author: Matt Falkenhagen <falken@chromium.org> Date: Thu Jul 21 05:55:57 2016 [M53] service worker: Wait for inflight requests before activating This patch makes changes as per discussion at https://github.com/slightlyoff/ServiceWorker/issues/916 Namely, * The waiting worker gets promoted to active once there are no controllees and the incumbent worker has no in-flight events (fetch/sync/push/message etc). * skipWaiting bypasses the no controllees requirement, but we still wait until there are no in-flight events. * As usual, incoming fetch/sync/push events always go to the active worker. BUG= 616331 Review-Url: https://codereview.chromium.org/2119143002 Cr-Commit-Position: refs/heads/master@{#405628} (cherry picked from commit 6c95b0ff8f694cda918e38c96387e5445713fc8e) Review URL: https://codereview.chromium.org/2164113003 . Cr-Commit-Position: refs/branch-heads/2785@{#258} Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382} [modify] https://crrev.com/2d740ca87cb68f2fbc558a34eb1f1e8328dd6470/content/browser/service_worker/service_worker_registration.cc [modify] https://crrev.com/2d740ca87cb68f2fbc558a34eb1f1e8328dd6470/content/browser/service_worker/service_worker_registration.h [modify] https://crrev.com/2d740ca87cb68f2fbc558a34eb1f1e8328dd6470/content/browser/service_worker/service_worker_registration_unittest.cc [modify] https://crrev.com/2d740ca87cb68f2fbc558a34eb1f1e8328dd6470/content/browser/service_worker/service_worker_storage.h [modify] https://crrev.com/2d740ca87cb68f2fbc558a34eb1f1e8328dd6470/content/browser/service_worker/service_worker_url_request_job_unittest.cc [modify] https://crrev.com/2d740ca87cb68f2fbc558a34eb1f1e8328dd6470/content/browser/service_worker/service_worker_version.cc [modify] https://crrev.com/2d740ca87cb68f2fbc558a34eb1f1e8328dd6470/content/browser/service_worker/service_worker_version.h [add] https://crrev.com/2d740ca87cb68f2fbc558a34eb1f1e8328dd6470/third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html [add] https://crrev.com/2d740ca87cb68f2fbc558a34eb1f1e8328dd6470/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/mint-new-worker.php
,
Jul 22 2016
Have you made any progress uplifting the tests? If not I may just convert them when I implement this fix in gecko next week. Thanks!
,
Jul 23 2016
bkelly@ I have not. Please go ahead, thanks!
,
Jul 26 2016
Here's my stab at converting: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1170543&attachment=8774918 Its failing, but I think its failing in the way we would expect given we haven't implemented the feature yet.
,
Jul 28 2016
FYI, asuth caught a bug in the test during review. On line 55 of activation.html it says:
add_result_callback(() => registration.unregister);
This should probably call unregister() instead of returning the unregister function.
,
Jul 28 2016
bkelly@, thank you for reporting it. I filed a separate issue ( issue 632256 ).
,
Aug 3 2016
By the way, I found another issue related to this. Its possible you already cover it, but I wanted to bring it to your attention. I wrote it up on the spec issue: https://github.com/slightlyoff/ServiceWorker/issues/916#issuecomment-237272060 Just cross posting here since I know github email can easily get lost.
,
Aug 4 2016
Another issue I ran into: Do you delay activation if the current .active worker is still in the activating state? For example, do you wait for the activate event to finish processing? It seems like we must since we could potentially have functional events queued up to fire after the activate event finishes.
,
Aug 31 2016
Hi bkelly you bring up good points. I haven't been able to investigate yet. I spun off to issue 642646 for tracking. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by falken@chromium.org
, Jun 16 2016