1157619 - Encoding problem forcing a reload from a ServiceWorker controlled page in non-e10s windows

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Andrea Marchesini [:baku]]

STR:

1. Open https://googlechrome.github.io/samples/service-worker/read-through-caching/index.html - this will register a SW
2. Refresh the page and the content will be loaded from the SW.
3. You can double check the SW in about:serviceworkers
4. Do a force reload of the https://googlechrome.github.io/samples/service-worker/read-through-caching/index.html tab (on linux I do CTRL+SHIFT+R)
5. a block of random data is shown: "��YsӸ���B��M�����BҙR�..."

Comment 1

[Andrea Marchesini [:baku]]

Doing a normal refresh shows the page correctly. But all the time I do a forced-reload, I see the random data.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I suspect we are somehow losing an encoding http header when we pull the Response out of Cache and hand it to FetchEvent::RespondWith().  It seems like the event propagates them correctly, though:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#125

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #2)
> I suspect we are somehow losing an encoding http header when we pull the
> Response out of Cache and hand it to FetchEvent::RespondWith().  It seems
> like the event propagates them correctly, though:
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/workers/
> ServiceWorkerEvents.cpp#125

That does not seem to be the problem.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

When we get the garbage in the window we have a full ssl lock icon.  When the page displays normally we get a mixed content warning icon.  Is this related to synthesizing encrypted resources?

Comment 5

[Andrea Marchesini [:baku]]

bkelly and I debugged this issue a bit and we saw some network/ssl issues:

1. with a normal SW loading we show the icon of ssl warning icon in the URL bar. Without SW we show the lock icon.

2. with a SW loading we just receive the index.html. Normal loading: 5 requests, 2 images, 1 index.html and something else.

3. With a force-reloading we have unreadable data. Maybe a double gunzip? Note: all the http entries are correctly set in the ServiceWorkerEvent.

NI jdm for a feedback.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Nikhil, is this a case of getting a filtered or opaque Response from fetch(), but maybe something different from Cache?

Comment 7

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

I'll wait for josh. filtered/opaque response should not matter for same domain stuff and the SW script isn't doing anything special.

Comment 8

[Josh Matthews [:jdm]]

1 & 2 might be handled by bug 1156847.

Comment 9

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #8)
> 1 & 2 might be handled by bug 1156847.

I think they are, but the rest of this bug remains unsolved.  Note that this only happens in non-e10s windows.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

I'm still seeing this one.  I think we need to fix this before v1.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Let me see if I can figure this one out.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Is it a bug that we intercept the top level document nsIChannel when doing the shift+reload?  It seems we only skip interception for the dependent resources on the page.

Comment 13

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

I am seeing a number of issues, none of which are the original issue in the description.

1) I visit the website, the page is loaded from the network as expected.
2) Reload the page (not force reload!). At this point the spinner keeps spinning, while the console has logs about the SW trying to fetch from network since there are no cached resources.
3) Step 2 does not finish in a reasonable time.
4) Reload the page again (again not a force reload), interrupting the earlier load. Now it loads fine and you can see console logs that stuff was retrieved from the cache. BUT, the images are broken (shows the gecko broken image icon), presumably because the caching was stopped halfway or because of encoding issues or similar (have to dig into this)
5) If I open the same URL in a new tab, for some reason it is not controlled until I reload that tab. I would expect that since an active worker is available, the first load is also controlled.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Network requests should not be intercepted when force-refreshed. r=jdm

This patch indirectly solves the problem by implementing step 12.1 from the HandleFetch algorithm here:

  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on-fetch-request-algorithm

Basically, we should never intercept a network request when the page was force-refreshed.  We are currently doing so for the top level navigation.

I think we must then be missing some other piece of logic for non-e10s that fires when the SW is normally enabled (non-force-refresh) that is skipped.

With this patch the page loads correctly from network without any encoding problems.

