This is useful for running WPT test and is a prerequisite for handling navigation requests through SW
Created attachment 325049 [details] Patch
Created attachment 325050 [details] Patch
Comment on attachment 325050 [details] Patch Clearing flags on attachment: 325050 Committed r224051: <https://trac.webkit.org/changeset/224051>
All reviewed patches have been landed. Closing bug.
Reopening for full implementation of the method
Created attachment 325072 [details] Patch
Skipping SW tests until we can post message back the results.
Comment on attachment 325072 [details] Patch Clearing flags on attachment: 325072 Committed r224066: <https://trac.webkit.org/changeset/224066>
Reopening to attach new patch.
Created attachment 325119 [details] Patch
Skipped more tests that were failing in https://trac.webkit.org/changeset/224112/webkit
Comment on attachment 325119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325119&action=review > Source/WebCore/workers/service/server/SWServer.cpp:288 > + return selectedRegistration && !selectedRegistration->isUninstallingFlagSet() ? selectedRegistration : nullptr; Should we check !selectedRegistration->isEmpty() too?
Comment on attachment 325119 [details] Patch Putting as r? View in context: https://bugs.webkit.org/attachment.cgi?id=325119&action=review > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:190 > + Not necessary, but we could probably call hasServiceWorkerRegisteredForOrigin check here to do some early null return. I'll add it as a follow-up or at landing. >> Source/WebCore/workers/service/server/SWServer.cpp:288 >> + return selectedRegistration && !selectedRegistration->isUninstallingFlagSet() ? selectedRegistration : nullptr; > > Should we check !selectedRegistration->isEmpty() too? I am not sure, it seems isEmpty is private and should not be called on the main thread.
Created attachment 325440 [details] Patch
Created attachment 325446 [details] Patch
Comment on attachment 325446 [details] Patch Attachment 325446 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5051474 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/multi-globals/url-parsing.https.html imported/w3c/web-platform-tests/service-workers/service-worker/multiple-register.https.html imported/w3c/web-platform-tests/service-workers/service-worker/serviceworkerobject-scripturl.https.html
Created attachment 325453 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 325454 [details] Patch
Created attachment 325460 [details] Patch
Comment on attachment 325460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325460&action=review > Source/WebCore/workers/service/server/SWServer.cpp:89 > + callOnMainThread([this] () mutable { What keep |this| alive?
Comment on attachment 325460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325460&action=review >> Source/WebCore/workers/service/server/SWServer.cpp:89 >> + callOnMainThread([this] () mutable { > > What keep |this| alive? SWServer has currently the lifetime of the StorageProcess. That said, this is another argument towards trying to move/capture m_registrations. I'll update the patch with this approach.
Comment on attachment 325460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325460&action=review > Source/WebCore/workers/service/server/SWServer.cpp:87 > +void SWServer::lastTaskBeforeClearing() Do we really need method for this. Cannot we merely use a lambda to construct the task? This would solve the fact that I don't like this method name :P > Source/WebCore/workers/service/server/SWServer.cpp:230 > + if (m_isClearing) This looks wrong. If we: 1. Clear() 2. Append a RegisterTask I would expected the registration to succeed but in your case, it will be ignored. > Source/WebCore/workers/service/server/SWServer.cpp:288 > + return selectedRegistration && !selectedRegistration->isUninstallingFlagSet() ? selectedRegistration : nullptr; As mentioned in an earlier iteration, we should make SWServerRegistration::isEmpty() public and make sure we do not return an empty registration. An empty registration in our implementation is equivalent to a null registration in the spec. Also, I think we should do the uninstalling flag and isEmpty() checks inside the loop to skip them. Otherwise, we may overlook a registration that matches but has a shorter URL than a registration that is uninstalled.
(In reply to Chris Dumez from comment #23) > Comment on attachment 325460 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=325460&action=review > > > Source/WebCore/workers/service/server/SWServer.cpp:87 > > +void SWServer::lastTaskBeforeClearing() > > Do we really need method for this. Cannot we merely use a lambda to > construct the task? This would solve the fact that I don't like this method > name :P > > > Source/WebCore/workers/service/server/SWServer.cpp:230 > > + if (m_isClearing) > > This looks wrong. If we: > 1. Clear() > 2. Append a RegisterTask > > I would expected the registration to succeed but in your case, it will be > ignored. I don't think it is a big deal since this will only happen when user is clearing its cache/stored data. But, yeah, I agree we actually might want to move/capture the map. > > Source/WebCore/workers/service/server/SWServer.cpp:288 > > + return selectedRegistration && !selectedRegistration->isUninstallingFlagSet() ? selectedRegistration : nullptr; > > As mentioned in an earlier iteration, we should make > SWServerRegistration::isEmpty() public and make sure we do not return an > empty registration. An empty registration in our implementation is > equivalent to a null registration in the spec. isEmpty relates to update via cache currently, not to whether a registration is null. Having a null registration in a map is adding complexity to me. It seems much simpler and closer to spec to not have the concept of a null registration and to remove registration from the map. > Also, I think we should do the uninstalling flag and isEmpty() checks inside > the loop to skip them. Otherwise, we may overlook a registration that > matches but has a shorter URL than a registration that is uninstalled. The spec is written the way it is implemented: the uninstalling flag is checked at return time, not in the loop. I was also surprised by that but prefer sticking to the spec right now and see later on whether we can change the spec or not.
I will update the patch to move/capture the map instead of clearing it afterwards
After discussion, plan is to have m_registrations as the scope to registration map. Another map might be introduced for scope to job queues in a future refactoring. This will simplify the model and address Chris concern on this patch. I'll reupload a new patch with m_registrations being cleared through move/lambda-capture. At some point, we might even be able to clear m_registrations right away once the scope to job queue map is added.
Comment on attachment 325460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325460&action=review > Source/WebCore/workers/service/ServiceWorkerRegistrationKey.h:54 > + encoder << clientCreationURL << topOrigin << scopeURL; Note that I am personally unclear what ServiceWorkerRegistrationKey matches to in the spec. In the spec, the key of a registration in the scope to registration hash map is the scopeURL (without fragment). Why are we using clientCreationURL / topOrigin? > Source/WebCore/workers/service/server/SWServer.cpp:74 > + m_taskQueue.append(createCrossThreadTask(*this, &SWServer::lastTaskBeforeClearing)); For my sake, can you clarify why it is not OK to just do: m_registrations.clear(); here from the main thread? Why do we need to wait of its job to execute since we are going to destroy it anyway?
We are segregating sw according the top origin like done for itp, so that foo.com sw is different from foo.com sw in iframe from bar.com
Need to look at the class precisely, but we may be able to replace the origin by the scope url since it contains the origin.
AS of clearing the map right away, this is causing issue with the job queue lifetime and the background thread.
Created attachment 325667 [details] Patch
Comment on attachment 325667 [details] Patch Attachment 325667 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5070424 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/update-bytecheck.https.html
Created attachment 325673 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 325667 [details] Patch Attachment 325667 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5070504 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/update-bytecheck.https.html
Created attachment 325677 [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
Created attachment 325686 [details] Patch
Comment on attachment 325686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325686&action=review > Source/WebCore/workers/service/ServiceWorkerJobData.cpp:46 > + return { clientCreationURL, topOrigin, scopeURL }; Aren't we supposed to clear the fragment from the scopeURL as per the spec? > Source/WebCore/workers/service/server/SWServer.cpp:92 > + m_registrations.clear(); In a follow up, we probably want to go through each registration and call clearRegistration(registration) (once the patch for clearRegistration() lands). clearRegistration() is supposed to take care of terminating the service worker, among other things. > Source/WebCore/workers/service/server/SWServer.cpp:285 > + return (selectedRegistration && !selectedRegistration->isUninstallingFlagSet()) ? selectedRegistration : nullptr; !selectedRegistration->isUninstalling() > Source/WebCore/workers/service/server/SWServerJobQueue.cpp:103 > + if (!registration) { How can this happen? If you remove the job queue from m_jobQueues (when you clear registrations), then scriptContextStarted() should never be called on a jobQueue that has no registration. > Source/WebCore/workers/service/server/SWServerRegistration.h:48 > + const URL& scopeURL() const { return m_scopeURL; } Cannot we use key().scopeURL ? I also think we can get rid of m_scopeURL now that you added it to the key, no? > Source/WebCore/workers/service/server/SWServerRegistration.h:49 > + bool isUninstallingFlagSet() const { return m_uninstalling; } Not needed, we have isUninstalling() getter below. > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/update-bytecheck.https-expected.txt:2 > +Harness Error (TIMEOUT), message = null I do not think you should commit this new baseline given that it is worse and you marked the test as flaky.
(In reply to Chris Dumez from comment #37) > Comment on attachment 325686 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=325686&action=review > > > Source/WebCore/workers/service/ServiceWorkerJobData.cpp:46 > > + return { clientCreationURL, topOrigin, scopeURL }; > > Aren't we supposed to clear the fragment from the scopeURL as per the spec? Right. > > Source/WebCore/workers/service/server/SWServer.cpp:92 > > + m_registrations.clear(); > > In a follow up, we probably want to go through each registration and call > clearRegistration(registration) (once the patch for clearRegistration() > lands). clearRegistration() is supposed to take care of terminating the > service worker, among other things. Or it could be done in the destructor of the registration. > > Source/WebCore/workers/service/server/SWServerRegistration.h:48 > > + const URL& scopeURL() const { return m_scopeURL; } > > Cannot we use key().scopeURL ? I also think we can get rid of m_scopeURL now > that you added it to the key, no? key().scopeURL will be without frag ID, m_scopeURL has frag id that is used to recreate the registration data that is exposed to scripts. Maybe SWServerRegistration should have its own registration data instead of recreating one when needed.
Created attachment 325777 [details] Patch
Created attachment 325792 [details] Patch
Created attachment 325802 [details] Patch
Attachment 325802 [details] did not pass style-queue: ERROR: Source/WebCore/workers/service/server/SWServer.cpp:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 325802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325802&action=review r=me with comments. > Source/WebCore/page/SecurityOriginData.h:86 > +inline bool operator!=(const SecurityOriginData& first, const SecurityOriginData& second) { return !(first == second); } This is a bit inefficient. If you implemented a real !=, we would be able to abort early as soon as one of the component does not match. I'd prefer we provide a proper implementation. > Source/WebCore/workers/service/ServiceWorkerRegistrationKey.h:38 > + URL scope; As mentioned earlier, I think this is error prone. It makes it looks like we have the full scopeURL while this does not container the fragment. I think we should either: 1. Use String scopeString; as in the spec or 2. Rename to scopeWithoutFragment.
Comment on attachment 325802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325802&action=review >> Source/WebCore/page/SecurityOriginData.h:86 >> +inline bool operator!=(const SecurityOriginData& first, const SecurityOriginData& second) { return !(first == second); } > > This is a bit inefficient. If you implemented a real !=, we would be able to abort early as soon as one of the component does not match. I'd prefer we provide a proper implementation. That doesn’t sound right. This is not different between != and ==. The real == can also abort early as soon as one of the components does not match.
> > Source/WebCore/workers/service/ServiceWorkerRegistrationKey.h:38 > > + URL scope; > > As mentioned earlier, I think this is error prone. It makes it looks like we > have the full scopeURL while this does not container the fragment. I think > we should either: > 1. Use String scopeString; as in the spec > or > 2. Rename to scopeWithoutFragment. I would therefore prefer keeping scope. Once created, we should really manipulate the registration key as a whole and not one field inside it. Let's make it private and kept as an URL which might be useful for comparison.
(In reply to Darin Adler from comment #44) > Comment on attachment 325802 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=325802&action=review > > >> Source/WebCore/page/SecurityOriginData.h:86 > >> +inline bool operator!=(const SecurityOriginData& first, const SecurityOriginData& second) { return !(first == second); } > > > > This is a bit inefficient. If you implemented a real !=, we would be able to abort early as soon as one of the component does not match. I'd prefer we provide a proper implementation. > > That doesn’t sound right. This is not different between != and ==. The real > == can also abort early as soon as one of the components does not match. Right. After a good night sleep, I do think this comment was wrong. Sorry about that.
Created attachment 325911 [details] Patch for landing
Created attachment 325914 [details] Rebasing
Created attachment 325915 [details] Rebasing
Comment on attachment 325915 [details] Rebasing Clearing flags on attachment: 325915 Committed r224408: <https://trac.webkit.org/changeset/224408>
<rdar://problem/35568061>