Closed Bug 1172884 Opened 8 years ago Closed 8 years ago

Synthesized permanent redirected response crashes browser

Categories

(Core :: DOM: Service Workers, defect)

Product:
Core
Component:
DOM: Service Workers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: azasypkin, Assigned: bkelly)

References

(
URL
)

Details

Attachments

(3 files)

garbled response
10.07 KB, text/plain
Details
P1 Properly decode body when intercepted response redirects. r=mayhemer
4.34 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
P2 Add test for synthesizing a redirect to a compressed resource. r=ehsan
5.95 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
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 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))
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
Status: NEW → ASSIGNED
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 @@
> +<!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+
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 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.
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
Closed: 8 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)
(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.