Closed
Bug 831153
Opened 11 years ago
Closed 11 years ago
Shift-Reload serves up cache entries for page subresources w/o checking server for new ones
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
VERIFIED
FIXED
mozilla28
Tracking | Status | |
---|---|---|
firefox28 | --- | verified |
People
(Reporter: jduell.mcbugs, Assigned: mcmanus)
References
Details
Attachments
(1 file)
1.93 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•11 years ago
|
||
A regression range would be really useful here if someone has time for it.
Keywords: qawanted
Comment 2•11 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?
Assignee | ||
Comment 4•11 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•11 years ago
|
||
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•11 years ago
|
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Comment 6•11 years ago
|
||
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•11 years ago
|
||
Thanks for figuring this out Patrick! Awesome.
Assignee | ||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/249913d17777
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/249913d17777
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
status-firefox28:
--- → fixed
Keywords: verifyme
Comment 10•10 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•10 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•10 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 :)
Comment hidden (spam) |
Comment hidden (spam) |
You need to log in
before you can comment on or make changes to this bug.
Description
•