Closed Bug 1319846 Opened 8 years ago Closed 8 years ago

Caching Response.redirect response results in garbled page when using the cached response.

Categories

(Core :: DOM: Service Workers, defect)

49 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: john.david.dalton, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

Attached image garbled.png
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.59 Safari/537.36 Steps to reproduce: Visiting https://lodash.com/docs/ Actual results: The page display is garbled. Expected results: The page should display as it does if you visit https://lodash.com/docs/4
I have encountered the same problem. User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
It looks like a problem with the service worker caching the redirect response. If I remove that bit of code then the page loads fine. https://github.com/lodash/lodash.com/commit/f3e2b0e4e0f7a4b3ad0b85c5da65dfc625cf9bb6
Summary: Visiting https://lodash.com/docs/ results in a garbled page → Caching Response.redirect response results in garbled page when using the cached response.
Could you keep a page with the issue as testcase, please.
Flags: needinfo?(john.david.dalton)
Component: Untriaged → DOM: Service Workers
Product: Firefox → Core
I'm looking at this.
Flags: needinfo?(john.david.dalton) → needinfo?(bkelly)
It seems the Cache API is not reproducing headers correctly. This is a copy/paste from a web console session: var redirResponse = Response.redirect('foo-bar.html', 302) undefined redirResponse Response { type: "default", url: "", redirected: false, status: 302, ok: false, statusText: "OK", headers: Headers, bodyUsed: false } redirResponse.headers.get('Location') "https://lodash.com/foo-bar.html" cache.put('foo.html', redirResponse.clone()) Promise { <state>: "pending" } cache.match('foo.html').then(response => console.log(response)) Promise { <state>: "pending" } Response { type: "default", url: "", redirected: false, status: 302, ok: false, statusText: "OK", headers: Headers, bodyUsed: false } cache.match('foo.html').then(response => console.log(response.headers.get('Location'))) Promise { <state>: "pending" } null
Assignee: nobody → bkelly
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bkelly)
This patch fixes a bug from the initial Cache API implementation. Its possible a Response to have headers set and then have the header guard locked down. Therefore we need to set headers before restoring the guard when deserializing a response from the disk.
Attachment #8815053 - Flags: review?(bugmail)
I wrote a mochitest at first because I figured this was just a stupid gecko bug.
Attachment #8815056 - Flags: review?(bugmail)
Then I decided I should probably add a wpt as well. Both of these tests fail without the P1 applied. In particular, they trigger assertions and crash in DEBUG builds. https://treeherder.mozilla.org/#/jobs?repo=try&revision=096daf35a6244912a008737dd68b5f6ed5cdcad7
Attachment #8815058 - Flags: review?(bugmail)
John, I was never able to reproduce your garbled output. I was, however, able to produce broken behavior where we just showed a blank screen. This was due to losing the `Location` header when pulling the Response out of the Cache. The patches I have here fix the problem locally for me. It still worries me, though, that I didn't see the garbled output. Perhaps something changed between FF49 to FF50 that explains the difference. Can you try with FF50 and see if you still get the garbled output? Once the try build finishes I can also provide a link for a fixed binary to try.
Flags: needinfo?(john.david.dalton)
Note, I do believe the garbled output could occur, though. I have seen that before when the page is compressed (which is here) and we try to decompress twice. I just can't reproduce it with the https://lodash.com/fx_bug_1319846/ link so far.
Comment on attachment 8815053 [details] [diff] [review] P1 Deserialize headers before restoring guard value when reading from Cache API. r=asuth Review of attachment 8815053 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cache/TypeUtils.cpp @@ +262,5 @@ > > RefPtr<InternalHeaders> internalHeaders = > ToInternalHeaders(aIn.headers(), aIn.headersGuard()); > ErrorResult result; > + ir->Headers()->Fill(*internalHeaders, result); I think this merits a comment along the lines of: // Enabling the guard will cause Fill to fail, so obviously we want to fill before possibly enabling the guard.
Attachment #8815053 - Flags: review?(bugmail) → review+
Comment on attachment 8815056 [details] [diff] [review] P2 Add a mochitest verifying storing a Response.redirect() works. r=asuth Review of attachment 8815056 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cache/test/mochitest/test_cache_redirect.html @@ +2,5 @@ > + - http://creativecommons.org/publicdomain/zero/1.0/ --> > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Validate Interfaces Exposed to Workers</title> nit: Title not updated. (Doesn't really matter, but if we're gonna have a title, it should be generic or correctly specific. There are 5 others that are equally wrong; rs=asuth if you decide to change them too.)
Attachment #8815056 - Flags: review?(bugmail) → review+
Attachment #8815058 - Flags: review?(bugmail) → review+
> John, can you try this binary and tell me if it works for you? It works
I added a comment as requested. In fact, I re-used the comment I added in bug 1217501 when I fixed this problem for Request deserialization.
Attachment #8815053 - Attachment is obsolete: true
Flags: needinfo?(john.david.dalton)
Attachment #8815080 - Flags: review+
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1e6c7e2a43 P1 Deserialize headers before restoring guard value when reading from Cache API. r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/2b7386abfe14 P2 Add a mochitest verifying storing a Response.redirect() works. r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/7e1519cd5349 P3 Add a wpt test verifying Cache API can store and reproduce Response.redirect(). r=asuth
Comment on attachment 8815080 [details] [diff] [review] P1 Deserialize headers before restoring guard value when reading from Cache API. r=asuth Approval Request Comment [Feature/Bug causing the regression]: Service worker Cache API [User impact if declined]: Certain sites will not function properly. For example, the lodash docs site was showing either garbled output or a white screen. [Is this code covered by automated tests?] I added a mochitest and wpt test in this bug to check the condition. [Has the fix been verified in Nightly? I had the reporter verify the fix with a try build binary. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: No other bugs need to be uplifted. [Is the change risky?]: Minimal risk [Why is the change risky/not risky?]: Its a very small code change that is easily tested. [String changes made/needed]: None.
Attachment #8815080 - Flags: approval-mozilla-beta?
Attachment #8815080 - Flags: approval-mozilla-aurora?
Comment on attachment 8815082 [details] [diff] [review] P2 Add a mochitest verifying storing a Response.redirect() works. r=asuth See comment 20.
Attachment #8815082 - Flags: approval-mozilla-beta?
Attachment #8815082 - Flags: approval-mozilla-aurora?
Comment on attachment 8815058 [details] [diff] [review] P3 Add a wpt test verifying Cache API can store and reproduce Response.redirect(). r=asuth See comment 20.
Attachment #8815058 - Flags: approval-mozilla-beta?
Attachment #8815058 - Flags: approval-mozilla-aurora?
This is a long standing bug and not a recent regression.
Comment on attachment 8815058 [details] [diff] [review] P3 Add a wpt test verifying Cache API can store and reproduce Response.redirect(). r=asuth Fix an issue related to service worker. Beta51+ and Aurora52+. Should be in 51 beta 6.
Attachment #8815058 - Flags: approval-mozilla-beta?
Attachment #8815058 - Flags: approval-mozilla-beta+
Attachment #8815058 - Flags: approval-mozilla-aurora?
Attachment #8815058 - Flags: approval-mozilla-aurora+
Attachment #8815080 - Flags: approval-mozilla-beta?
Attachment #8815080 - Flags: approval-mozilla-beta+
Attachment #8815080 - Flags: approval-mozilla-aurora?
Attachment #8815080 - Flags: approval-mozilla-aurora+
Attachment #8815082 - Flags: approval-mozilla-beta?
Attachment #8815082 - Flags: approval-mozilla-beta+
Attachment #8815082 - Flags: approval-mozilla-aurora?
Attachment #8815082 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: