Last Comment Bug 752675 - Fix misuses of LOAD_FROM_CACHE
: Fix misuses of LOAD_FROM_CACHE
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jason Duell [:jduell] (needinfo me)
:
Mentors:
Depends on:
Blocks: 752663
  Show dependency treegraph
 
Reported: 2012-05-07 13:53 PDT by Jason Duell [:jduell] (needinfo me)
Modified: 2012-05-09 10:44 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Clarify in IDL that LOAD_FROM_CACHE may hit the network (1.27 KB, patch)
2012-05-07 13:54 PDT, Jason Duell [:jduell] (needinfo me)
michal.novotny: review+
Details | Diff | Splinter Review
Don't block SpeculativeConnect on LOAD_FROM_CACHE. (1.21 KB, patch)
2012-05-07 13:57 PDT, Jason Duell [:jduell] (needinfo me)
mcmanus: review-
Details | Diff | Splinter Review
Definition of PERSIST_FLAGS_FROM_CACHE implies LOAD_ONLY_FROM_CACHE should be used? (1.14 KB, patch)
2012-05-07 14:00 PDT, Jason Duell [:jduell] (needinfo me)
bzbarsky: review-
Details | Diff | Splinter Review
Clarify why we don't preconnect for some cache flags. (1.35 KB, patch)
2012-05-07 14:17 PDT, Jason Duell [:jduell] (needinfo me)
mcmanus: review+
Details | Diff | Splinter Review
Fix nsIWebBrowserPersist.idl comment (1.43 KB, patch)
2012-05-08 16:42 PDT, Jason Duell [:jduell] (needinfo me)
bzbarsky: review+
Details | Diff | Splinter Review

Description Jason Duell [:jduell] (needinfo me) 2012-05-07 13:53:11 PDT
This bug contains some uses of LOAD_FROM_CACHE that appear to (mistakenly) assume that the flag guarantees we'll never load the channel from the network. See bug 752663 for overview of the issue.
Comment 1 Jason Duell [:jduell] (needinfo me) 2012-05-07 13:54:58 PDT
Created attachment 621720 [details] [diff] [review]
Clarify in IDL that LOAD_FROM_CACHE may hit the network
Comment 2 Jason Duell [:jduell] (needinfo me) 2012-05-07 13:57:47 PDT
Created attachment 621723 [details] [diff] [review]
Don't block SpeculativeConnect on LOAD_FROM_CACHE.

Patrick:  I'm not sure 100% that we want this.  My understanding is that the primary use of LOAD_FROM_CACHE is session restore and history back/forward navigation.  We do hit the net if a resource isn't available in those cases, so the speculative cxn is a benefit.  OTOH we're also opening a lot of channels, most of which will be hit from cache, so they'll be a lot of speculative opens and most of them will not be used.
Comment 3 Jason Duell [:jduell] (needinfo me) 2012-05-07 14:00:37 PDT
Created attachment 621724 [details] [diff] [review]
Definition of PERSIST_FLAGS_FROM_CACHE implies LOAD_ONLY_FROM_CACHE should be used?

bz: I don't know anything about nsWebBrowserPersist, but the comment here seems to imply that we want LOAD_ONLY_FROM_CACHE in addition (or instead?) of LOAD_FROM_CACHE when PERSIST_FLAGS_FROM_CACHE is set:

   http://mxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/public/nsIWebBrowserPersist.idl#57
Comment 4 Patrick McManus [:mcmanus] PTO until Sep 6 2012-05-07 14:02:03 PDT
Comment on attachment 621723 [details] [diff] [review]
Don't block SpeculativeConnect on LOAD_FROM_CACHE.

its all about how you define likely.. so in this case I did mean to disable preconnects on this basis, lacking real data.
Comment 5 Jason Duell [:jduell] (needinfo me) 2012-05-07 14:17:03 PDT
Created attachment 621735 [details] [diff] [review]
Clarify why we don't preconnect for some cache flags.

Patrick: ok, fair enough.  Let's document it?
Comment 6 Boris Zbarsky [:bz] 2012-05-08 12:09:23 PDT
Comment on attachment 621724 [details] [diff] [review]
Definition of PERSIST_FLAGS_FROM_CACHE implies LOAD_ONLY_FROM_CACHE should be used?

I have no idea what the intent was, but the main in-tree consumer ("save as" in the Firefox UI) certainly expects the current behavior.  I believe the right thing here is to just fix the IDL comment to reflect reality.
Comment 7 Jason Duell [:jduell] (needinfo me) 2012-05-08 16:42:25 PDT
Created attachment 622204 [details] [diff] [review]
Fix nsIWebBrowserPersist.idl comment
Comment 8 Boris Zbarsky [:bz] 2012-05-08 17:41:58 PDT
Comment on attachment 622204 [details] [diff] [review]
Fix nsIWebBrowserPersist.idl comment

r=me
Comment 11 Jason Duell [:jduell] (needinfo me) 2012-05-09 10:44:10 PDT
dev-doc-needed?  No behavior actually changed, just correcting/refining IDL comments.

Note You need to log in before you can comment on or make changes to this bug.