WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
179318
Implement "UpdateWorkerState"
https://bugs.webkit.org/show_bug.cgi?id=179318
Summary
Implement "UpdateWorkerState"
Brady Eidson
Reported
2017-11-06 07:28:04 PST
Implement "UpdateWorkerState", "fire updatefound event", and actually rely on the real update/install algorithms
Attachments
WIP, trying to reconcile that microtask without causing test failures
(33.54 KB, patch)
2017-11-06 07:29 PST
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
WIP to take home
(36.96 KB, patch)
2017-11-06 17:10 PST
,
Brady Eidson
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
(1.44 MB, application/zip)
2017-11-06 17:56 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews125 for ios-simulator-wk2
(1.48 MB, application/zip)
2017-11-06 18:26 PST
,
Build Bot
no flags
Details
Getting close to review, lingering test issue(s)
(41.45 KB, patch)
2017-11-06 21:53 PST
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Getting closer
(41.45 KB, patch)
2017-11-06 21:56 PST
,
Brady Eidson
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
(1.51 MB, application/zip)
2017-11-06 22:51 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews125 for ios-simulator-wk2
(1.13 MB, application/zip)
2017-11-06 23:23 PST
,
Build Bot
no flags
Details
Some small tweaks
(41.66 KB, patch)
2017-11-07 10:30 PST
,
Brady Eidson
buildbot
: commit-queue-
Details
Formatted Diff
Diff
I don't think that fixed it.
(41.80 KB, patch)
2017-11-07 11:25 PST
,
Brady Eidson
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
(3.09 MB, application/zip)
2017-11-07 11:32 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews121 for ios-simulator-wk2
(2.76 MB, application/zip)
2017-11-07 11:52 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
(3.33 MB, application/zip)
2017-11-07 12:48 PST
,
Build Bot
no flags
Details
Now getRegistration() passes
(42.56 KB, patch)
2017-11-07 12:58 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2
(2.26 MB, application/zip)
2017-11-07 13:00 PST
,
Build Bot
no flags
Details
Only one failure left?
(38.18 KB, patch)
2017-11-07 13:08 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Only one failure left?
(43.29 KB, patch)
2017-11-07 13:09 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Should pass all tests
(44.67 KB, patch)
2017-11-07 13:33 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Possibly for review
(50.68 KB, patch)
2017-11-07 13:59 PST
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(18)
View All
Add attachment
proposed patch, testcase, etc.
Brady Eidson
Comment 1
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.
Brady Eidson
Comment 2
2017-11-06 07:29:30 PST
Created
attachment 326120
[details]
WIP, trying to reconcile that microtask without causing test failures
Chris Dumez
Comment 3
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.
Brady Eidson
Comment 4
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.
Brady Eidson
Comment 5
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)
Build Bot
Comment 6
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.
Build Bot
Comment 7
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.
Build Bot
Comment 8
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
Build Bot
Comment 9
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.
Build Bot
Comment 10
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
Chris Dumez
Comment 11
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
Chris Dumez
Comment 12
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*>.
Chris Dumez
Comment 13
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.
Brady Eidson
Comment 14
2017-11-06 21:53:38 PST
Created
attachment 326191
[details]
Getting close to review, lingering test issue(s)
Brady Eidson
Comment 15
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.
Brady Eidson
Comment 16
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.
Brady Eidson
Comment 17
2017-11-06 21:56:20 PST
Created
attachment 326192
[details]
Getting closer
Chris Dumez
Comment 18
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.
Chris Dumez
Comment 19
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?
Chris Dumez
Comment 20
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.
Brady Eidson
Comment 21
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.
Brady Eidson
Comment 22
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)
Chris Dumez
Comment 23
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?
Build Bot
Comment 24
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
Build Bot
Comment 25
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
Build Bot
Comment 26
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
Build Bot
Comment 27
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
Brady Eidson
Comment 28
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...
Chris Dumez
Comment 29
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.
Brady Eidson
Comment 30
2017-11-07 10:30:16 PST
Created
attachment 326221
[details]
Some small tweaks
Brady Eidson
Comment 31
2017-11-07 10:52:24 PST
> Not sure what goes to active means here
Me neither - It was already here before me!
Chris Dumez
Comment 32
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.
Brady Eidson
Comment 33
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...
Brady Eidson
Comment 34
2017-11-07 11:25:14 PST
Created
attachment 326226
[details]
I don't think that fixed it.
Build Bot
Comment 35
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.
Build Bot
Comment 36
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
Build Bot
Comment 37
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.
Build Bot
Comment 38
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
Chris Dumez
Comment 39
2017-11-07 12:30:38 PST
I may know what's going on. I'm giving it a try locally.
Build Bot
Comment 40
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
Build Bot
Comment 41
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
Chris Dumez
Comment 42
2017-11-07 12:58:32 PST
Created
attachment 326244
[details]
Now getRegistration() passes
Build Bot
Comment 43
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
Build Bot
Comment 44
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
Chris Dumez
Comment 45
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.
Chris Dumez
Comment 46
2017-11-07 13:08:49 PST
Created
attachment 326248
[details]
Only one failure left?
Chris Dumez
Comment 47
2017-11-07 13:09:29 PST
Created
attachment 326249
[details]
Only one failure left?
Chris Dumez
Comment 48
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().
Chris Dumez
Comment 49
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.
Chris Dumez
Comment 50
2017-11-07 13:33:33 PST
Created
attachment 326253
[details]
Should pass all tests
Brady Eidson
Comment 51
2017-11-07 13:38:37 PST
(In reply to Chris Dumez from
comment #50
)
> Created
attachment 326253
[details]
> Should pass all tests
Nice!
Chris Dumez
Comment 52
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).
Chris Dumez
Comment 53
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.
Brady Eidson
Comment 54
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
Chris Dumez
Comment 55
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.
Brady Eidson
Comment 56
2017-11-07 13:59:31 PST
Created
attachment 326256
[details]
Possibly for review
Chris Dumez
Comment 57
2017-11-07 14:46:55 PST
Comment on
attachment 326256
[details]
Possibly for review r=me
WebKit Commit Bot
Comment 58
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
>
WebKit Commit Bot
Comment 59
2017-11-07 15:07:01 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 60
2017-11-15 12:11:25 PST
<
rdar://problem/35567110
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug