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)
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: john.david.dalton, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 2 obsolete files)
222.02 KB,
image/png
|
Details | |
1.78 KB,
patch
|
asuth
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.43 KB,
patch
|
bkelly
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.72 KB,
patch
|
bkelly
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Comment 1•8 years ago
|
||
I have encountered the same problem. User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Reporter | ||
Comment 2•8 years ago
|
||
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
Reporter | ||
Updated•8 years ago
|
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)
Reporter | ||
Comment 4•8 years ago
|
||
@Loic I added a repro: https://github.com/lodash/lodash.com/commit/80e732d8b6bbf5b95c6dcb026fabce74bdeb66f4 Try hitting this url a few times https://lodash.com/fx_bug_1319846/
Assignee | ||
Comment 5•8 years ago
|
||
I'm looking at this.
Flags: needinfo?(john.david.dalton) → needinfo?(bkelly)
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
I wrote a mochitest at first because I figured this was just a stupid gecko bug.
Attachment #8815056 -
Flags: review?(bugmail)
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox-esr45:
--- → affected
Assignee | ||
Comment 11•8 years ago
|
||
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.
Reporter | ||
Comment 12•8 years ago
|
||
With Firefox 50 it's a blank screen. Here are other reports of garbled display: https://twitter.com/haoliang_yu/status/801468231856848896 https://twitter.com/kJamesy/status/799273549139615744 https://gitter.im/lodash/lodash?at=582d7a1fdf5ae966456b3c55
Assignee | ||
Comment 13•8 years ago
|
||
John, can you try this binary and tell me if it works for you? https://queue.taskcluster.net/v1/task/B1Lv0uxDSeeGc3olZQgtIQ/runs/0/artifacts/public/build/target.dmg
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8815058 -
Flags: review?(bugmail) → review+
Reporter | ||
Comment 16•8 years ago
|
||
> John, can you try this binary and tell me if it works for you?
It works
Assignee | ||
Comment 17•8 years ago
|
||
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+
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8815056 -
Attachment is obsolete: true
Attachment #8815082 -
Flags: review+
Comment 19•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Blocks: 1217501, ServiceWorkers-compat
Assignee | ||
Comment 20•8 years ago
|
||
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?
Assignee | ||
Comment 21•8 years ago
|
||
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?
Assignee | ||
Comment 22•8 years ago
|
||
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?
Assignee | ||
Comment 23•8 years ago
|
||
This is a long standing bug and not a recent regression.
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ac1e6c7e2a43 https://hg.mozilla.org/mozilla-central/rev/2b7386abfe14 https://hg.mozilla.org/mozilla-central/rev/7e1519cd5349
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 25•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8815080 -
Flags: approval-mozilla-beta?
Attachment #8815080 -
Flags: approval-mozilla-beta+
Attachment #8815080 -
Flags: approval-mozilla-aurora?
Attachment #8815080 -
Flags: approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8815082 -
Flags: approval-mozilla-beta?
Attachment #8815082 -
Flags: approval-mozilla-beta+
Attachment #8815082 -
Flags: approval-mozilla-aurora?
Attachment #8815082 -
Flags: approval-mozilla-aurora+
Comment 26•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8ad14b973ac https://hg.mozilla.org/releases/mozilla-aurora/rev/d85bdb39cf3f https://hg.mozilla.org/releases/mozilla-aurora/rev/96503957841c
Flags: in-testsuite+
Comment 27•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/4847bb920028 https://hg.mozilla.org/releases/mozilla-beta/rev/e261a8b9adaa https://hg.mozilla.org/releases/mozilla-beta/rev/34c73c520f93
You need to log in
before you can comment on or make changes to this bug.
Description
•