Bug 179396 – [Service Workers] Support waitUntil() on the 'install' event

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Support waitUntil() on the 'install' event:
- https://w3c.github.io/ServiceWorker/#installation-algorithm (step 10.4)

[Chris Dumez]

Created attachment 326273 [details]
WIP Patch

[Build Bot]

Comment on attachment 326273 [details]
WIP Patch

Attachment 326273 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5140403

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/windowclient-navigate.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/oninstall-script-error.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/register-wait-forever-in-install-worker.https.html

[Build Bot]

Created attachment 326275 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 326273 [details]
WIP Patch

Attachment 326273 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5140439

New failing tests:
imported/w3c/web-platform-tests/streams/readable-streams/count-queuing-strategy-integration.serviceworker.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/oninstall-script-error.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/register-wait-forever-in-install-worker.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/windowclient-navigate.https.html

[Build Bot]

Created attachment 326279 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 326283 [details]
WIP Patch

[Chris Dumez]

Created attachment 326288 [details]
WIP Patch

[Build Bot]

Attachment 326288 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/ServiceWorkerContainer.h:75:  The parameter name "registration" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/workers/service/ServiceWorkerContainer.h:76:  The parameter name "registration" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 326289 [details]
WIP Patch

[Chris Dumez]

Created attachment 326290 [details]
WIP Patch

[Chris Dumez]

Created attachment 326292 [details]
WIP Patch

[Chris Dumez]

Created attachment 326294 [details]
WIP Patch

[Chris Dumez]

Created attachment 326295 [details]
Patch

[Chris Dumez]

Created attachment 326296 [details]
Patch

[Chris Dumez]

Comment on attachment 326296 [details]
Patch

Clearing flags on attachment: 326296

Committed r224584: <https://trac.webkit.org/changeset/224584>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Comment on attachment 326296 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326296&action=review

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:126
> +        installEvent->whenAllExtendLifetimePromisesAreSettled([serviceWorkerIdentifier](HashSet<Ref<DOMPromise>>&& extendLifetimePromises) {

Removing a promise from the hashset as soon as promise is settled seems simpler and is more efficient.
I am not sure why we want to keep references for all promises.
In the particular install case, we could instead have a callback taking an allResolved/rejected flag.
Would that work with all other cases?

[youenn fablet]

Ah I arrived too late there...
If the callback(allresolved/rejected) works, I would much prefer this model.

[Chris Dumez]

(In reply to youenn fablet from comment #18)
> Ah I arrived too late there...
> If the callback(allresolved/rejected) works, I would much prefer this model.

We can simplify later on. The current design is very flexible since it provides all the promises to the caller. This also matches the spec very closely.

I do not really buy the 'more efficient' argument since I do not have to remove anything from the HashSet and I only keep the promises alive until I call the "all promises resolved" callback.

[Chris Dumez]

(In reply to Chris Dumez from comment #19)
> (In reply to youenn fablet from comment #18)
> > Ah I arrived too late there...
> > If the callback(allresolved/rejected) works, I would much prefer this model.
> 
> We can simplify later on. The current design is very flexible since it
> provides all the promises to the caller. This also matches the spec very
> closely.
> 
> I do not really buy the 'more efficient' argument since I do not have to
> remove anything from the HashSet and I only keep the promises alive until I
> call the "all promises resolved" callback.

Note that I am not against the AllResolved/Rejected idea. I just do not think this has to be done now. There will be more users of waitUntil() and I'd rather we simplify the design once all are implemented (correctly).

[youenn fablet]

"more efficient" is in the sense that before the patch the promise is collectable as soon as settled. AFAIUI, with this patch, this will not happen until all related promises are settled. This does not seem like an improvement to me.

If there is an actual use case for checking each individual promise result, this patch is fine. My guess is that rejected/resolved is probably sufficient.

[Chris Dumez]

(In reply to Chris Dumez from comment #20)
> (In reply to Chris Dumez from comment #19)
> > (In reply to youenn fablet from comment #18)
> > > Ah I arrived too late there...
> > > If the callback(allresolved/rejected) works, I would much prefer this model.
> > 
> > We can simplify later on. The current design is very flexible since it
> > provides all the promises to the caller. This also matches the spec very
> > closely.
> > 
> > I do not really buy the 'more efficient' argument since I do not have to
> > remove anything from the HashSet and I only keep the promises alive until I
> > call the "all promises resolved" callback.
> 
> Note that I am not against the AllResolved/Rejected idea. I just do not
> think this has to be done now. There will be more users of waitUntil() and
> I'd rather we simplify the design once all are implemented (correctly).

I'll let you know once we implement the other call sites properly.

[Radar WebKit Bug Importer]

<rdar://problem/35567002>