Implement "UpdateWorkerState"

[Brady Eidson]

Implement "UpdateWorkerState", "fire updatefound event", and actually rely on the real update/install algorithms

[Brady Eidson]

It is difficult to reconcile doing the work for real with getting rid of the fake Microtask we put in place to get tests running.

This WIP is me in the middle of trying to reconcile that.

[Brady Eidson]

Created attachment 326120 [details]
WIP, trying to reconcile that microtask without causing test failures

[Chris Dumez]

Comment on attachment 326120 [details]
WIP, trying to reconcile that microtask without causing test failures

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

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:136
> +    // FIXME: Here is the point in time where we need to create a ServiceWorkerRegistration object

This seems premature, no? What if the registration fails?
In the spec, it is constructed when we resolve the promise (https://w3c.github.io/ServiceWorker/#resolve-job-promise), early in the Install steps.

> Source/WebCore/workers/service/server/SWClientConnection.cpp:200
> +            registration->dispatchEvent(Event::create(eventNames().updatefoundEvent, false, false));

You now fire this event twice?

> Source/WebCore/workers/service/server/SWClientConnection.cpp:219
> +    auto* registrations = m_registrations.get(key);

I believe the registrations are on the container nowadays.

> Source/WebCore/workers/service/server/SWClientConnection.cpp:229
> +        registration->dispatchEvent(Event::create(eventNames().updatefoundEvent, false, false));

Why are we moving more of the event firing logic into SWClientConnection? This does not seem like the right place for this.

> Source/WebCore/workers/service/server/SWServer.cpp:262
> +    registration->fireUpdateFoundEvent();

This is technically part of the install steps, maybe this should be moved to an install function for clarity?

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:98
> +    for (auto& connectionIdentifierWithClients : m_clientRegistrationsByConnection.keys()) {

In my experience, the client registration is not added to m_clientRegistrationsByConnection yet by the time we reach this code, in the case of a registration, so it did not end up firing the event at the new registration object.

[Brady Eidson]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 326120 [details]
> WIP, trying to reconcile that microtask without causing test failures
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326120&action=review
> 
> > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:136
> > +    // FIXME: Here is the point in time where we need to create a ServiceWorkerRegistration object
> 
> This seems premature, no? What if the registration fails?
> In the spec, it is constructed when we resolve the promise
> (https://w3c.github.io/ServiceWorker/#resolve-job-promise), early in the
> Install steps.

That's not true.

It's created at step 6. of the register algorithm by invoking Set Registration.

https://w3c.github.io/ServiceWorker/#register-algorithm

> 
> > Source/WebCore/workers/service/server/SWClientConnection.cpp:200
> > +            registration->dispatchEvent(Event::create(eventNames().updatefoundEvent, false, false));
> 
> You now fire this event twice?

Why do you say that? (If this patch happens to fire it twice, that's because I'm trying to refactor out one of them.

> 
> > Source/WebCore/workers/service/server/SWClientConnection.cpp:219
> > +    auto* registrations = m_registrations.get(key);
> 
> I believe the registrations are on the container nowadays.

After I started this Friday, yes - As you can see from the patch not applying it's simply out of date.

> 
> > Source/WebCore/workers/service/server/SWClientConnection.cpp:229
> > +        registration->dispatchEvent(Event::create(eventNames().updatefoundEvent, false, false));
> 
> Why are we moving more of the event firing logic into SWClientConnection?
> This does not seem like the right place for this.

SWClientConnection isn't firing the event. It's telling the registration to.

> 
> > Source/WebCore/workers/service/server/SWServer.cpp:262
> > +    registration->fireUpdateFoundEvent();
> 
> This is technically part of the install steps, maybe this should be moved to
> an install function for clarity?

Maybe. Once I rebase to trunk will take a look.

> 
> > Source/WebCore/workers/service/server/SWServerRegistration.cpp:98
> > +    for (auto& connectionIdentifierWithClients : m_clientRegistrationsByConnection.keys()) {
> 
> In my experience, the client registration is not added to
> m_clientRegistrationsByConnection yet by the time we reach this code, in the
> case of a registration, so it did not end up firing the event at the new
> registration object.

Right, and that's what I'm fixing above.

[Brady Eidson]

Created attachment 326172 [details]
WIP to take home

This'll crash the tests for now, as I'm trying to unravel this assumption of always having an active worker (ServiceWorkerContainer::jobResolvedWithRegistration)

[Build Bot]

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


ERROR: Source/WebCore/workers/service/server/SWServer.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 28 files


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

[Build Bot]

Comment on attachment 326172 [details]
WIP to take home

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

Number of test failures exceeded the failure limit.

[Build Bot]

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

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

[Build Bot]

Comment on attachment 326172 [details]
WIP to take home

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 326180 [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]

Comment on attachment 326172 [details]
WIP to take home

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

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:120
> +        installingID = m_waitingWorker->identifier();

waitingID

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:124
> +        installingID = m_activeWorker->identifier();

activeID

[Chris Dumez]

Comment on attachment 326172 [details]
WIP to take home

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

> Source/WebCore/workers/service/ServiceWorker.h:68
> +    static const HashMap<ServiceWorkerIdentifier, HashSet<ServiceWorker*>>& allWorkers();

Chances are we will want to easily get an existing ServiceWorker object for a given ID and ScriptExecutionContext (like we do for registrations). As such, I think it'd be nicer if this method was not static and if it were a HashMap<ServiceWorkerIdentifier, ServiceWorker*>.

[Chris Dumez]

Comment on attachment 326172 [details]
WIP to take home

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

> Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:154
> +        worker = ServiceWorker::create(*context, *serviceWorkerIdentifier, m_registrationData.scriptURL);

For example here, it'd be useful to use allWorkers() to get the ServiceWorker object for id serviceWorkerIdentifier in this context, if it exist.

[Brady Eidson]

Created attachment 326191 [details]
Getting close to review, lingering test issue(s)

[Brady Eidson]

(In reply to Chris Dumez from comment #12)
> Comment on attachment 326172 [details]
> WIP to take home
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326172&action=review
> 
> > Source/WebCore/workers/service/ServiceWorker.h:68
> > +    static const HashMap<ServiceWorkerIdentifier, HashSet<ServiceWorker*>>& allWorkers();
> 
> Chances are we will want to easily get an existing ServiceWorker object for
> a given ID and ScriptExecutionContext (like we do for registrations). As
> such, I think it'd be nicer if this method was not static and if it were a
> HashMap<ServiceWorkerIdentifier, ServiceWorker*>.

I agree we'll need that, but we'll also need a global lookup of all service workers for a given ID.

[Brady Eidson]

(In reply to Chris Dumez from comment #13)
> Comment on attachment 326172 [details]
> WIP to take home
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326172&action=review
> 
> > Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:154
> > +        worker = ServiceWorker::create(*context, *serviceWorkerIdentifier, m_registrationData.scriptURL);
> 
> For example here, it'd be useful to use allWorkers() to get the
> ServiceWorker object for id serviceWorkerIdentifier in this context, if it
> exist.

Yup - This'll be needed. But so will the global map. We truly do need the ability to broadcast to all workers, regardless of context.

[Brady Eidson]

Created attachment 326192 [details]
Getting closer

[Chris Dumez]

(In reply to Brady Eidson from comment #16)
> (In reply to Chris Dumez from comment #13)
> > Comment on attachment 326172 [details]
> > WIP to take home
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=326172&action=review
> > 
> > > Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:154
> > > +        worker = ServiceWorker::create(*context, *serviceWorkerIdentifier, m_registrationData.scriptURL);
> > 
> > For example here, it'd be useful to use allWorkers() to get the
> > ServiceWorker object for id serviceWorkerIdentifier in this context, if it
> > exist.
> 
> Yup - This'll be needed. But so will the global map. We truly do need the
> ability to broadcast to all workers, regardless of context.

Oh I see, makes sense.

[Chris Dumez]

Comment on attachment 326192 [details]
Getting closer

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

> Source/WebCore/workers/service/ServiceWorker.cpp:85
> +    if (shouldFire == FireStateChangeEvent)

In the spec, as far as I can tell, we always fire astatechange event. Why do we need this check?

[Chris Dumez]

Comment on attachment 326192 [details]
Getting closer

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

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:-345
> -    registration->setInstallingWorker(activeServiceWorker);

Dropping this seems bad and likely breaks update() which I implemented recently.

[Brady Eidson]

(In reply to Chris Dumez from comment #19)
> Comment on attachment 326192 [details]
> Getting closer
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326192&action=review
> 
> > Source/WebCore/workers/service/ServiceWorker.cpp:85
> > +    if (shouldFire == FireStateChangeEvent)
> 
> In the spec, as far as I can tell, we always fire astatechange event. Why do
> we need this check?

This is not spec usage.

Spec usage is Install, step 4 "Run the Update Worker State algorithm passing registration’s installing worker and installing as the arguments."

But that broadcasts out to all context pages and affects ServiceWorker objects that are in existence at that point in time.

This is part of Step 6 - Invoke Resolve Job Promise with job and registration.
We don't dispatch the event there. It's assumed the worker that's created for the promise resolution is already set up correctly.

[Brady Eidson]

(In reply to Chris Dumez from comment #20)
> Comment on attachment 326192 [details]
> Getting closer
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326192&action=review
> 
> > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:-345
> > -    registration->setInstallingWorker(activeServiceWorker);
> 
> Dropping this seems bad and likely breaks update() which I implemented
> recently.

Aha, but I didn't drop it!

Previously we populated active worker, then set it as the installing worker.
Now we populate installing worker and set it as active worker.

Same effect, I think?

(Lingering test failures might prove me wrong)

[Chris Dumez]

Comment on attachment 326192 [details]
Getting closer

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

>>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:-345
>>> -    registration->setInstallingWorker(activeServiceWorker);
>> 
>> Dropping this seems bad and likely breaks update() which I implemented recently.
> 
> Aha, but I didn't drop it!
> 
> Previously we populated active worker, then set it as the installing worker.
> Now we populate installing worker and set it as active worker.
> 
> Same effect, I think?
> 
> (Lingering test failures might prove me wrong)

You are in a if(!registration) scope so not an update. In the case there is an existing registration , the update case, it looks to me that you no longer set the installing worker on the registration, no?

[Build Bot]

Comment on attachment 326192 [details]
Getting closer

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/abort/serviceworker-intercepted.https.html
http/tests/workers/service/service-worker-getRegistration.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/claim-affect-other-registration.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/foreign-fetch-cors.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/import-scripts-redirect.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/update.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/windowclient-navigate.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/skip-waiting.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/multi-globals/url-parsing.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/clients-matchall-client-types.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/postmessage-from-waiting-serviceworker.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/client-navigate.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/claim-using-registration.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register.https.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 326192 [details]
Getting closer

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/abort/serviceworker-intercepted.https.html
http/tests/workers/service/service-worker-getRegistration.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/claim-affect-other-registration.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/foreign-fetch-cors.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/import-scripts-redirect.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/update.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/windowclient-navigate.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/skip-waiting.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/multi-globals/url-parsing.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/clients-matchall-client-types.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/postmessage-from-waiting-serviceworker.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/client-navigate.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/claim-using-registration.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register.https.html

[Build Bot]

Created attachment 326196 [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

[Brady Eidson]

(In reply to Chris Dumez from comment #23)
> Comment on attachment 326192 [details]
> Getting closer
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326192&action=review
> 
> >>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:-345
> >>> -    registration->setInstallingWorker(activeServiceWorker);
> >> 
> >> Dropping this seems bad and likely breaks update() which I implemented recently.
> > 
> > Aha, but I didn't drop it!
> > 
> > Previously we populated active worker, then set it as the installing worker.
> > Now we populate installing worker and set it as active worker.
> > 
> > Same effect, I think?
> > 
> > (Lingering test failures might prove me wrong)
> 
> You are in a if(!registration) scope so not an update. In the case there is
> an existing registration , the update case, it looks to me that you no
> longer set the installing worker on the registration, no?

YUP.

Is it actually guaranteed that the worker always goes to active in the other case? Seems a little suspect.

But oh well, get tests working...

[Chris Dumez]

Comment on attachment 326192 [details]
Getting closer

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

>>>>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:-345
>>>>> -    registration->setInstallingWorker(activeServiceWorker);
>>>> 
>>>> Dropping this seems bad and likely breaks update() which I implemented recently.
>>> 
>>> Aha, but I didn't drop it!
>>> 
>>> Previously we populated active worker, then set it as the installing worker.
>>> Now we populate installing worker and set it as active worker.
>>> 
>>> Same effect, I think?
>>> 
>>> (Lingering test failures might prove me wrong)
>> 
>> You are in a if(!registration) scope so not an update. In the case there is an existing registration , the update case, it looks to me that you no longer set the installing worker on the registration, no?
> 
> YUP.
> 
> Is it actually guaranteed that the worker always goes to active in the other case? Seems a little suspect.
> 
> But oh well, get tests working...

Not sure what goes to active means here. The code was "registration->setInstallingWorker(activeServiceWorker);" so it is set as installing worker, which is correct.

[Brady Eidson]

Created attachment 326221 [details]
Some small tweaks

[Brady Eidson]

> Not sure what goes to active means here

Me neither - It was already here before me!

[Chris Dumez]

Comment on attachment 326221 [details]
Some small tweaks

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

Still looking into why getRegistration() fails but here are a few things I noticed.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:334
> +    if (data.activeServiceWorkerIdentifier && (!installingServiceWorker || installingServiceWorker->identifier() != *data.activeServiceWorkerIdentifier)) {

This behavior change would likely break tests. We used to set ScriptExecutionContext::m_activeServiceWorker to the newly *installing* worker. You now set it to the *active* worker, which is most cases (non-updates) will be null.

ScriptExecutionContext::m_activeServiceWorker is a hack but I think we should try to maintain previous behavior until we implement proper selection. This code should look like so I believe:
auto* installingServiceWorker = context->activeServiceWorker();
if (!installingServiceWorker || installingServiceWorker->identifier() != *data.installingServiceWorkerIdentifier) {
    context->setActiveServiceWorker(ServiceWorker::create(*context, * data.installingServiceWorkerIdentifier, data.scriptURL));
    installingServiceWorker = context->activeServiceWorker();
}

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:352
> +    // FIXME: Implement proper selection of service workers.

I don't think this comment applies here. This has nothing to do with selection AFAICT.

[Brady Eidson]

(In reply to Chris Dumez from comment #32)
> Comment on attachment 326221 [details]
> Some small tweaks
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326221&action=review
> 
> Still looking into why getRegistration() fails but here are a few things I
> noticed.
> 
> > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:334
> > +    if (data.activeServiceWorkerIdentifier && (!installingServiceWorker || installingServiceWorker->identifier() != *data.activeServiceWorkerIdentifier)) {
> 
> This behavior change would likely break tests. We used to set
> ScriptExecutionContext::m_activeServiceWorker to the newly *installing*
> worker. You now set it to the *active* worker, which is most cases
> (non-updates) will be null.

It's disappointing that we kludged it together like this.

"Active worker" has a very explicit meaning, but I think what you're saying is that we decided on a completely different meaning...?

Will try without that then...

[Brady Eidson]

Created attachment 326226 [details]
I don't think that fixed it.

[Build Bot]

Comment on attachment 326221 [details]
Some small tweaks

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 326227 [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 326221 [details]
Some small tweaks

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

Number of test failures exceeded the failure limit.

[Build Bot]

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

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

[Chris Dumez]

I may know what's going on. I'm giving it a try locally.

[Build Bot]

Comment on attachment 326226 [details]
I don't think that fixed it.

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/abort/serviceworker-intercepted.https.html
http/tests/workers/service/service-worker-getRegistration.html
imported/w3c/web-platform-tests/service-workers/service-worker/windowclient-navigate.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/update.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/multi-globals/url-parsing.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/clients-matchall-client-types.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/client-navigate.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/import-scripts-redirect.https.html

[Build Bot]

Created attachment 326242 [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

[Chris Dumez]

Created attachment 326244 [details]
Now getRegistration() passes

[Build Bot]

Comment on attachment 326226 [details]
I don't think that fixed it.

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/abort/serviceworker-intercepted.https.html
http/tests/workers/service/service-worker-getRegistration.html
imported/w3c/web-platform-tests/service-workers/service-worker/windowclient-navigate.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/update.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/multi-globals/url-parsing.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/clients-matchall-client-types.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/client-navigate.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/import-scripts-redirect.https.html

[Build Bot]

Created attachment 326245 [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]

(In reply to Chris Dumez from comment #42)
> Created attachment 326244 [details]
> Now getRegistration() passes

Only 2 failures remain:
Regressions: Unexpected text-only failures (2)
  imported/w3c/web-platform-tests/service-workers/service-worker/import-scripts-redirect.https.html [ Failure ]

Not sure why yet.

  imported/w3c/web-platform-tests/service-workers/service-worker/update.https.html [ Failure ]

I'll verify but this looks like a progression, I believe we are now merely failing on the next check.

[Chris Dumez]

Created attachment 326248 [details]
Only one failure left?

[Chris Dumez]

Created attachment 326249 [details]
Only one failure left?

[Chris Dumez]

(In reply to Chris Dumez from comment #45)
> (In reply to Chris Dumez from comment #42)
> > Created attachment 326244 [details]
> > Now getRegistration() passes
> 
> Only 2 failures remain:
> Regressions: Unexpected text-only failures (2)
>  
> imported/w3c/web-platform-tests/service-workers/service-worker/import-
> scripts-redirect.https.html [ Failure ]
> 
> Not sure why yet.
> 
>  
> imported/w3c/web-platform-tests/service-workers/service-worker/update.https.
> html [ Failure ]
> 
> I'll verify but this looks like a progression, I believe we are now merely
> failing on the next check.

imported/w3c/web-platform-tests/service-workers/service-worker/import-scripts-redirect.https.html seems to be stuck at the end of the last test on:
await Promise.all([
      wait_for_update(t, reg),
      reg.update()
    ]);

So may be an issue with firing the updatefound event on a registration after updating it via update().

[Chris Dumez]

(In reply to Chris Dumez from comment #48)
> (In reply to Chris Dumez from comment #45)
> > (In reply to Chris Dumez from comment #42)
> > > Created attachment 326244 [details]
> > > Now getRegistration() passes
> > 
> > Only 2 failures remain:
> > Regressions: Unexpected text-only failures (2)
> >  
> > imported/w3c/web-platform-tests/service-workers/service-worker/import-
> > scripts-redirect.https.html [ Failure ]
> > 
> > Not sure why yet.
> > 
> >  
> > imported/w3c/web-platform-tests/service-workers/service-worker/update.https.
> > html [ Failure ]
> > 
> > I'll verify but this looks like a progression, I believe we are now merely
> > failing on the next check.
> 
> imported/w3c/web-platform-tests/service-workers/service-worker/import-
> scripts-redirect.https.html seems to be stuck at the end of the last test on:
> await Promise.all([
>       wait_for_update(t, reg),
>       reg.update()
>     ]);
> 
> So may be an issue with firing the updatefound event on a registration after
> updating it via update().

This is because we are taking this shortcut:
    // If newestWorker is not null, newestWorker's script url equals job's script url with the exclude fragments
    // flag set, and script's source text is a byte-for-byte match with newestWorker's script resource's source
    // text, then:
    if (newestWorker && equalIgnoringFragmentIdentifier(newestWorker->scriptURL(), job.scriptURL) && result.script == newestWorker->script()) {
        // FIXME: if script is a classic script, and script's module record's [[ECMAScriptCode]] is a byte-for-byte
        // match with newestWorker's script resource's module record's [[ECMAScriptCode]] otherwise.

        // Invoke Resolve Job Promise with job and registration.
        m_server.resolveRegistrationJob(job, registration->data());

        // Invoke Finish Job with job and abort these steps.
        finishCurrentJob();
        return;
    }

in SWServerJobQueue::scriptFetchFinished(). The JS calls update() but the downloaded script is the same so we just return the registration as is, without running the install steps. We therefore do not fire the updatefound event.

This was supposed to be as per spec so the test could be wrong. I'll double check.

[Chris Dumez]

Created attachment 326253 [details]
Should pass all tests

[Brady Eidson]

(In reply to Chris Dumez from comment #50)
> Created attachment 326253 [details]
> Should pass all tests

Nice!

[Chris Dumez]

Comment on attachment 326253 [details]
Should pass all tests

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

Brady, here are the changes I made to your patch. Those were needed because getNewestWorker() started returning non-null on server side after your patch (because you actually set the installing worker on SWServerRegistration now):

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:-79
> -    if (newestWorker && equalIgnoringFragmentIdentifier(newestWorker->scriptURL(), job.scriptURL) && result.script == newestWorker->script()) {

I dropped this optimization. Although it is part of the specification, imported/w3c/web-platform-tests/service-workers/service-worker/import-scripts-redirect.https.html expects that an updatefound event is fired when it calls update(), even though the script has not changed. I think it is safe to drop this for now and re-evaluate later.

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:-254
> -    if (!registration.getNewestWorker())

I dropped this check, it is not technically as per spec and it was causing us to be too conservative and not clear the registration is some cases we should have. For now, I unconditionally call clearRegistration() and clear too aggressively instead until we can implement the full logic in the spec.
This was causing the getRegistration() test to fail because of the way the Match Registration algorithm works (https://www.w3.org/TR/service-workers-1/#scope-match-algorithm). At the last step, if we find a matching registration but its uninstall flag is set, we return null, even though there was potentially another suitable match that was NOT uninstalling. Now that we always clear the uninstalled registrations, this no longer happens and we'll find the remaining matching registration (not the already uninstalled one).

[Chris Dumez]

Comment on attachment 326253 [details]
Should pass all tests

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

I'll let you get the patch ready for review.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:369
> +    // FIXME: Implement proper selection of service workers.

I do not think this comment belong here. The code below looks correct to me.

[Brady Eidson]

(In reply to Chris Dumez from comment #53)
> Comment on attachment 326253 [details]
> Should pass all tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326253&action=review
> 
> I'll let you get the patch ready for review.
> 
> > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:369
> > +    // FIXME: Implement proper selection of service workers.
> 
> I do not think this comment belong here. The code below looks correct to me.

This entire method is a mess and I have a feeling it's because no proper selection, but, fine. Once in the method is probably enough

[Chris Dumez]

Comment on attachment 326253 [details]
Should pass all tests

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

>>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:369
>>> +    // FIXME: Implement proper selection of service workers.
>> 
>> I do not think this comment belong here. The code below looks correct to me.
> 
> This entire method is a mess and I have a feeling it's because no proper selection, but, fine. Once in the method is probably enough

I am planning to clean it up in a follow-up. The context->setActiveServiceWorker() is a hack to work around our lack of selection, and we need to keep it for now. Some of the other logic was to avoid constructing a new ServiceWorker object if it was the same as the  context->activeServiceWorker(), as an optimization.

[Brady Eidson]

Created attachment 326256 [details]
Possibly for review

[Chris Dumez]

Comment on attachment 326256 [details]
Possibly for review

r=me

[WebKit Commit Bot]

Comment on attachment 326256 [details]
Possibly for review

Clearing flags on attachment: 326256

Committed r224553: <https://trac.webkit.org/changeset/224553>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35567110>