Bug 178882 – Implement Service Worker Matching Registration algorithm

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

This is useful for running WPT test and is a prerequisite for handling navigation requests through SW

[youenn fablet]

Created attachment 325049 [details]
Patch

[youenn fablet]

Created attachment 325050 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 325050 [details]
Patch

Clearing flags on attachment: 325050

Committed r224051: <https://trac.webkit.org/changeset/224051>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Reopening for full implementation of the method

[youenn fablet]

Created attachment 325072 [details]
Patch

[youenn fablet]

Skipping SW tests until we can post message back the results.

[WebKit Commit Bot]

Comment on attachment 325072 [details]
Patch

Clearing flags on attachment: 325072

Committed r224066: <https://trac.webkit.org/changeset/224066>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 325119 [details]
Patch

[Ryan Haddad]

Skipped more tests that were failing in https://trac.webkit.org/changeset/224112/webkit

[Chris Dumez]

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?

[youenn fablet]

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.

[youenn fablet]

Created attachment 325440 [details]
Patch

[youenn fablet]

Created attachment 325446 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 325454 [details]
Patch

[youenn fablet]

Created attachment 325460 [details]
Patch

[Chris Dumez]

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?

[youenn fablet]

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.

[Chris Dumez]

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.

[youenn fablet]

(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.

[youenn fablet]

I will update the patch to move/capture the map instead of clearing it afterwards

[youenn fablet]

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.

[Chris Dumez]

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?

[youenn fablet]

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

[youenn fablet]

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.

[youenn fablet]

AS of clearing the map right away, this is causing issue with the job queue lifetime and the background thread.

[youenn fablet]

Created attachment 325667 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 325686 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

(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.

[youenn fablet]

Created attachment 325777 [details]
Patch

[youenn fablet]

Created attachment 325792 [details]
Patch

[youenn fablet]

Created attachment 325802 [details]
Patch

[Build Bot]

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.

[Chris Dumez]

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.

[Darin Adler]

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.

[youenn fablet]

> > 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.

[Chris Dumez]

(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.

[youenn fablet]

Created attachment 325911 [details]
Patch for landing

[youenn fablet]

Created attachment 325914 [details]
Rebasing

[youenn fablet]

Created attachment 325915 [details]
Rebasing

[WebKit Commit Bot]

Comment on attachment 325915 [details]
Rebasing

Clearing flags on attachment: 325915

Committed r224408: <https://trac.webkit.org/changeset/224408>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35568061>