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

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jason Duell]

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.

Comment 1

[Jason Duell]

A regression range would be really useful here if someone has time for it.

Comment 2

[Ioana (away)]

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

[Viga]

Any update on this?

Comment 4

[Patrick McManus [:mcmanus]]

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.

Comment 5

[Patrick McManus [:mcmanus]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Jason Duell]

Thanks for figuring this out Patrick!  Awesome.

Comment 8

[Patrick McManus [:mcmanus]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/249913d17777

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/249913d17777

Comment 10

[Ioana (away)]

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?

Comment 11

[Jason Duell]

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.

Comment 12

[Ioana (away)]

(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 :)