Closed Bug 1172884 Opened 8 years ago Closed 8 years ago

Synthesized permanent redirected response crashes browser


(Core :: DOM: Service Workers, defect)

Not set



Tracking Status
firefox41 --- fixed


(Reporter: azasypkin, Assigned: bkelly)





(3 files)

Attached file 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

Both and localhost examples work well on Chrome (Version 45.0.2421.0 dev (64-bit))
I don't think the gzip encoding is being properly decoded in this case for some reason.  I'll take a look.
Assignee: nobody → bkelly
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)
Blocks: 1172992
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.
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 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 @@
> +<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+
Sorry, I forgot to update the r= in the P1 commit message.  It was really jduell.  Thank you for the review!
Actually, I pushed the wrong patches completely.  Without ehsan's review feedback addressed.  I asked for a back out.
Backed out again in because one bug or the other caused the same timeouts on OS X as it did the first time they landed.
The failing test is in the other bug.  So re-landing this one.
Closed: 8 years ago
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)
(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.