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

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

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

Tracking

(Blocks 1 bug)

49 Branch
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox50 wontfix, firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Posted 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

Comment 1

2 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

2 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

2 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.

Comment 3

2 years ago
Could you keep a page with the issue as testcase, please.
Flags: needinfo?(john.david.dalton)

Updated

2 years ago
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+
(Reporter)

Comment 16

2 years ago
> 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+

Comment 19

2 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
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 24

2 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
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.