1201498 - Service worker update should compare scriptURL to worker URL without fragment

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

See:

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

This effects step 25 of the update algorithm:

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

And this is the relevent gecko code:

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

Comment 1

[(no longer active)]

I looked at this briefly.  If we register a worker with a fragment in its script URL and then update() it, in ContinueUpdate() both workerInfo->ScriptSpec() and mRegistration->mScriptSpec include the fragment in the URL, so it looks like there is nothing to do here?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Consider:

1) Page registers a service worker foo.js#bar
2) Later the same page registers a service worker foo.js#snafu
3) Second register triggers an update()

It seems the update algorithm should treat these as the same script since the server just sees foo.js.  With our current code I think we end up taking the wrong path in the update algorithm.

Comment 3

[(no longer active)]

You're right!

Comment 4

[(no longer active)]

As I was trying to simulate comment 2 in a test, I found out that we end up with a new registration object in step 2.  It seems that the reason is this function: <https://dxr.mozilla.org/mozilla-central/rev/6c7c983bce46a460c2766fbdd73283f6d2b03a69/dom/workers/ServiceWorkerManager.cpp#2622>  Here we read the origin suffix, which will always be an empty string with codebase principals with https URLs, so the hashtable lookup in mRegistrationInfos fails.  It seems to me that this means that re-registering even the same URL always results in a new registration, which sounds wrong.

Andrea, you have added this code in ServiceWorkerRegistrationJob::Start() in bug 1162088.  Am I missing something, or is this actually wrong?  Don't we want to use GetServiceWorkerRegistrationInfo() or something like that...

Comment 5

[(no longer active)]

Unassigning myself since I'm still waiting on comment 4 and will probably not have enough time to do anything here.  I have a pretty boring test WIP which I will attach shortly.

Comment 6

[(no longer active)]

Attachment: WIP test for bug 1201498

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

What PrincipalToScopeKey does is completely non-related to this issue. That creates a map of what a principal can load or not.
Here the issue is that we use the spec of the script URL instead the spec without ref.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

We should uplift this to aurora.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8685480 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: ServiceWorker
[User impact if declined]: if the SW script URL contains a ref, we consider it as a new script instead checking if there is a previous script with ref in the URI.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: none. We just use GetSpecIgnoringRef instead GetSpec.
[String/UUID change made/needed]: none

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27f5e66b3919

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for w(8) failures: https://treeherder.mozilla.org/logviewer.html#?job_id=17087978&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/67f8880bc989

Comment 12

[Andrea Marchesini [:baku]]

So, we have a WPT that wants the code as it is...
bkelly, can you help me to check what the spec says about this?

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Hmm, it was fixed slightly differently in the spec.  See the outcome of this issue:

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

Not sure if the test is correct for the spec either.

Comment 14

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92bbfb9abef9

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Baku, could you please describe the test coverage and manual testing if any that was done on this patch? When I see test coverage "None" I don't feel very confident taking this patch in Aurora44. Thanks!

Comment 16

[Andrea Marchesini [:baku]]

I wrote 'none' because we don't have a specific test for this use-case. But I tested in 2 ways:
1. I had to fix an existing WPT in order to be compatible with the spec.
2. I used gdb to follow if the correct patch is done when I was debugging point 1.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/92bbfb9abef9

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8685480 [details] [diff] [review]
patch

Given that this was manually tested and that SW plans to ship in 44, let's uplift to Aurora44.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b6161cc379b

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4b6161cc379b

Comment 21

[(no longer active)]

Andrea, was the test I wrote for this completely wrong?  I'm trying to understand why this landed without a test...

Comment 22

[Andrea Marchesini [:baku]]

(In reply to :Ehsan Akhgari from comment #21)
> Andrea, was the test I wrote for this completely wrong?  I'm trying to
> understand why this landed without a test...

The test was succeeding also without the patch because it was not testing that the new registration was connected with the existing one. I had to touch a web-platform test. Maybe we can start from that to write a new test. If you file a follow up I can attach a test.