ServiceWorker: Keep the worker alive until FetchEvent.waitUntil settles

third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp

Index: third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp
diff --git a/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp b/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp
index e998bb03a9a984f99ee6280e306dffb18078bcf8..91c88a536e1f5e9b6890d14d0371080f14b13294 100644
--- a/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp
+++ b/third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp
@@ -131,9 +131,10 @@ void ServiceWorkerGlobalScopeProxy::dispatchExtendableMessageEvent(int eventID,
     workerGlobalScope()->dispatchExtendableEvent(event, observer);
 }
 
-void ServiceWorkerGlobalScopeProxy::dispatchFetchEvent(int eventID, const WebServiceWorkerRequest& webRequest)
+void ServiceWorkerGlobalScopeProxy::dispatchFetchEvent(int responseID, int eventFinishID, const WebServiceWorkerRequest& webRequest)
 {
-    RespondWithObserver* observer = RespondWithObserver::create(workerGlobalScope(), eventID, webRequest.url(), webRequest.mode(), webRequest.frameType(), webRequest.requestContext());
+    WaitUntilObserver* waitUntilObserver = WaitUntilObserver::create(workerGlobalScope(), WaitUntilObserver::Fetch, eventFinishID);
+    RespondWithObserver* respondWithObserver = RespondWithObserver::create(workerGlobalScope(), responseID, webRequest.url(), webRequest.mode(), webRequest.frameType(), webRequest.requestContext(), waitUntilObserver);
     Request* request = Request::create(workerGlobalScope()->scriptController()->getScriptState(), webRequest);
     request->getHeaders()->setGuard(Headers::ImmutableGuard);
     FetchEventInit eventInit;
@@ -141,24 +142,29 @@ void ServiceWorkerGlobalScopeProxy::dispatchFetchEvent(int eventID, const WebSer
     eventInit.setRequest(request);
     eventInit.setClientId(webRequest.isMainResourceLoad() ? WebString() : webRequest.clientId());
     eventInit.setIsReload(webRequest.isReload());
-    FetchEvent* fetchEvent = FetchEvent::create(workerGlobalScope()->scriptController()->getScriptState(), EventTypeNames::fetch, eventInit, observer);
+    FetchEvent* fetchEvent = FetchEvent::create(workerGlobalScope()->scriptController()->getScriptState(), EventTypeNames::fetch, eventInit, respondWithObserver, waitUntilObserver);
+    waitUntilObserver->willDispatchEvent();
     DispatchEventResult dispatchResult = workerGlobalScope()->dispatchEvent(fetchEvent);
-    observer->didDispatchEvent(dispatchResult);
+    respondWithObserver->didDispatchEvent(dispatchResult);
+    waitUntilObserver->didDispatchEvent(dispatchResult != DispatchEventResult::NotCanceled);

[falken, 2016/06/16 06:23:11]

Why do we need to check if NotCanceled? What's the consequence of passing true
vs false to didDispatchEvent here?

Also, is waitUntil(Promise.resolve()) any different than
waitUntil(Promise.reject()) for a fetch event? I think there shouldn't be a
difference. Can we have a test case for that?

[shimazu, 2016/06/21 02:43:38]

There is no difference between the two because a receiver of didDispatchEvent
for FetchEvent is SWUtils::NoOpStatusCallback.

The spec says "the rejected state of any of the promise in extend lifetime
promises to suggest operation failure" originally, so I thought an argument of
didDispatchEvent should be the same with other ExtendableEvents, and the
browser-side callback should treat it as they want.

[1]
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...

[falken, 2016/06/21 04:12:32]

Hm, the problem I have is WaitUntilObserver::didDispatchEvent takes a "bool
errorOccurred" which is different than  canceled. The other ExtendableEvents
don't look at DispatchEventResult. dispatchExtendableEvent just tracks whether
an runtime error occurred.

I think this might be an existing bug that dispatchFetchEvent doesn't care about
runtime errors. What happens if you do:
onfetch = e => {
  ...
  e.respondWith(response);
  throw 'error';
};

The spec seems to say the respondWith shouldn't work.

I don't think we need to block this patch on this though. Can you change it to
waitUntilObserver->didDispatchEvent(false /* errorOccurred */) and add a TODO
that dispatchFetchEvent should track whether a runtime error occurred, along
with a comment that this is OK for now because waitUntil for fetch event doesn't
care about promise rejection or runtime errors?

[shimazu, 2016/06/21 10:00:36]

Done.

I'll file the bug.

[Marijn Kruisselbrink, 2016/06/21 17:32:15]

Note that there is also a discussion about what throwing in onfetch should do at
https://github.com/slightlyoff/ServiceWorker/issues/896, where it seems perhaps
the current chrome behavior rather than the speced behavior is what is desired.

[falken, 2016/06/22 01:27:31]

Aha! Thanks for pointing that out.

 }
 
-void ServiceWorkerGlobalScopeProxy::dispatchForeignFetchEvent(int eventID, const WebServiceWorkerRequest& webRequest)
+void ServiceWorkerGlobalScopeProxy::dispatchForeignFetchEvent(int responseID, int eventFinishID, const WebServiceWorkerRequest& webRequest)
 {
     RefPtr<SecurityOrigin> origin = SecurityOrigin::create(webRequest.referrerUrl());
-    ForeignFetchRespondWithObserver* observer = ForeignFetchRespondWithObserver::create(workerGlobalScope(), eventID, webRequest.url(), webRequest.mode(), webRequest.frameType(), webRequest.requestContext(), origin);
+    WaitUntilObserver* waitUntilObserver = WaitUntilObserver::create(workerGlobalScope(), WaitUntilObserver::Fetch, eventFinishID);
+    ForeignFetchRespondWithObserver* respondWithObserver = ForeignFetchRespondWithObserver::create(workerGlobalScope(), responseID, webRequest.url(), webRequest.mode(), webRequest.frameType(), webRequest.requestContext(), origin, waitUntilObserver);
     Request* request = Request::create(workerGlobalScope()->scriptController()->getScriptState(), webRequest);
     request->getHeaders()->setGuard(Headers::ImmutableGuard);
     ForeignFetchEventInit eventInit;
     eventInit.setCancelable(true);
     eventInit.setRequest(request);
     eventInit.setOrigin(origin->toString());
-    ForeignFetchEvent* fetchEvent = ForeignFetchEvent::create(workerGlobalScope()->scriptController()->getScriptState(), EventTypeNames::foreignfetch, eventInit, observer);
+    ForeignFetchEvent* fetchEvent = ForeignFetchEvent::create(workerGlobalScope()->scriptController()->getScriptState(), EventTypeNames::foreignfetch, eventInit, respondWithObserver, waitUntilObserver);
+    waitUntilObserver->willDispatchEvent();
     DispatchEventResult dispatchResult = workerGlobalScope()->dispatchEvent(fetchEvent);
-    observer->didDispatchEvent(dispatchResult);
+    respondWithObserver->didDispatchEvent(dispatchResult);
+    waitUntilObserver->didDispatchEvent(dispatchResult != DispatchEventResult::NotCanceled);
 }
 
 void ServiceWorkerGlobalScopeProxy::dispatchInstallEvent(int eventID)