Closed Bug 1201498 Opened 4 years ago Closed 4 years ago

Service worker update should compare scriptURL to worker URL without fragment

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- wontfix
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: bkelly, Assigned: baku)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

Blocks: ServiceWorkers-v1
No longer blocks: ServiceWorkers-postv1
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?
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
You're right!
Assignee: nobody → ehsan
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...
Flags: needinfo?(amarchesini)
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.
Assignee: ehsan → nobody
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch patchSplinter Review
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.
Attachment #8685480 - Flags: review?(bkelly)
Attachment #8685480 - Flags: review?(bkelly) → review+
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
Attachment #8685480 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(amarchesini) → needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
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!
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/92bbfb9abef9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.
Attachment #8685480 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Andrea, was the test I wrote for this completely wrong?  I'm trying to understand why this landed without a test...
Flags: needinfo?(amarchesini)
(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.
Flags: needinfo?(amarchesini)
Depends on: 1229042
You need to log in before you can comment on or make changes to this bug.