1195980 - wpt service_worker_unregister() function does not perform exact matching on scope parameter

Status: RESOLVED DUPLICATE of bug 1180754

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

While working on tests for bug 1184607 I discovered if you attempt to register two service workers for these scopes:

  /some/scopeislong
  /some/scope

The second registration will overwrite the first.  This is incorrect.  These two scopes are peers and should be able to exist at the same time.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

The problem is using StringBeginsWith():

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#1054
  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#3343

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Actually, the spec says to do this.  I think thats a bug in the spec, though:

  https://github.com/slightlyoff/ServiceWorker/issues/734

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

It was pointed out that the spec language is confusing, but should not lead to this result.

So it seems its just a gecko issue.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

To see this:

1) apply the patches in bug 1184607
2) Modify fetch-event-redirect.https.html to remove the '-nocreds' at the end of one of the test's name fields.  This will make the scope be a substring of a later scope.
3) Run the tests and see the test times out.  I believe this is because the later test overwrites the registration since wpt runs all test cases in parallel.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Ben Kelly [:bkelly] from comment #4)
> To see this:
> 
> 1) apply the patches in bug 1184607
> 2) Modify fetch-event-redirect.https.html to remove the '-nocreds' at the
> end of one of the test's name fields.  This will make the scope be a
> substring of a later scope.
> 3) Run the tests and see the test times out.  I believe this is because the
> later test overwrites the registration since wpt runs all test cases in
> parallel.

Our implementation follows the spec correctly. I believe the bug is that this test uses unregister_and_register() which first tries to unregister. If we continue using that function, we will need non-substring scopes.
Here is what happens (with suffixes "sameorigin" and "sameorigin-nocreds")

1) unregister_and_register("sameorigin") creates a registration for "sameorigin".
2) unregister_and_register("sameorigin-nocreds") -> getRegistration("sameorigin-nocreds").unregister().
Now getRegistration() is defined as fetching the registration most suitable for a URL, in this case that is "sameorigin", so it unregisters "sameorigin"
3) Eventual test failure for "sameorigin"

With "sameorigin-creds", step 2 does not lead to unregistration.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

The unregister should be using the "Get Registration" algorithm and not "getRegistration()".  Step 2.2:

  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#unregister-algorithm

The "Get Registration" algorithm does an exact scope key match.  Step 4.1:

  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#get-registration-algorithm

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

That's not what I meant. Here https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/resources/test-helpers.sub.js#17 it uses getRegistration("sameorigin-creds") which returns the reg for "sameorigin", which then gets unregister()ed.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Ah, ok.  So its a bug in the wpt service-worker test framework.  Thanks for investigating!

Lets leave this open to see if we can fix those helper functions while fixing the wpt tests.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Patch in another bug fixes this.