1251229 - request.url includes the hash of the loaded URL

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Valentin Gosu [:valentin] (he/him)]

Load https://awesome-sw.github.io/hello-world/#/2 (repeatedly to make sure the resources are put into the cache).

In the console you'll see that the intercepted URL includes the hash.
Moreover, in the devtools Cache storage, multiple entries for the same URL with different hashes are available.

There should only be one entry for each URL, and it should not have a hash set.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Pretty sure we try to implement fragment stripping.

Ehsan, any thoughts?  I think you were just looking at this code.

Does our parser just get confused if there is a slash inside of the fragment?

Valentin, does this work if you use URLs like "hello-world/#2"?

Comment 2

[Valentin Gosu [:valentin] (he/him)]

(In reply to Ben Kelly [PTO, back Feb 29][:bkelly] from comment #1)
> Pretty sure we try to implement fragment stripping.
> 
> Ehsan, any thoughts?  I think you were just looking at this code.
> 
> Does our parser just get confused if there is a slash inside of the fragment?
> Valentin, does this work if you use URLs like "hello-world/#2"?

It doesn't seem to be the slash. It still caches hello-world/#2

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

This might be fallout from bug 1201664.  We used to run the Request constructor which stripped the fragment, but are no longer doing that.  Maybe we miss stripping the fragment now.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Yea, this does not reproduce in FF44.  Bug 1201664 was landed in FF46.  I think that's probably the culprit.  We should fix before it hits release and write a test for this.

Ehsan, can you take?  Or I can do it next week.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Actually, I may have time for this today while the movers load the truck.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Strip fragment from request URL when creating FetchEvent. r=ehsan

This fixes the problem on the comment 0 site for me.  I'll write a mochitest next.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add wpt test verifying FetchEvent.request.url does not include fragments. r=ehsan

This test fails without the P1 patch.  It succeeds with the P1 patch.

Ehsan, do you think I need to worry about a Cache migration?  I could delete request entries with a fragment.  Alternatively we just tell people on dev edition and nightly to wipe any Cache objects affected by this.  (They will trip on the MOZ_ASSERT in debug builds.)

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=075daebfb038

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Regarding the possibility of writing a migration, the only observable case will be if someone does cache.keys().  Those resources will not be found by match() any more because the URL will be different.  So if script or devtools does a .keys() the urls-with-fragments will show up.  In debug builds they will trigger the MOZ_ASSERT.

I'm inclined not to write a migration for this.

Comment 10

[(no longer active)]

(In reply to Ben Kelly [PTO, back Feb 29][:bkelly] from comment #7)
> Created attachment 8724087 [details] [diff] [review]
> P2 Add wpt test verifying FetchEvent.request.url does not include fragments.
> r=ehsan
> 
> This test fails without the P1 patch.  It succeeds with the P1 patch.
> 
> Ehsan, do you think I need to worry about a Cache migration?  I could delete
> request entries with a fragment.  Alternatively we just tell people on dev
> edition and nightly to wipe any Cache objects affected by this.  (They will
> trip on the MOZ_ASSERT in debug builds.)

I think it's better to do a migration anyway.  It should be fairly straightforward.

Comment 11

[(no longer active)]

(Assuming that you're not planning to backport this to 46, of course.  If you do, then I'm fine without having a migration.)

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

I am going to uplift to 46.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/982cf5b26ae1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6df13193dc8

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8723585 [details] [diff] [review]
P1 Strip fragment from request URL when creating FetchEvent. r=ehsan

Approval Request Comment
[Feature/regressing bug #]: Service worker regression caused by bug 1201664.
[User impact if declined]: Sites can end up with multiple copies of the same resource in Cache and in could fail to work offline in some cases.
[Describe test coverage new/current, TreeHerder]:  Tests included.
[Risks and why]: Minimal.  Only affects service workers.
[String/UUID change made/needed]: None

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8724087 [details] [diff] [review]
P2 Add wpt test verifying FetchEvent.request.url does not include fragments. r=ehsan

See comment 14.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/982cf5b26ae1
https://hg.mozilla.org/mozilla-central/rev/d6df13193dc8

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a regression from 46.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8723585 [details] [diff] [review]
P1 Strip fragment from request URL when creating FetchEvent. r=ehsan

Fix for SW regression, includes tests. Please uplift to aurora.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8724087 [details] [diff] [review]
P2 Add wpt test verifying FetchEvent.request.url does not include fragments. r=ehsan

Test only changes, fine to uplift to aurora

Comment 20

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/10e93ba2abad
https://hg.mozilla.org/releases/mozilla-aurora/rev/5f6dd8def16f