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

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?

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.

You're right!

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

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.

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.

We should uplift this to aurora.

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

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

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?

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.

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!

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

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

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