Synthesized permanent redirected response crashes browser

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: azasypkin, Assigned: bkelly)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

(URL)

Attachments

(3 attachments)

Created attachment 8617262 [details]
garbled response

If in Service Worker's fetch event handler we respond with permanent redirect like this:

e.respondWith(new Response('', {
  status: 301,
  statusText: 'Moved Permanently',
  headers: {
    'Location': '/sw-tests/permanent-redirect/redirected-index.html'
  }
}));

browser will display some garbled content (see attachment) or even _crash_ if test case run on the local host (Nightly 41.0a1 (2015-06-08)).

See test case at https://azasypkin.github.io/sw-tests/permanent-redirect/.

Both github.io and localhost examples work well on Chrome (Version 45.0.2421.0 dev (64-bit))
(Assignee)

Updated

3 years ago
Blocks: 1059784
(Assignee)

Comment 1

3 years ago
I don't think the gzip encoding is being properly decoded in this case for some reason.  I'll take a look.
(Assignee)

Updated

3 years ago
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8617403 [details] [diff] [review]
P1 Properly decode body when intercepted response redirects. r=mayhemer

This fixes the garbled response issue by ensuring the encoding is converted when redirecting.  I will add a test in a second patch.

Note, the actual redirect is only working in non-e10s, however.  In e10s we get a 404 page.  I'll write a separate bug for this.
Attachment #8617403 - Flags: review?(honzab.moz)
(Assignee)

Updated

3 years ago
Blocks: 1172992
(Assignee)

Comment 3

3 years ago
So I have a test case that navigates an iframe to a fake URL and then the SW redirects to a compressed page.  The failure doesn't reproduce in the test, though.
(Assignee)

Comment 4

3 years ago
Created attachment 8617592 [details] [diff] [review]
P2 Add test for synthesizing a redirect to a compressed resource. r=ehsan

This patch successfully provokes the garbled text problem on redirect to a compressed resource.  The trick was to not use respondWith(fetch(event.request)) for the fallback case, but instead just let the default action take place.

Note, however, this test does *not* provoke the failure to redirect in e10s in bug 1172992.  I don't understand why yet.
Attachment #8617592 - Flags: review?(ehsan)

Comment 5

3 years ago
Comment on attachment 8617592 [details] [diff] [review]
P2 Add test for synthesizing a redirect to a compressed resource. r=ehsan

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

::: dom/workers/test/serviceworkers/sw_clients/navigator.html
@@ +5,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Bug 982726 - test match_all not crashing</title>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>

Nit: for sanity's sake, please remove this, and change the info() further down to dump().
Attachment #8617592 - Flags: review?(ehsan) → review+
Comment on attachment 8617403 [details] [diff] [review]
P1 Properly decode body when intercepted response redirects. r=mayhemer

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

I don't know this code well, but this smells right to me, and we have a test.  So in the absence of Honza I'm going to +r this.
Attachment #8617403 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 9

3 years ago
Sorry, I forgot to update the r= in the P1 commit message.  It was really jduell.  Thank you for the review!
(Assignee)

Comment 10

3 years ago
Actually, I pushed the wrong patches completely.  Without ehsan's review feedback addressed.  I asked for a back out.
Backed out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/91a848ffec69 because one bug or the other caused the same https://treeherder.mozilla.org/logviewer.html#?job_id=10697453&repo=mozilla-inbound timeouts on OS X as it did the first time they landed.
(Assignee)

Comment 14

3 years ago
The failing test is in the other bug.  So re-landing this one.
https://hg.mozilla.org/mozilla-central/rev/3747f24b5a60
https://hg.mozilla.org/mozilla-central/rev/97a88dbb4206
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Yay, now it works in Nightly, thanks for the fix!

But I see that it still doesn't work in b2g browser (hosted app via https). I see 404 like in bug 1172992. 

Is it just another reincarnation of bug 1172992 or they're unrelated?
Flags: needinfo?(bkelly)
(Assignee)

Comment 18

3 years ago
(In reply to Oleg Zasypkin [:azasypkin] from comment #17)
> But I see that it still doesn't work in b2g browser (hosted app via https).
> I see 404 like in bug 1172992. 
> 
> Is it just another reincarnation of bug 1172992 or they're unrelated?

Correct.  b2g runs in e10s, so it should see bug 1172992.  We haven't had time to investigate that yet.
Flags: needinfo?(bkelly)
You need to log in before you can comment on or make changes to this bug.