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

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: baku, Assigned: bkelly)

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox40 affected, firefox41 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Reporter

Description

4 years ago
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�..."
Reporter

Comment 1

4 years ago
Doing a normal refresh shows the page correctly. But all the time I do a forced-reload, I see the random data.
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
(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.
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?
Reporter

Comment 5

4 years ago
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.
Flags: needinfo?(josh)
Nikhil, is this a case of getting a filtered or opaque Response from fetch(), but maybe something different from Cache?
Flags: needinfo?(nsm.nikhil)
I'll wait for josh. filtered/opaque response should not matter for same domain stuff and the SW script isn't doing anything special.
Flags: needinfo?(nsm.nikhil)
1 & 2 might be handled by bug 1156847.
Flags: needinfo?(josh)
See Also: → 1156847

Comment 9

4 years ago
(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.
Summary: Encoding problem forcing a reload from a ServiceWorker controlled page. → Encoding problem forcing a reload from a ServiceWorker controlled page in non-e10s windows
I'm still seeing this one.  I think we need to fix this before v1.
Let me see if I can figure this one out.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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.
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.
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.
Attachment #8609682 - Flags: review?(josh)
(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?
Blocks: 1167808
Actually, I went ahead and cloned to bug 1167808.
Component: DOM → DOM: Service Workers
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
Attachment #8609682 - Attachment is obsolete: true
Attachment #8609682 - Flags: review?(josh)
Attachment #8609701 - Flags: review?(bugs)
Attachment #8609701 - Flags: feedback?(josh)
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 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.
Attachment #8609701 - Flags: review?(bugs) → review+
I think I can write a test using window.location.reload(true) to simulate the shift-reload.

Comment 21

4 years ago
(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

4 years ago
(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 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.
Attachment #8609701 - Flags: feedback?(josh) → feedback+
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.
Attachment #8612473 - Attachment is obsolete: true
That was the wrong patch.  This is the iframe test.
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.
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.)
Attachment #8612475 - Attachment is obsolete: true
Attachment #8613115 - Flags: review?(ehsan)
Fix the commit message to reflect r=smaug.  I'll probably have to commit this with checkin-needed.
Attachment #8609701 - Attachment is obsolete: true
Attachment #8613117 - Flags: review+
Blocks: 1169819

Comment 30

4 years ago
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.
Attachment #8613115 - Flags: review?(ehsan) → review+

Comment 31

4 years ago
I'll land the patches for Ben as he is on vacation.
https://hg.mozilla.org/mozilla-central/rev/f081db03c464
https://hg.mozilla.org/mozilla-central/rev/40f5f8f3e1fb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.