Closed Bug 1312646 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Networking: Cache, defect)

49 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
platform-rel --- +
firefox49 --- wontfix
firefox50 + fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: davem, Assigned: mayhemer, NeedInfo)

References

Details

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

Attachments

(2 files)

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.
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
Closed: 8 years 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)
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
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 → ---
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]
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
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/d65a9494010e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
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?
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+
This is hitting conflicts on the uplift to beta. Could we get a rebased patch if this needs uplifting?
Flags: needinfo?(honzab.moz)
Attached patch v1 for m-bSplinter Review
This is it, builds locally for me.
Flags: needinfo?(honzab.moz)
Comment on attachment 8806475 [details] [diff] [review]
v1 for m-b

comment 12
Attachment #8806475 - Flags: approval-mozilla-beta?
Attachment #8806475 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Regressions: 1545909
You need to log in before you can comment on or make changes to this bug.