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 |
You need to log in
before you can comment on or make changes to this bug.
Description
•