Bug 179318

Summary: Implement "UpdateWorkerState"
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates, ggaren, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP, trying to reconcile that microtask without causing test failures
none
WIP to take home
buildbot: commit-queue-
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Getting close to review, lingering test issue(s)
none
Getting closer
buildbot: commit-queue-
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Some small tweaks
buildbot: commit-queue-
I don't think that fixed it.
buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Now getRegistration() passes
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Only one failure left?
none
Only one failure left?
none
Should pass all tests
none
Possibly for review none

Description Brady Eidson 2017-11-06 07:28:04 PST
Implement "UpdateWorkerState", "fire updatefound event", and actually rely on the real update/install algorithms
Comment 1 Brady Eidson 2017-11-06 07:28:54 PST
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.
Comment 2 Brady Eidson 2017-11-06 07:29:30 PST
Created attachment 326120 [details]
WIP, trying to reconcile that microtask without causing test failures
Comment 3 Chris Dumez 2017-11-06 09:30:26 PST
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.
Comment 4 Brady Eidson 2017-11-06 11:51:49 PST
(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.
Comment 5 Brady Eidson 2017-11-06 17:10:47 PST
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)
Comment 6 Build Bot 2017-11-06 17:12:09 PST
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.
Comment 7 Build Bot 2017-11-06 17:56:22 PST
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.
Comment 8 Build Bot 2017-11-06 17:56:23 PST
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
Comment 9 Build Bot 2017-11-06 18:26:16 PST
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.
Comment 10 Build Bot 2017-11-06 18:26:17 PST
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
Comment 11 Chris Dumez 2017-11-06 21:10:10 PST
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
Comment 12 Chris Dumez 2017-11-06 21:39:12 PST
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*>.
Comment 13 Chris Dumez 2017-11-06 21:41:46 PST
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.
Comment 14 Brady Eidson 2017-11-06 21:53:38 PST
Created attachment 326191 [details]
Getting close to review, lingering test issue(s)
Comment 15 Brady Eidson 2017-11-06 21:55:04 PST
(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.
Comment 16 Brady Eidson 2017-11-06 21:55:45 PST
(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.
Comment 17 Brady Eidson 2017-11-06 21:56:20 PST
Created attachment 326192 [details]
Getting closer
Comment 18 Chris Dumez 2017-11-06 22:07:56 PST
(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.
Comment 19 Chris Dumez 2017-11-06 22:17:42 PST
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?
Comment 20 Chris Dumez 2017-11-06 22:24:58 PST
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.
Comment 21 Brady Eidson 2017-11-06 22:31:58 PST
(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.
Comment 22 Brady Eidson 2017-11-06 22:33:46 PST
(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)
Comment 23 Chris Dumez 2017-11-06 22:43:28 PST
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?
Comment 24 Build Bot 2017-11-06 22:51:51 PST
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
Comment 25 Build Bot 2017-11-06 22:51:52 PST
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
Comment 26 Build Bot 2017-11-06 23:23:42 PST
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
Comment 27 Build Bot 2017-11-06 23:23:43 PST
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
Comment 28 Brady Eidson 2017-11-07 08:55:26 PST
(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...
Comment 29 Chris Dumez 2017-11-07 09:03:44 PST
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.
Comment 30 Brady Eidson 2017-11-07 10:30:16 PST
Created attachment 326221 [details]
Some small tweaks
Comment 31 Brady Eidson 2017-11-07 10:52:24 PST
> Not sure what goes to active means here

Me neither - It was already here before me!
Comment 32 Chris Dumez 2017-11-07 11:00:32 PST
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.
Comment 33 Brady Eidson 2017-11-07 11:17:57 PST
(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...
Comment 34 Brady Eidson 2017-11-07 11:25:14 PST
Created attachment 326226 [details]
I don't think that fixed it.
Comment 35 Build Bot 2017-11-07 11:32:47 PST
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.
Comment 36 Build Bot 2017-11-07 11:32:48 PST
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
Comment 37 Build Bot 2017-11-07 11:51:59 PST
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.
Comment 38 Build Bot 2017-11-07 11:52:01 PST
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
Comment 39 Chris Dumez 2017-11-07 12:30:38 PST
I may know what's going on. I'm giving it a try locally.
Comment 40 Build Bot 2017-11-07 12:48:35 PST
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
Comment 41 Build Bot 2017-11-07 12:48:37 PST
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
Comment 42 Chris Dumez 2017-11-07 12:58:32 PST
Created attachment 326244 [details]
Now getRegistration() passes
Comment 43 Build Bot 2017-11-07 13:00:03 PST
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
Comment 44 Build Bot 2017-11-07 13:00:04 PST
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
Comment 45 Chris Dumez 2017-11-07 13:07:15 PST
(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.
Comment 46 Chris Dumez 2017-11-07 13:08:49 PST
Created attachment 326248 [details]
Only one failure left?
Comment 47 Chris Dumez 2017-11-07 13:09:29 PST
Created attachment 326249 [details]
Only one failure left?
Comment 48 Chris Dumez 2017-11-07 13:13:42 PST
(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().
Comment 49 Chris Dumez 2017-11-07 13:27:55 PST
(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.
Comment 50 Chris Dumez 2017-11-07 13:33:33 PST
Created attachment 326253 [details]
Should pass all tests
Comment 51 Brady Eidson 2017-11-07 13:38:37 PST
(In reply to Chris Dumez from comment #50)
> Created attachment 326253 [details]
> Should pass all tests

Nice!
Comment 52 Chris Dumez 2017-11-07 13:40:13 PST
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).
Comment 53 Chris Dumez 2017-11-07 13:45:31 PST
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.
Comment 54 Brady Eidson 2017-11-07 13:53:55 PST
(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
Comment 55 Chris Dumez 2017-11-07 13:56:53 PST
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.
Comment 56 Brady Eidson 2017-11-07 13:59:31 PST
Created attachment 326256 [details]
Possibly for review
Comment 57 Chris Dumez 2017-11-07 14:46:55 PST
Comment on attachment 326256 [details]
Possibly for review

r=me
Comment 58 WebKit Commit Bot 2017-11-07 15:06:59 PST
Comment on attachment 326256 [details]
Possibly for review

Clearing flags on attachment: 326256

Committed r224553: <https://trac.webkit.org/changeset/224553>
Comment 59 WebKit Commit Bot 2017-11-07 15:07:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 60 Radar WebKit Bug Importer 2017-11-15 12:11:25 PST
<rdar://problem/35567110>