Oh that's embarrassing. The fix should also implement https://github.com/slightlyoff/ServiceWorker/issues/771.

When you get there, can you let us know when your fix for #771 will ship?  I'd like to make sure our version of that change ships around the same time to avoid compat issues.  Thanks!

I'm not sure if fixing waitUntil on FetchEvent and allowing waitUntil to be called async are really that intertwined. A large part of getting waitUntil on fetch events to work will be at the IPC and browser process side. I tried to keep this use-case into account when designing the current way service worker event IPCs are dispatched through SWVersion, although it would still be a bit tricky with classical IPC. I wonder if converting fetch event dispatching to mojo first would be a good idea (since it would make having multiple types of "replies" to an event easier), although that has its own set of difficulties of course (when eventually all service worker event dispatching is converted to mojo we will be able to get rid of a lot of code in content/renderer as well).

Naively I just thought we'd just split the ServiceWorkerHostMsg_FetchEventFinished IPC into ServiceWorkerHostMsg_FetchEventResponse and ServiceWorkerHostMsg_FetchEventFinished, and FetchEvent would be moved back to custom handling SWVersion rather than using the DispatchEvent<> way. But maybe that's ugly. And if we're special casing FetchEvent then yea maybe converting it to mojo first would make sense.

I guess foreignfetch event has the same problem. Should it be solved at the same time?

That would be nice, yes. Especially since these events share a lot of the implementation anyway. And if there is any way I can help with fixing any of this just let me know.

Thanks, I'll do it:)

I'm now considering how to implement it; we have two options as the previous discussion.
- mojo-nize
- Add a "response" chromium IPC

For mojo-nize, we have to make ServiceWorkerContextClient as a service of mojo, or have to create Fetch/ForeignFetch services for IPC like the same with the background sync. However, I'm afraid of the messages' ordering when using mojo. Mojo ensures only the messages' order of the same service (afaik it is corresponding to an object in C++ world), so I think we have to do much investigation about the ordering beyond the services. and I think this will take much time rather than just implementing .waitUntil, so I'm considering mojo-nize should be done as one individual task, not side-task.

Adding chroimum IPC, I'm planning to add waituntilobserver into fetch/foreignfetch event, add ServiceWorkerHostMsg_(Foreign)FetchEventResponse as new IPCs and let Response fired when respondWith is resolved. This might be fine, but now I'm wondering when "ServiceWorkerHostMsg_...EventFinished" should be fired. 

It's easier to implement if it is fired when calling waitUntil. For implementing that, ServiceWorkerVersion::StartRequest will be called twice (for FetchEventResponse and FetchEventFinished) and SWVersion will wait for two responses corresponding to the two messages (Response and Finished) by using BarrierClosure before running some EventFinished callback in browser process. (so the name of the EventFinished message should be changed from "EventFinished" to "WaitUntilFinished"). 

Another option is to send the EventFinished message after promises of respondWith and waitUntil are resolved. To do this, I think respondWith and waitUntil should collaborate to decide when they fire "...EventFinished".
I think this is better than changing "EventFinished" to "WaitUntilFinished" because the meaning of IPC looks simple, but I want a good idea for the implementation. This looks breaking the simpleness of RespondWithObserver and WaitUntilObserver for me.

Do you have any comment or recommendation for this? 

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

commit 3f122ea68eb653a175edc74ce5dbf9e8ba344e5e
Author: shimazu <shimazu@chromium.org>
Date: Wed Jun 29 04:12:24 2016

ServiceWorker: Keep the worker alive until FetchEvent.waitUntil settles

Currently ServiceWorker doesn't track if FetchEvent.waitUntil is finished, so
worker will be shutted down by idle timeout though a heavy task is running.
This patch adds WaitUntilObserver and makes RespondWith collaborate with it
in order to keep the worker alive.

BUG= 601095 
TEST=./out/Release/content_unittests --gtest_filter="ServiceWorkerURLRequestJobTest.*"
TEST=./out/Release/content_unittests --gtest_filter="ServiceWorkerVersionTest.*"

Review-Url: https://codereview.chromium.org/2034663002
Cr-Commit-Position: refs/heads/master@{#402710}

[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/browser/service_worker/embedded_worker_test_helper.cc
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/browser/service_worker/embedded_worker_test_helper.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/browser/service_worker/service_worker_metrics.cc
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/browser/service_worker/service_worker_metrics.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/browser/service_worker/service_worker_register_job.cc
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/browser/service_worker/service_worker_url_request_job_unittest.cc
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/browser/service_worker/service_worker_version_unittest.cc
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/common/service_worker/service_worker_messages.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/content/renderer/service_worker/service_worker_context_client.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/modules/serviceworkers/ForeignFetchEvent.cpp
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/modules/serviceworkers/ForeignFetchEvent.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/modules/serviceworkers/ForeignFetchRespondWithObserver.cpp
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/modules/serviceworkers/ForeignFetchRespondWithObserver.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h
[modify] https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e/tools/metrics/histograms/histograms.xml

Does this implement async execution of waitUntil() as well?  It was mentioned in comment 2 here, but its not clear if it made it into this fix.  Thanks!

omg, I deleted the message by mistake, sorry;(
That was the following comment from bkelly@mozilla.com.

--

Does this implement async execution of waitUntil() as well? It was mentioned in comment 2 here, but its not clear if it made it into this fix. Thanks!

And, I didn't implement it yet. I'm planning to do it soon, and already filed it here:  http://crbug.com/621440 

Thanks!  I just want to try to coordinate releases with our implementation to make compat easier for web devs.

Can you add the service worker component to that issue?  That way I'll see it when you land changes.

FWIW, our corresponding gecko bug is here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1263304