[e10s] The document is not loading from application cache if it was returned as a redirect location to the previous request from server

RESOLVED FIXED in Firefox 50

Status

()

Core
Networking: Cache
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Dave Meyers, Assigned: mayhemer, NeedInfo)

Tracking

49 Branch
mozilla52
Points:
---

Firefox Tracking Flags

(platform-rel +, firefox49 wontfix, firefox50+ fixed, firefox51 fixed, firefox52 fixed)

Details

(Whiteboard: [platform-rel-Microsoft][platform-rel-Outlook][necko-active])

Attachments

(2 attachments)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36

Steps to reproduce:

simplified repro outside of our app:

1. Open https://appcacherepro.azurewebsites.net/demo.aspx in Firefox 
2. This page will install appcache and cache itself
3. Hit F5 and note the page generation time
4. Open  https://appcacherepro.azurewebsites.net/demo.aspx?authRedirect=true, it will redirect to https://appcacherepro.azurewebsites.net/demo.aspx 
5. Step 4's final request https://appcacherepro.azurewebsites.net/demo.aspx does not load from appcache. The page generation time will not match to that of step 3.


Actual results:

Example from production:

1. Navigating: https://outlook.office.com/owa/?authRedirect=true => returns 302 Redirect to https://outlook.office.com/owa/#authRedirect=true 
2. https://outlook.office.com/owa/#authRedirect=true should load from appcache as the url is cached but it skips appcache and a request is made to server.
3. If you hit F5 again with url https://outlook.office.com/owa/#authRedirect=true the document successfully loads from appcache.
Component: Untriaged → Networking
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(honzab.moz)
platform-rel: --- → +
Whiteboard: [platform-rel-Microsoft][platform-rel-Outlook]
[Tracking Requested - why for this release]:
Important performance regression, we should investigate before 50 goes live.
status-firefox49: --- → wontfix
status-firefox50: --- → affected
tracking-firefox50: --- → ?
(Assignee)

Comment 2

a year ago
Appcache is about to be removed, we only fix critical bugs or serious regressions.

This is probably not a bug, seems like we purposely don't look into appcache for results of redirects:

https://dxr.mozilla.org/mozilla-central/rev/c845bfd0accb7e0c29b41713255963b08006e701/netwerk/protocol/http/HttpBaseChannel.cpp#3125

  // transfer application cache information
  nsCOMPtr<nsIApplicationCacheChannel> appCacheChannel =
    do_QueryInterface(newChannel);
  if (appCacheChannel) {
    appCacheChannel->SetApplicationCache(mApplicationCache);
    appCacheChannel->SetInheritApplicationCache(mInheritApplicationCache);
>   // We purposely avoid transfering mChooseApplicationCache.
  }

mChooseApplicationCache == true makes us to look the appcache up for the URL being loaded.  In the redirect target channel it tho remains false.

Unfortunately, the one who wrote that comment didn't say "why"...

If other browsers treat this differently, let's reopen this bug and carry the flag.  The fix should be easy, but I'm not sure if we have tests for this and can't estimate any regressions this could cause, a path I don't want to go...  automated tests are now disabled for appcache (e10s, afaik).
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(honzab.moz)
Resolution: --- → INVALID
can you give details on why this is listed as a regression in comment 1? Does 48 perform differently or has the content changed? The code honza cites didn't change in the last several years.
Flags: needinfo?(sledru)

Comment 4

a year ago
It's an issue only with e10s.
I tested with FF49/52 and e10s turned off, I can't reproduce the issue. Same with the old FF40, it's reproducible only with e10s enabled.

As automated tests for appcache are disabled with e10s, this issue has been probably not caught.
Resolution: INVALID → FIXED
Summary: In Firefox 49, the document is not loading from application cache if it was returned as a redirect location to the previous request from server → [e10s] The document is not loading from application cache if it was returned as a redirect location to the previous request from server

Updated

a year ago
Resolution: FIXED → INVALID
Flags: needinfo?(jduell.mcbugs)
> It's an issue only with e10s.

Then this at some level is a regression--sounds like our e10s code is busted.

I hate to do this to you Honza...
Assignee: nobody → honzab.moz
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
(Assignee)

Comment 6

a year ago
I agree Jason, something to fix.  Will take a look.
Status: REOPENED → NEW
Component: Networking → Networking: Cache
Flags: needinfo?(sledru)
Whiteboard: [platform-rel-Microsoft][platform-rel-Outlook] → [platform-rel-Microsoft][platform-rel-Outlook][necko-active]
(Assignee)

Comment 7

a year ago
So, the reason why this works in non-e10s and not in e10s is that mChooseApplicationCache member on nsHttpChannel is set on redirect by docshell.  In e10s this happens on the child process.  However, we don't carry the flag back to the parent.  The fix is easy.
Status: NEW → ASSIGNED
(Assignee)

Comment 8

a year ago
Created attachment 8805087 [details] [diff] [review]
v1 (carry the choose app cache flag after redirect back to parent)

Straightforward.  I won't push to try because there are no tests for this anyway.
Attachment #8805087 - Flags: review?(jduell.mcbugs)
Comment on attachment 8805087 [details] [diff] [review]
v1 (carry the choose app cache flag after redirect back to parent)

Review of attachment 8805087 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome--thanks Honza.
Attachment #8805087 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 10

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d65a9494010e
Carry ChooseApplicationCache flag from child after redirect callback is done. r=jduell
Keywords: checkin-needed

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d65a9494010e
Status: ASSIGNED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8805087 [details] [diff] [review]
v1 (carry the choose app cache flag after redirect back to parent)

Review of attachment 8805087 [details] [diff] [review]:
-----------------------------------------------------------------

This is breaking Microsoft Office online, and it's a simple, risk-free fix (just schlepping a boolean from the child to the parent).  Please uplift as far up as possible.
Attachment #8805087 - Flags: approval-mozilla-release?
Attachment #8805087 - Flags: approval-mozilla-beta?
Attachment #8805087 - Flags: approval-mozilla-aurora?
status-firefox51: --- → affected
Hi Dave, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(davem)
Comment on attachment 8805087 [details] [diff] [review]
v1 (carry the choose app cache flag after redirect back to parent)

Fixes a regression in MS Office online, Aurora51+, Beta50+
Attachment #8805087 - Flags: approval-mozilla-beta?
Attachment #8805087 - Flags: approval-mozilla-beta+
Attachment #8805087 - Flags: approval-mozilla-aurora?
Attachment #8805087 - Flags: approval-mozilla-aurora+

Updated

a year ago
tracking-firefox50: ? → +
This is hitting conflicts on the uplift to beta. Could we get a rebased patch if this needs uplifting?
Flags: needinfo?(honzab.moz)

Comment 16

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/273eaffac141
status-firefox51: affected → fixed
(Assignee)

Comment 17

a year ago
Created attachment 8806475 [details] [diff] [review]
v1 for m-b

This is it, builds locally for me.
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 18

a year ago
Comment on attachment 8806475 [details] [diff] [review]
v1 for m-b

comment 12
Attachment #8806475 - Flags: approval-mozilla-beta?

Updated

a year ago
Attachment #8806475 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-release/rev/b0a45e5d44e48c88545f8abd1f7fff7a21c88f7e
status-firefox50: affected → fixed

Updated

a year ago
Attachment #8805087 - Flags: approval-mozilla-release?
You need to log in before you can comment on or make changes to this bug.