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

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[John-David Dalton]

Attachment: 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

[Haoliang Yu]

I have encountered the same problem. 

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0

Comment 2

[John-David Dalton]

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

Comment 3

[Loic]

Could you keep a page with the issue as testcase, please.

Comment 4

[John-David Dalton]

@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/

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I'm looking at this.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Deserialize headers before restoring guard value when reading from Cache API. r=asuth

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a mochitest verifying storing a Response.redirect() works. r=asuth

I wrote a mochitest at first because I figured this was just a stupid gecko bug.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a wpt test verifying Cache API can store and reproduce Response.redirect(). r=asuth

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

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

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 12

[John-David Dalton]

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

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 16

[John-David Dalton]

> John, can you try this binary and tell me if it works for you?

It works 

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Deserialize headers before restoring guard value when reading from Cache API. r=asuth

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.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a mochitest verifying storing a Response.redirect() works. r=asuth

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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 20

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8815082 [details] [diff] [review]
P2 Add a mochitest verifying storing a Response.redirect() works. r=asuth

See comment 20.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

This is a long standing bug and not a recent regression.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac1e6c7e2a43
https://hg.mozilla.org/mozilla-central/rev/2b7386abfe14
https://hg.mozilla.org/mozilla-central/rev/7e1519cd5349

Comment 25

[Gerry Chang [:gchang]]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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