The default bug view has changed. See this FAQ.

Fix misuses of LOAD_FROM_CACHE

RESOLVED FIXED in mozilla15

Status

()

Core
Networking: Cache
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jduell, Assigned: jduell)

Tracking

({dev-doc-needed})

unspecified
mozilla15
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
(Assignee)

Comment 1

5 years ago
Created attachment 621720 [details] [diff] [review]
Clarify in IDL that LOAD_FROM_CACHE may hit the network
Attachment #621720 - Flags: review?(michal.novotny)
(Assignee)

Comment 2

5 years ago
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.
Attachment #621723 - Flags: review?(mcmanus)
(Assignee)

Comment 3

5 years ago
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
Attachment #621724 - Flags: review?(bzbarsky)
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.
Attachment #621723 - Flags: review?(mcmanus) → review-
(Assignee)

Updated

5 years ago
Blocks: 752663
(Assignee)

Comment 5

5 years ago
Created attachment 621735 [details] [diff] [review]
Clarify why we don't preconnect for some cache flags.

Patrick: ok, fair enough.  Let's document it?
Attachment #621723 - Attachment is obsolete: true
Attachment #621735 - Flags: review?(mcmanus)
Attachment #621735 - Flags: review?(mcmanus) → review+
Attachment #621720 - Flags: review?(michal.novotny) → review+
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.
Attachment #621724 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 7

5 years ago
Created attachment 622204 [details] [diff] [review]
Fix nsIWebBrowserPersist.idl comment
Assignee: nobody → jduell.mcbugs
Attachment #621724 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #622204 - Flags: review?(bzbarsky)
Comment on attachment 622204 [details] [diff] [review]
Fix nsIWebBrowserPersist.idl comment

r=me
Attachment #622204 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f16221d9c85d
https://hg.mozilla.org/integration/mozilla-inbound/rev/06da6c1f3bed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4599e608efb
https://hg.mozilla.org/mozilla-central/rev/f4599e608efb
https://hg.mozilla.org/mozilla-central/rev/06da6c1f3bed
https://hg.mozilla.org/mozilla-central/rev/f16221d9c85d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Assignee)

Comment 11

5 years ago
dev-doc-needed?  No behavior actually changed, just correcting/refining IDL comments.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.