Bug 178882 - Implement Service Worker Matching Registration algorithm
Summary: Implement Service Worker Matching Registration algorithm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-26 13:24 PDT by youenn fablet
Modified: 2017-11-15 12:44 PST (History)
11 users (show)

See Also:


Attachments
Patch (1.93 KB, patch)
2017-10-26 13:31 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (2.99 KB, patch)
2017-10-26 13:34 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (4.61 KB, patch)
2017-10-26 16:05 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (37.44 KB, patch)
2017-10-26 21:15 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (35.43 KB, patch)
2017-10-31 08:04 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (40.84 KB, patch)
2017-10-31 09:01 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.60 MB, application/zip)
2017-10-31 09:50 PDT, Build Bot
no flags Details
Patch (46.10 KB, patch)
2017-10-31 10:02 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (45.25 KB, patch)
2017-10-31 10:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (48.48 KB, patch)
2017-11-01 20:00 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.20 MB, application/zip)
2017-11-01 21:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.01 MB, application/zip)
2017-11-01 21:24 PDT, Build Bot
no flags Details
Patch (48.92 KB, patch)
2017-11-01 22:53 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (46.00 KB, patch)
2017-11-02 15:14 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (39.79 KB, patch)
2017-11-02 16:26 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (41.15 KB, patch)
2017-11-02 17:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (48.58 KB, patch)
2017-11-03 10:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (46.80 KB, patch)
2017-11-03 10:38 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (48.89 KB, patch)
2017-11-03 10:46 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-10-26 13:24:43 PDT
This is useful for running WPT test and is a prerequisite for handling navigation requests through SW
Comment 1 youenn fablet 2017-10-26 13:31:01 PDT
Created attachment 325049 [details]
Patch
Comment 2 youenn fablet 2017-10-26 13:34:42 PDT
Created attachment 325050 [details]
Patch
Comment 3 WebKit Commit Bot 2017-10-26 14:41:22 PDT
Comment on attachment 325050 [details]
Patch

Clearing flags on attachment: 325050

Committed r224051: <https://trac.webkit.org/changeset/224051>
Comment 4 WebKit Commit Bot 2017-10-26 14:41:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 youenn fablet 2017-10-26 14:43:44 PDT
Reopening for full implementation of the method
Comment 6 youenn fablet 2017-10-26 16:05:07 PDT
Created attachment 325072 [details]
Patch
Comment 7 youenn fablet 2017-10-26 16:05:35 PDT
Skipping SW tests until we can post message back the results.
Comment 8 WebKit Commit Bot 2017-10-26 16:37:37 PDT
Comment on attachment 325072 [details]
Patch

Clearing flags on attachment: 325072

Committed r224066: <https://trac.webkit.org/changeset/224066>
Comment 9 WebKit Commit Bot 2017-10-26 16:37:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 youenn fablet 2017-10-26 21:15:10 PDT
Reopening to attach new patch.
Comment 11 youenn fablet 2017-10-26 21:15:11 PDT
Created attachment 325119 [details]
Patch
Comment 12 Ryan Haddad 2017-10-27 09:09:48 PDT
Skipped more tests that were failing in https://trac.webkit.org/changeset/224112/webkit
Comment 13 Chris Dumez 2017-10-27 10:47:12 PDT
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 14 youenn fablet 2017-10-27 15:22:08 PDT
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.
Comment 15 youenn fablet 2017-10-31 08:04:15 PDT
Created attachment 325440 [details]
Patch
Comment 16 youenn fablet 2017-10-31 09:01:36 PDT
Created attachment 325446 [details]
Patch
Comment 17 Build Bot 2017-10-31 09:50:23 PDT
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
Comment 18 Build Bot 2017-10-31 09:50:25 PDT
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
Comment 19 youenn fablet 2017-10-31 10:02:57 PDT
Created attachment 325454 [details]
Patch
Comment 20 youenn fablet 2017-10-31 10:59:31 PDT
Created attachment 325460 [details]
Patch
Comment 21 Chris Dumez 2017-10-31 11:57:22 PDT
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 22 youenn fablet 2017-10-31 13:05:09 PDT
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 23 Chris Dumez 2017-10-31 14:58:20 PDT
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.
Comment 24 youenn fablet 2017-10-31 15:23:33 PDT
(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.
Comment 25 youenn fablet 2017-10-31 15:25:34 PDT
I will update the patch to move/capture the map instead of clearing it afterwards
Comment 26 youenn fablet 2017-11-01 08:32:04 PDT
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 27 Chris Dumez 2017-11-01 09:02:31 PDT
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?
Comment 28 youenn fablet 2017-11-01 11:03:48 PDT
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
Comment 29 youenn fablet 2017-11-01 11:12:21 PDT
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.
Comment 30 youenn fablet 2017-11-01 11:13:16 PDT
AS of clearing the map right away, this is causing issue with the job queue lifetime and the background thread.
Comment 31 youenn fablet 2017-11-01 20:00:59 PDT
Created attachment 325667 [details]
Patch
Comment 32 Build Bot 2017-11-01 21:05:27 PDT
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
Comment 33 Build Bot 2017-11-01 21:05:28 PDT
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 34 Build Bot 2017-11-01 21:24:14 PDT
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
Comment 35 Build Bot 2017-11-01 21:24:15 PDT
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
Comment 36 youenn fablet 2017-11-01 22:53:03 PDT
Created attachment 325686 [details]
Patch
Comment 37 Chris Dumez 2017-11-02 09:13:49 PDT
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.
Comment 38 youenn fablet 2017-11-02 12:51:25 PDT
(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.
Comment 39 youenn fablet 2017-11-02 15:14:42 PDT
Created attachment 325777 [details]
Patch
Comment 40 youenn fablet 2017-11-02 16:26:11 PDT
Created attachment 325792 [details]
Patch
Comment 41 youenn fablet 2017-11-02 17:35:35 PDT
Created attachment 325802 [details]
Patch
Comment 42 Build Bot 2017-11-02 17:37:52 PDT
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 43 Chris Dumez 2017-11-02 19:11:37 PDT
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 44 Darin Adler 2017-11-03 07:33:49 PDT
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.
Comment 45 youenn fablet 2017-11-03 09:05:09 PDT
> > 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.
Comment 46 Chris Dumez 2017-11-03 09:12:02 PDT
(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.
Comment 47 youenn fablet 2017-11-03 10:30:05 PDT
Created attachment 325911 [details]
Patch for landing
Comment 48 youenn fablet 2017-11-03 10:38:06 PDT
Created attachment 325914 [details]
Rebasing
Comment 49 youenn fablet 2017-11-03 10:46:30 PDT
Created attachment 325915 [details]
Rebasing
Comment 50 WebKit Commit Bot 2017-11-03 11:21:50 PDT
Comment on attachment 325915 [details]
Rebasing

Clearing flags on attachment: 325915

Committed r224408: <https://trac.webkit.org/changeset/224408>
Comment 51 WebKit Commit Bot 2017-11-03 11:21:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 52 Radar WebKit Bug Importer 2017-11-15 12:44:28 PST
<rdar://problem/35568061>