Does anyone know if there is a way to force a shift-reload in a mochitest?  I could write a test.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #13)
> I am seeing a number of issues, none of which are the original issue in the
> description.
> 
> 1) I visit the website, the page is loaded from the network as expected.
> 2) Reload the page (not force reload!). At this point the spinner keeps
> spinning, while the console has logs about the SW trying to fetch from
> network since there are no cached resources.
> 3) Step 2 does not finish in a reasonable time.
> 4) Reload the page again (again not a force reload), interrupting the
> earlier load. Now it loads fine and you can see console logs that stuff was
> retrieved from the cache. BUT, the images are broken (shows the gecko broken
> image icon), presumably because the caching was stopped halfway or because
> of encoding issues or similar (have to dig into this)
> 5) If I open the same URL in a new tab, for some reason it is not controlled
> until I reload that tab. I would expect that since an active worker is
> available, the first load is also controlled.

If you apply the patch and then do a force-refresh, many of these symptoms go away.  Its unclear to me what is going wrong on the first load, though.

Can you open a new bug for this?

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Actually, I went ahead and cloned to bug 1167808.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Network requests should not be intercepted when force-refreshed. r=jdm

Actually, here is a better patch.  This uses the nsDocShell definition of shift-reload instead of trying to infer it from the LOAD_BYPASS_CACHE flag.

Relevant spec text is step 12.1 here:

  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on-fetch-request-algorithm

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

By the way, I would be happy to fix the issue with nsDocument using LOAD_BYPASS_CACHE to infer shift-reload, but its unclear the best way to get at that information at that point.  Any suggestions welcome.  Thanks!

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8609701 [details] [diff] [review]
Network requests should not be intercepted when force-refreshed. r=jdm

I wish we had some helper method to detect what might be shift-reload, but we seem to use this variant for new URIs.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

I think I can write a test using window.location.reload(true) to simulate the shift-reload.

Comment 21

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #20)
> I think I can write a test using window.location.reload(true) to simulate
> the shift-reload.

Another way to test this would be writing a browser-chrome test, and there using synthesizeKey to simulate Cmd+Shift+R...

Comment 22

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #18)
> By the way, I would be happy to fix the issue with nsDocument using
> LOAD_BYPASS_CACHE to infer shift-reload, but its unclear the best way to get
> at that information at that point.  Any suggestions welcome.  Thanks!

Can we add another load flag explicitly set by the docshell?

Comment 23

[Josh Matthews [:jdm]]

Comment on attachment 8609701 [details] [diff] [review]
Network requests should not be intercepted when force-refreshed. r=jdm

Review of attachment 8609701 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test that service worker is not intercepted on force refresh. r=ehsan

Here is a force-refresh test that runs in an iframe and uses window.location.reload(true).  Unfortunately it does not trigger the encoding problem we are trying to test.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test that service worker is not intercepted on force refresh. r=ehsan

That was the wrong patch.  This is the iframe test.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

It doesn't reproduce in the browser chrome test either.

I'm starting to think I need to be loading a gzip'd page.  Investigating how to do that in mochitest.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test that service worker is not intercepted on force refresh. r=ehsan

Ok, adding the gzip encoding to the files causes the bug to reproduce with the iframe test.  I'm going to go with that one for now.

I'll write a follow-up to add a chrome refresh test.  (I triggered some asserts while working on it, so it would probably be good to have it.)

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Network requests should not be intercepted when force-refreshed. r=smaug

Fix the commit message to reflect r=smaug.  I'll probably have to commit this with checkin-needed.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d79f78f5767

Comment 30

[(no longer active)]

Comment on attachment 8613115 [details] [diff] [review]
P2 Test that service worker is not intercepted on force refresh. r=ehsan

Review of attachment 8613115 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/serviceworkers/test_force_refresh.html
@@ +74,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +  SpecialPowers.pushPrefEnv({"set": [
> +    ["dom.serviceWorkers.exemptFromPerDomainMax", true],
> +    ["dom.serviceWorkers.enabled", true],
> +    ["dom.serviceWorkers.testing.enabled", true]

You need to set dom.caches.enabled here as well.

Comment 31

[(no longer active)]

I'll land the patches for Ben as he is on vacation.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f081db03c464
https://hg.mozilla.org/integration/mozilla-inbound/rev/40f5f8f3e1fb

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f081db03c464
https://hg.mozilla.org/mozilla-central/rev/40f5f8f3e1fb