Bug 179401 - Implement real post 'install' event steps of the Install algorithm (steps 14+)
Summary: Implement real post 'install' event steps of the Install algorithm (steps 14+)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
: 179395 (view as bug list)
Depends on:
Blocks: 179436 179441
  Show dependency treegraph
 
Reported: 2017-11-07 16:25 PST by Brady Eidson
Modified: 2017-12-19 15:00 PST (History)
9 users (show)

See Also:


Attachments
WIP patch (5.35 KB, patch)
2017-11-07 17:21 PST, Brady Eidson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (2.93 MB, application/zip)
2017-11-07 18:36 PST, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.32 MB, application/zip)
2017-11-07 18:40 PST, Build Bot
no flags Details
Another WIP (5.37 KB, patch)
2017-11-08 12:29 PST, Brady Eidson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (2.47 MB, application/zip)
2017-11-08 13:30 PST, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.30 MB, application/zip)
2017-11-08 13:55 PST, Build Bot
no flags Details
WIP (11.06 KB, patch)
2017-11-08 14:44 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
WIP (8.40 KB, patch)
2017-11-08 15:33 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
New WIP (11.17 KB, patch)
2017-11-08 17:01 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
New WIP (11.17 KB, patch)
2017-11-08 21:50 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Not pretty but seems to pass the tests (27.63 KB, patch)
2017-11-08 22:14 PST, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.22 MB, application/zip)
2017-11-08 23:38 PST, Build Bot
no flags Details
Alternate approach (35.33 KB, patch)
2017-11-09 10:50 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (45.38 KB, patch)
2017-11-09 13:45 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (58.46 KB, patch)
2017-11-09 14:54 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (58.40 KB, patch)
2017-11-09 15:41 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2017-11-07 16:25:46 PST
Implement real post 'install' event steps of the Install algorithm (steps 14+)
Comment 1 Brady Eidson 2017-11-07 17:21:12 PST
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.
Comment 2 Build Bot 2017-11-07 18:36:08 PST
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
Comment 3 Build Bot 2017-11-07 18:36:09 PST
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
Comment 4 Build Bot 2017-11-07 18:40:06 PST
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.
Comment 5 Build Bot 2017-11-07 18:40:08 PST
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
Comment 6 Chris Dumez 2017-11-07 18:58:48 PST
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.
Comment 7 Chris Dumez 2017-11-07 19:29:51 PST
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
Comment 8 Brady Eidson 2017-11-08 12:29:05 PST
Created attachment 326344 [details]
Another WIP
Comment 9 Build Bot 2017-11-08 13:30:49 PST
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.
Comment 10 Build Bot 2017-11-08 13:30:50 PST
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
Comment 11 Build Bot 2017-11-08 13:55:38 PST
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.
Comment 12 Build Bot 2017-11-08 13:55:39 PST
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
Comment 13 Brady Eidson 2017-11-08 14:44:20 PST
Created attachment 326382 [details]
WIP
Comment 14 Brady Eidson 2017-11-08 15:33:37 PST
Created attachment 326395 [details]
WIP
Comment 15 Brady Eidson 2017-11-08 17:01:01 PST
Created attachment 326407 [details]
New WIP
Comment 16 Brady Eidson 2017-11-08 21:50:47 PST
Created attachment 326428 [details]
New WIP
Comment 17 Chris Dumez 2017-11-08 22:14:08 PST
Created attachment 326430 [details]
Not pretty but seems to pass the tests
Comment 18 Chris Dumez 2017-11-08 22:24:30 PST
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.
Comment 19 Build Bot 2017-11-08 23:38:31 PST
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
Comment 20 Build Bot 2017-11-08 23:38:33 PST
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
Comment 21 Chris Dumez 2017-11-09 10:50:55 PST
Created attachment 326468 [details]
Alternate approach
Comment 22 Chris Dumez 2017-11-09 12:27:57 PST
(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.
Comment 23 Brady Eidson 2017-11-09 12:49:41 PST
(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
Comment 24 Chris Dumez 2017-11-09 13:45:22 PST
Created attachment 326484 [details]
WIP Patch
Comment 25 Chris Dumez 2017-11-09 14:54:58 PST
Created attachment 326492 [details]
Patch
Comment 26 Brady Eidson 2017-11-09 15:36:36 PST
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
Comment 27 Chris Dumez 2017-11-09 15:41:51 PST
Created attachment 326499 [details]
Patch
Comment 28 Chris Dumez 2017-11-09 15:45:35 PST
Committed r224652: <https://trac.webkit.org/changeset/224652>
Comment 29 Radar WebKit Bug Importer 2017-11-15 09:40:20 PST
<rdar://problem/35562200>
Comment 30 Geoffrey Garen 2017-12-19 15:00:14 PST
*** Bug 179395 has been marked as a duplicate of this bug. ***