Shift-Reload serves up cache entries for page subresources w/o checking server for new ones

VERIFIED FIXED in Firefox 28

Status

()

VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: jduell.mcbugs, Assigned: mcmanus)

Tracking

unspecified
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Follow the STR in bug 829616 in the browser, but hit "shift-reload" instead of reload (make sure the page resources haven't been modified very recently when you start: this bug relies on the RFC 2616 section 13.2.4 semantics, where by default we skip revalidating resources that are less than 10% older now than they were when first fetched them, i.e. if you modify the pages 1 minute before running the test, you only have 6 seconds to hit shift-reload).

Expected: all resources on the page are fetched (or at least revalidated) over the network.

Actual: Only the top-level document is fetched over the network.  iframe2 and 3 see 

   Not validating based on expiration time

in the HTTP log, and are returned from the HTTP cache w/o any network validation, so are stale compared with website.

Looking at the HttpChannel's LoadFlags, I'm seeing the following:

1) Regular reload (not shift-reload): both top-level document and sub-iframes are loaded with VALIDATE_ALWAYS:
- top-level doc: VALIDATE_ALWAYS | LOAD_DOCUMENT_URI | LOAD_INITIAL_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS | LOAD_CLASSIFY_URI
- iframe2/3:     VALIDATE_ALWAYS | LOAD_DOCUMENT_URI | LOAD_INITIAL_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS | LOAD_CLASSIFY_URI

This is slightly buggy (in that iframe2/3 shouldn't have LOAD_INITIAL_DOCUMENT_URI set IIUC).  But it results in seeing all changes, due to VALIDATE_ALWAYS.

2) Shift-reload: top-level document gets LOAD_BYPASS_CACHE | LOAD_FRESH_CONNECTION, sub-frames get no cache-related flags:

- top-level doc: LOAD_BYPASS_CACHE | LOAD_FRESH_CONNECTION | LOAD_DOCUMENT_URI | LOAD_INITIAL_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS | LOAD_CLASSIFY_URI
- iframe2/3: LOAD_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS | LOAD_CLASSIFY_URI

So we force a full reload from a fresh network connection for top-level doc, but then treat sub-resources as normal loads, and if they're recent enough we get from cache w/o revalidating.  == BUG.  This makes shift-reload actually weaker than regular reload.

Fix: we should pass either VALIDATE_ALWAYS, or better still LOAD_BYPASS_CACHE, to iframe2/3 loads. I don't think we should pass LOAD_FRESH_CONNECTION: that should only be passed to the top-level doc, else we'll wind up forcing a brand-new TCP/HTTP connection for each page resource, which seems really inefficient: see 

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp?rev=620f2cbe08be#4465

I've started tracing through the DocShell code here, but someone with more DocShell-Fu would probably make shorter work of this than I.
(Reporter)

Updated

6 years ago
Blocks: 731887
(Reporter)

Comment 1

6 years ago
A regression range would be really useful here if someone has time for it.
Keywords: qawanted

Comment 2

5 years ago
Added the testcase in bug 829616 where I could edit it: http://test.ayana503.5gbfree.com/iframe1.html.
I used Ctrl+Shift+R for shift-reload.

I reproduced this bug once on the latest Firefox 27 Nightly and once on Firefox 24.0. Then I went on to try and reproduce it on 16.0.2, but it worked fine. After that, I couldn't reproduce the bug anymore although I tried again on the same Firefox 24 and 27, and on Firefox 21, 22, 23.0.1 and 25b4. I tried new and old profiles, making different changes to the iframes, reloading with Shift + click Reload button and reloading after different periods of time (a few seconds, 15 minutes, 30 minutes, 1 hour, 1-2hrs).

Any ideas what could be happening here?

Comment 3

5 years ago
Any update on this?
(Assignee)

Comment 4

5 years ago
afaict the STR here is valid, but the summary is overstated.

This doesn't apply to all subresources - specifically it doesn't apply to js/css and images. Here is a counter example - http://www.mozilla.org/en-US/ 

reload that page and all the subresources are validated with 304's.. shift reload that page and all the subresources are fetched fresh with 200's. That's a relief!

I suspect the STR points to something iframe specific.
(Assignee)

Comment 5

5 years ago
Created attachment 832495 [details] [diff] [review]
iframes are not reloaded on shift reload

I'm not confident this is the right fix; I'm always dangerous in the
docshell. In particular I'm concerned about the comment a few lines
above that says "By default the subframe will inherit the parent's
loadType." I can't find anywhere that actually happens - I think if it
did this patch wouldn't be needed.. but I wasn't sure of the
ramifications of just doing it that way, so I made it conditional on
the shift reload.

It does straighten out the above test case and normal reload behavior
continues to look ok. (try pending)
Attachment #832495 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Comment on attachment 832495 [details] [diff] [review]
iframes are not reloaded on shift reload

I guess not putting this in the existing "loadType = parentLoadType" block is ok.

This setup is so broken...  Once you shift-reload, all navigation (e.g. via clicks!) in subframes of that page will bypass cache.  I wish we could scope it somehow, but I'm not sure how.

r=me
Attachment #832495 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 7

5 years ago
Thanks for figuring this out Patrick!  Awesome.

Updated

5 years ago
Keywords: qawanted
https://hg.mozilla.org/mozilla-central/rev/249913d17777
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

5 years ago
Keywords: qawanted
status-firefox28: --- → fixed
Keywords: verifyme

Comment 10

5 years ago
As specified in comment 2, I only reproduce this issue twice.

I tried again today with a Firefox 27 build and changed iframe details a few times, but the bug is still not reproducible.

Jason, since you reproduced this issue before, can you please try again on a Firefox 28 build and let us know if it's fixed for you?
Flags: needinfo?(jduell.mcbugs)
Keywords: verifyme
(Reporter)

Comment 11

5 years ago
I just tested with a firefox 27 build and I saw the bug (Ioana: you probably modified the page too recently before you started the test, as comment 0 mentions).

I also tried on aurora (ff 29) and the bug is fixed there.  I don't have a copy of FF 28 handy but I'm confident that this was the fix that changed the behavior.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jduell.mcbugs)

Comment 12

5 years ago
(In reply to Jason Duell (:jduell) from comment #11)
> I just tested with a firefox 27 build and I saw the bug (Ioana: you probably
> modified the page too recently before you started the test, as comment 0
> mentions).

Just as in comment 2, I tried different intervals, from several seconds to an hour. It worked the same for me.

Anyway, thanks for verifying this :)
status-firefox28: fixed → verified
You need to log in before you can comment on or make changes to this bug.