Bug 179401 – Implement real post 'install' event steps of the Install algorithm (steps 14+)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Implement real post 'install' event steps of the Install algorithm (steps 14+)

[Brady Eidson]

Created attachment 326274 [details]
WIP patch

Of our tests:
1 timeout
1 assert (know how to resolve that)
others pass.

EWS for WPT while I continue hacking away.

[Build Bot]

Comment on attachment 326274 [details]
WIP patch

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

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/synced-state.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/getregistrations.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/ServiceWorkerGlobalScope/postmessage.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/multiple-update.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/state.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/claim-affect-other-registration.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/waiting.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/update.https.html
http/tests/workers/service/service-worker-clear.html
imported/w3c/web-platform-tests/service-workers/service-worker/onactivate-script-error.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/skip-waiting.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/postmessage-from-waiting-serviceworker.https.html
http/tests/workers/service/basic-install-event.html
imported/w3c/web-platform-tests/service-workers/service-worker/active.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/clients-matchall-exact-controller.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/activate-event-after-install-state-change.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/install-event-type.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/claim-using-registration.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/register-wait-forever-in-install-worker.https.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 326274 [details]
WIP patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

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

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

[Chris Dumez]

Comment on attachment 326274 [details]
WIP patch

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

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:159
> +    registration->updateRegistrationState(ServiceWorkerRegistrationState::Waiting, installing);

We never register the client registrations with the server, do we? I do not see WebSWClientConnection::addServiceWorkerRegistrationInServer() being called. If so, then those update*State() functions only update the server-side objects, not the client ones.

[Chris Dumez]

Comment on attachment 326274 [details]
WIP patch

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

>> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:159
>> +    registration->updateRegistrationState(ServiceWorkerRegistrationState::Waiting, installing);
> 
> We never register the client registrations with the server, do we? I do not see WebSWClientConnection::addServiceWorkerRegistrationInServer() being called. If so, then those update*State() functions only update the server-side objects, not the client ones.

I had to do something like this for it to work:
 452void ServiceWorkerContainer::addRegistration(ServiceWorkerRegistration& registration)
 453{
 454    m_swConnection->addServiceWorkerRegistrationInServer(registration.data().key, registration.identifier());
 455    m_registrations.add(registration.data().key, &registration);
 456}
 457
 458void ServiceWorkerContainer::removeRegistration(ServiceWorkerRegistration& registration)
 459{
 460    m_swConnection->removeServiceWorkerRegistrationInServer(registration.data().key, registration.identifier());
 461    m_registrations.remove(registration.data().key);
 462}

See my patch at https://bugs.webkit.org/show_bug.cgi?id=179396

[Brady Eidson]

Created attachment 326344 [details]
Another WIP

[Build Bot]

Comment on attachment 326344 [details]
Another WIP

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

Number of test failures exceeded the failure limit.

[Build Bot]

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

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

[Build Bot]

Comment on attachment 326344 [details]
Another WIP

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

Number of test failures exceeded the failure limit.

[Build Bot]

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

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

[Brady Eidson]

Created attachment 326382 [details]
WIP

[Brady Eidson]

Created attachment 326395 [details]
WIP

[Brady Eidson]

Created attachment 326407 [details]
New WIP

[Brady Eidson]

Created attachment 326428 [details]
New WIP

[Chris Dumez]

Created attachment 326430 [details]
Not pretty but seems to pass the tests

[Chris Dumez]

Comment on attachment 326430 [details]
Not pretty but seems to pass the tests

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

> Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:64
> +    context.setActiveServiceWorker(getNewestWorker());

I moved this here from ServiceWorkerContainer::jobResolvedWithRegistration() to keep one test (imported/w3c/web-platform-tests/service-workers/service-worker/claim-affect-other-registration.https.html) passing. The test relies on serviceWorker.controller which is currently support via this hack.

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:120
> +    registration.updateRegistrationState(job, ServiceWorkerRegistrationState::Installing, worker, false);

The 'false' here and on the next line is a hack to make sure these 2 tasks get executed *before* the promise is resolved below. It does so by NOT using a micro task.

The rest of this patch seems fine but this part is terrible and we need to find a better way. At least, we now know what was going on.

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:177
> +    auto* waiting = registration->waitingWorker();

These changes below are probably not needed but it seems much cleaner than keeping the fake PostInstall Microtask in the WebProcess.

[Build Bot]

Comment on attachment 326430 [details]
Not pretty but seems to pass the tests

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

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/foreign-fetch-cors.https.html
imported/w3c/web-platform-tests/fetch/api/abort/serviceworker-intercepted.https.html

[Build Bot]

Created attachment 326432 [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 326468 [details]
Alternate approach

[Chris Dumez]

(In reply to Chris Dumez from comment #21)
> Created attachment 326468 [details]
> Alternate approach

Ok, will clean up this last patch and get it ready for review today.

[Brady Eidson]

(In reply to Chris Dumez from comment #22)
> (In reply to Chris Dumez from comment #21)
> > Created attachment 326468 [details]
> > Alternate approach
> 
> Ok, will clean up this last patch and get it ready for review today.

Ping me, will r+ asap

[Chris Dumez]

Created attachment 326484 [details]
WIP Patch

[Chris Dumez]

Created attachment 326492 [details]
Patch

[Brady Eidson]

Comment on attachment 326492 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Implement real post 'install' event steps of the Install algorithm (steps 14+)
> +        https://bugs.webkit.org/show_bug.cgi?id=179401
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Implement real post 'install' event steps of the Install algorithm (steps 14+):
> +        - https://w3c.github.io/ServiceWorker/#installation-algorithm

This is weird duplication.

> Source/WebKit/ChangeLog:9
> +        Implement real post 'install' event steps of the Install algorithm (steps 14+)
> +        https://bugs.webkit.org/show_bug.cgi?id=179401
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Implement real post 'install' event steps of the Install algorithm (steps 14+):
> +        - https://w3c.github.io/ServiceWorker/#installation-algorithm

This is weird duplication

[Chris Dumez]

Created attachment 326499 [details]
Patch

[Chris Dumez]

Committed r224652: <https://trac.webkit.org/changeset/224652>

[Radar WebKit Bug Importer]

<rdar://problem/35562200>

[Geoffrey Garen]

*** Bug 179395 has been marked as a duplicate of this bug. ***