Closed Bug 1027579 Opened 5 years ago Closed 4 years ago

Add docShell flag DISABLE_CACHE to disable all caching for the lifetime of a page, including AJAX (XHR), web fonts, images etc.

Categories

(Core :: Networking: Cache, defect)

32 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jon.rimmer, Assigned: dragana)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140616004003

Steps to reproduce:

1. Open the DevTools
2. Select "Disable Cache"
3. Browse a site that makes requests via XHR


Actual results:

XHR requests are served from the cache.


Expected results:

XHR requests shouldn't be served from the cache.
Component: Untriaged → Developer Tools
"Disable cache" reloads the page, are you sure you are not observing requests that were made before that?
Yes, I'm certain. It's actually that I'm *not* observing requests. When the cache is disabled, everything that triggers and XHR request should show up in the network tab, but I'm seeing many, many requests not appear and instead the data returned to the JS code is the stale, cached version.
Assignee: nobody → mratcliffe
Assignee: mratcliffe → nobody
Status: UNCONFIRMED → NEW
Component: Developer Tools → Networking: Cache
Ever confirmed: true
Product: Firefox → Core
We are using this to disable the cache:
this.docShell.defaultLoadFlags = Ci.nsIRequest.LOAD_BYPASS_CACHE | Ci.nsIRequest.INHIBIT_CACHING;
Assignee: nobody → mratcliffe
Summary: "Disable Cache" does not disable caching for XHR → Make aysnc XHR obey caching from parent docShell
Attached patch ajax-cache-1027579.patch (obsolete) — Splinter Review
This patch should fix it.
Attachment #8445164 - Flags: review?(honzab.moz)
Attachment #8445164 - Flags: review?(honzab.moz) → review?(bzbarsky)
Component: Networking: Cache → DOM
Sweet! FYI a similar bug seems to affect fonts: https://bugzilla.mozilla.org/show_bug.cgi?id=1027124
Comment on attachment 8445164 [details] [diff] [review]
ajax-cache-1027579.patch

Why doesn't this already work?  nsLoadGroup::MergeLoadFlags should propagate at least the LOAD_BYPASS_CACHE flag, though it doesn't propagate INHIBIT_CACHING.

Is this because the XHR is happening after the page has completed loading so !mDefaultLoadRequest in the loadgroup?  If so, that's somewhat on purpose: when doing a cache-bypassing reload we don't want to disable caching completely for the lifetime of the web page...

If we're in that situation, that suggests that if we're going to have a "really, do disable the cache for the lifetime of the web page" mode then we should tell the loadgroup we're in that mode and have our existing machinery for propagating load flags to all requests deal instead of whack-a-moling every single place we start a load.
Attachment #8445164 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8445164 [details] [diff] [review]
> ajax-cache-1027579.patch
> 
> Why doesn't this already work?  nsLoadGroup::MergeLoadFlags should propagate
> at least the LOAD_BYPASS_CACHE flag, though it doesn't propagate
> INHIBIT_CACHING.
> 
> Is this because the XHR is happening after the page has completed loading so
> !mDefaultLoadRequest in the loadgroup?  If so, that's somewhat on purpose:
> when doing a cache-bypassing reload we don't want to disable caching
> completely for the lifetime of the web page...
> 
> If we're in that situation, that suggests that if we're going to have a
> "really, do disable the cache for the lifetime of the web page" mode then we
> should tell the loadgroup we're in that mode and have our existing machinery
> for propagating load flags to all requests deal instead of whack-a-moling
> every single place we start a load.

I think something like a DISABLE_CACHE flag that lasts for the lifetime of the docShell makes the most sense. Of course, this should also work for everything else that we cache (images, fonts etc.)

I am going to bow out of this one and leave it to somebody more familiar with our caching system.
Assignee: mratcliffe → nobody
OS: Mac OS X → All
Hardware: x86 → All
Repurposing this bug to add a docShell flag DISABLE_CACHE to disable all caching including AJAX (XHR), web fonts, images etc. for the lifetime of the docShell as per comment 7.
Summary: Make aysnc XHR obey caching from parent docShell → Add docShell flag DISABLE_CACHE to disable all caching for the lifetime of a page, including AJAX (XHR), web fonts, images etc.
Assignee: nobody → mratcliffe
This issue is still present in Firefox 33.1 AND 35.0a2

I was had set the "Disable Cache" as true, and lodaed my page. My page had a script (bootstrap.js), which used requirejs to dynamically load another script (dyn.js) when a condition is satisfied. Everytime I changed the base `bootstrap.js` script, it loaded correctly, but the `dyn.js` was ALWAYS served from cache. Even after Disabling the cache. I was tearing my hairs apart on this... the only way to get it working was to clear cache manually (Ctrl+Shift+Del) on every reload. :(

Finally, I figured that the "Cache-Control: public, max-age=86400000" header was set, which may be causing this XHR request to be cached. Set the max-age to 0 to get it working, but I realised this is a big bug in Firefox. Especially, if the Firefox version being used is called "Developer Edition"...
A work around is to use private tabs. https://addons.mozilla.org/en-US/firefox/addon/private-tab/

:D

But it's definitely a feature I'd love to see. So much cache bug with fonts and swf.
Comment on attachment 8445164 [details] [diff] [review]
ajax-cache-1027579.patch

Filter on 1ff0543e-b501-4893-a72b-e4773c01e655
Attachment #8445164 - Attachment is obsolete: true
@Honza: I have heard a lot of criticism that our devtool's disable cache implementation is broken, which moves people away from our tools.

We are using this to disable the cache:
this.docShell.defaultLoadFlags = Ci.nsIRequest.LOAD_BYPASS_CACHE | Ci.nsIRequest.INHIBIT_CACHING;

But that obviously only disables the cache for page load and then the cache is active again for XHR, font imports etc.

bz suggests that we add something like:
Ci.nsIRequest.DISABLE_CACHE: Really, do disable the cache for the lifetime of the web page for all data types.

I am guessing that the cache is disabled for all data types in private browsing mode so I don't think it would be too much work (at least I hope so).

Is this something that you could take a look at?

Filter on 1ff0543e-b501-4893-a72b-e4773c01e655
Assignee: mratcliffe → nobody
Component: DOM → Networking: Cache
Flags: needinfo?(honzab.moz)
Private browsing mode uses an entirely separate cache, rather than disabling it.
(In reply to Josh Matthews [:jdm] from comment #15)
> Private browsing mode uses an entirely separate cache, rather than disabling
> it.

But still it's caching (memory/session only).  So the bug still applies.
Flags: needinfo?(honzab.moz)
We may want to add some flag on load group (not a load flag, rather a property).  When set, set "NO_CACHE" flags on each added request.

Problem is that load group is reused for following document navigation.  But maybe it's not that big problem, since the devtool's "disable cache" switch should work globally anyway, right?
Jason, could you find someone to work on this?  I won't get to anything sooner then in week or two since now.
Flags: needinfo?(jduell.mcbugs)
(In reply to Honza Bambas (:mayhemer) from comment #17)
> We may want to add some flag on load group (not a load flag, rather a
> property).  When set, set "NO_CACHE" flags on each added request.
> 
> Problem is that load group is reused for following document navigation.  But
> maybe it's not that big problem, since the devtool's "disable cache" switch
> should work globally anyway, right?

The DevTools "disable cache" feature is expected to apply when enabled and the toolbox is open on the tab.  So, I think what you're saying is fine, as long as resetting these new flags / properties to "default" and forcing the tab to reload is enough to go back to normal cache use when the toolbox is closed.
(In reply to Honza Bambas (:mayhemer) from comment #17)
> We may want to add some flag on load group (not a load flag, rather a
> property).  When set, set "NO_CACHE" flags on each added request.
> 
> Problem is that load group is reused for following document navigation.  But
> maybe it's not that big problem, since the devtool's "disable cache" switch
> should work globally anyway, right?

Ideally we would be able to disable the cache for a tab and all of it's children whenever the toolbox is open for that tab.

If that would be too much work then disabling the cache for all tabs and their children would be okay.

We don't mind having to reload the tab to apply the changes if we need to.
Michal's going to look into this.
Assignee: nobody → michal.novotny
Flags: needinfo?(jduell.mcbugs)
Redirections which are cached are also not disabled when the option in devtools is ticked on.
Assignee: michal.novotny → dd.mozilla
Bug 1027579 - Introduce DISABLE_CACHE load mode that disable cahe completely. r=:bz
Attachment #8629235 - Flags: review?(bzbarsky)
Sorry about the driveby but does this patch disable the cache for:
- dom/base/nsXMLHttpRequest.cpp
- dom/html/HTMLMediaElement.cpp
- dom/media/MediaResource.cpp
- @imported fonts & stylesheets.

There are probably a bunch of other places but nothing should be cached.
Flags: needinfo?(dd.mozilla)
Flags: needinfo?(dd.mozilla)
Attachment #8629235 - Flags: review?(bzbarsky)
imported fonds are not downloaded again :)
https://reviewboard.mozilla.org/r/12541/#review11151

::: netwerk/base/nsIRequest.idl:224
(Diff revision 1)
> +    const unsigned long DISABLE_CACHE = 1 << 16;

This is already LOAD_DOCUMENT_URI.

::: netwerk/protocol/http/nsHttpChannel.cpp:2758
(Diff revision 1)
> -    if (offline || (mLoadFlags & INHIBIT_CACHING)) {
> +

I'm not a necko peer.  Please ask one of them to review this....
Attached patch bug_1027579_v1.patch (obsolete) — Splinter Review
Not all calls to NS_NewChannel... have loadGroup set therefore for some loadFlags has flag DISABLE_CACHE is set.
I have not changed all loadFlags to have this flag set, because i am afraid that it is error-prone in the future
Attachment #8629235 - Attachment is obsolete: true
Attachment #8630468 - Flags: feedback?(michal.novotny)
Attachment #8630468 - Flags: feedback?(bzbarsky)
Comment on attachment 8630468 [details] [diff] [review]
bug_1027579_v1.patch

Seems at least plausible, though I didn't read too carefully.  The nsIRequest change should put things in the right order, of course.
Attachment #8630468 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8630468 [details] [diff] [review]
bug_1027579_v1.patch

Thanks, Honza!
Attachment #8630468 - Flags: feedback?(michal.novotny) → review?(honzab.moz)
Comment on attachment 8630468 [details] [diff] [review]
bug_1027579_v1.patch

Review of attachment 8630468 [details] [diff] [review]:
-----------------------------------------------------------------

dropping r now, few questions raised up.  few comments..

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2763,5 @@
>      uint32_t cacheEntryOpenFlags;
>      bool offline = gIOService->IsOffline() || appOffline;
> +    uint32_t loadGroupDefaultFlags;
> +    if (mLoadGroup) {
> +        mLoadGroup->GetDefaultLoadFlags(&loadGroupDefaultFlags);

initialize loadGroupDefaultFlags !!

@@ +5062,5 @@
>      gHttpHandler->AddConnectionHeader(&mRequestHead.Headers(), mCaps);
>  
> +    uint32_t loadGroupDefaultFlags;
> +    if (mLoadGroup) {
> +        mLoadGroup->GetDefaultLoadFlags(&loadGroupDefaultFlags);

as well here!

::: toolkit/devtools/server/actors/webbrowser.js
@@ +1466,2 @@
>  
>      this.docShell.defaultLoadFlags = disabled ? disable : enable;

should this be |= ?

@@ +1522,1 @@
>      return this.docShell.defaultLoadFlags === disable;

isn't this wrong? should be 'return !!(t.dS.dLF & d);' ?
Attachment #8630468 - Flags: review?(honzab.moz)
Depends on: 1182066
Attachment #8630468 - Attachment is obsolete: true
Bug 1182066 will fix most of cases. The bug 1182066 will land soon.
I notice only that fonts do not check defaults flags. Maybe there are some other cases that I have missed.
In FontFaceSet only loadType is check to decide if cache needs to be bypassed. The default loadGroup flags are not check. I do not know this code so my question:
Can we add check for default flags: around line 
http://mxr.mozilla.org/mozilla-central/source/layout/style/FontFaceSet.cpp#1333
add 
uint32_t flags;
if (NS_SUCCEEDED(docShell->GetDefaultLoadFlags(&flags))) {
  if (flags & nsIRequest::DISABLE_CACHE) {
    *aBypassCache = true;
  }
}

or we need another flag DISABLE_CACHE that is only use for devTools?

John, I am not sure who is the right person to answer this question.
Flags: needinfo?(jdaggett)
(In reply to Dragana Damjanovic [:dragana] from comment #32)
> In FontFaceSet only loadType is check to decide if cache needs to be
> bypassed. The default loadGroup flags are not check. I do not know this code
> so my question:
> Can we add check for default flags: around line 
> http://mxr.mozilla.org/mozilla-central/source/layout/style/FontFaceSet.
> cpp#1333
> add 
> uint32_t flags;
> if (NS_SUCCEEDED(docShell->GetDefaultLoadFlags(&flags))) {
>   if (flags & nsIRequest::DISABLE_CACHE) {
>     *aBypassCache = true;
>   }
> }
> 
> or we need another flag DISABLE_CACHE that is only use for devTools?
> 
> John, I am not sure who is the right person to answer this question.

Yeah, I'm not really the right person, sorry. bz, sicking maybe?
Flags: needinfo?(jdaggett)
Can you answer the question in comment #32 (see also comment #31)
Thank you!
Flags: needinfo?(bzbarsky)
So this is the in-memory font cache, right?

Checking the docshell flags is probably ok if those are meant to bypass in-memory object caches.  But are they?  That's not the behavior we have for images last I checked (or at least I really hope not)...

More specifically, it seems to me that disabling the in-memory font cache is only ok if we do one load per font per document to start with.  If we rely on the font cache to coalesce font loads for a particular document, then we'll need some other coalescing mechanism, right?  It would be pretty weird if we loaded a font multiple times just because multiple elements on the page are styled with that font or something, even if caches are supposed to be "disabled".
Flags: needinfo?(bzbarsky)
Dragana, what's the latest status for the overall issue here?  With bug 1182066 landed, does it mean caching should be correctly disabled for nearly all request types, except for fonts?  Or are there other exceptions?
Flags: needinfo?(dd.mozilla)
I have tested it  and looked though the code, as far as i know it is only fonts.
Flags: needinfo?(dd.mozilla)
Abouts fonts:

So fonds can be cached in UserFontCache and there is a normal necko cache.
Now, if fonts are found in the UserFontCache the fonts are served from there no matter if nsIRequest::LOAD_BYPASS_CACHE is set or not.

If the fonts are not found in UserFontCache and nsIRequest::LOAD_BYPASS_CACHE is not set fonts will be caught from network 2-3 times and afterwards they are served from http cache (tested it with youtube).

If the fonts are not found in UserFontCache and nsIRequest::LOAD_BYPASS_CACHE is set fonts will be caught from network 5-6 times (maybe they will be served from UserFontCache afterwards but I have not check that) 

I have not taken a good look at UserFontCache, I am not sure how this cache is handled, when entries are deleted.

so the question is - can we make this better to fetch fonts only once. In my opinion this should be in UserFontCache and not in http cache. If we fix this we can check nsIRequest::LOAD_BYPASS_CACHE befor serving font from UserFontCache and refresh it. But I do not know much about UserFontCache.
Hi
can you look at comment 38(and some above). I am not familiar with FontCache code. I am not sure if you are the right person to ask, but please needinfo someone you thing can take a look.

Maybe it would be the better to open another bug for this.
Flags: needinfo?(jfkthame)
(In reply to Boris Zbarsky [:bz] from comment #35)
> So this is the in-memory font cache, right?
> 
> Checking the docshell flags is probably ok if those are meant to bypass
> in-memory object caches.  But are they?  That's not the behavior we have for
> images last I checked (or at least I really hope not)...
> 
> More specifically, it seems to me that disabling the in-memory font cache is
> only ok if we do one load per font per document to start with.  If we rely
> on the font cache to coalesce font loads for a particular document, then
> we'll need some other coalescing mechanism, right?  It would be pretty weird
> if we loaded a font multiple times just because multiple elements on the
> page are styled with that font or something, even if caches are supposed to
> be "disabled".

If we bypass the UserFontCache, that shouldn't make us load a font multiple times just because multiple elements are styled with it, provided those elements are using the same font-family and other properties, resolving to the same @font-face rule. But it likely would cause us to load the font multiple times if there are multiple separate @font-face rules referring to the same resource.

I'm inclined to think that's probably OK. If a page has multiple @font-face rules loading the same resource (or if it's frequently reconstructing stylesheets, such that old @font-face rules get thrown away and new, identical ones get created), that sounds like an inefficiency that could usefully be surfaced by noting that it triggers repeated font loads.

Note that there's another issue with the UserFontCache at the moment, which is that it will fail to coalesce font loads if the first load hasn't completed yet at the time when a second rule requests the same resource. The UserFontCache only knows about fonts that have been fully downloaded and validated. See also bug 1188802.
Flags: needinfo?(jfkthame)
Attached patch bug_1027579.patch (obsolete) — Splinter Review
Is this ok to do?
Attachment #8658160 - Flags: feedback?(jfkthame)
Comment on attachment 8658160 [details] [diff] [review]
bug_1027579.patch

Review of attachment 8658160 [details] [diff] [review]:
-----------------------------------------------------------------

IMO, I think it'd be OK to do this; but if Boris thinks it's a bad idea (after considering comment 40), we should definitely listen to his concerns.
Attachment #8658160 - Flags: feedback?(jfkthame)
Attachment #8658160 - Flags: feedback?(bzbarsky)
Attachment #8658160 - Flags: feedback+
Comment on attachment 8658160 [details] [diff] [review]
bug_1027579.patch

Yeah, for purposes of this debugging mode this is probably ok given comment 40.
Attachment #8658160 - Flags: feedback?(bzbarsky) → feedback+
Attached patch bug_1027579_v1.patch (obsolete) — Splinter Review
For comment #43, I have added a separate flag so that it is used only for devtools.
Attachment #8658160 - Attachment is obsolete: true
Attachment #8659243 - Flags: review?(michal.novotny)
Attachment #8659243 - Flags: review?(jfkthame)
Comment on attachment 8659243 [details] [diff] [review]
bug_1027579_v1.patch

Review of attachment 8659243 [details] [diff] [review]:
-----------------------------------------------------------------

Although in principle I think this is OK to do, I'm not comfortable with the proposed flag name here; see below for some ideas, though I'd be happy to consider alternatives as well if something else seems more accurate/helpful.

::: layout/style/FontFaceSet.cpp
@@ +1344,5 @@
> +  if (NS_SUCCEEDED(docShell->GetDefaultLoadFlags(&flags))) {
> +    if (flags & nsIRequest::DISABLE_CACHING_COMPLETELY) {
> +      *aBypassCache = true;
> +    }
> +  }

This whole chunk should be inside the |if (docShell)| condition.

::: netwerk/base/nsIRequest.idl
@@ +153,5 @@
> +     * with LOAD_BYPASS_CACHE, because it is going to disable just special
> +     * cases where LOAD_BYPASS_CACHE cannot be used, e.g. FontCache. It is used
> +     * for dev tools.
> +     */
> +    const unsigned long DISABLE_CACHING_COMPLETELY = 1 << 5;

I don't think this is a good name for the new flag, as this bit does not "disable caching completely" at all. As the comment suggests, it is more like a supplement to the existing BYPASS_CACHE.

Maybe something more like BYPASS_ (or DISABLE_) SPECIAL_CACHES?

Are there any other "special caches" that we should consider here? If not, maybe this should be explicitly BYPASS_FONT_CACHE.

Also, please move this so that this group of flag bits are listed in sequential order; that makes it easier to see which bits are defined.
Attachment #8659243 - Flags: review?(jfkthame) → review-
(In reply to Jonathan Kew (:jfkthame) from comment #46)
> Comment on attachment 8659243 [details] [diff] [review]
> bug_1027579_v1.patch
> 
> Review of attachment 8659243 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: netwerk/base/nsIRequest.idl
> @@ +153,5 @@
> > +     * with LOAD_BYPASS_CACHE, because it is going to disable just special
> > +     * cases where LOAD_BYPASS_CACHE cannot be used, e.g. FontCache. It is used
> > +     * for dev tools.
> > +     */
> > +    const unsigned long DISABLE_CACHING_COMPLETELY = 1 << 5;
> 
> I don't think this is a good name for the new flag, as this bit does not
> "disable caching completely" at all. As the comment suggests, it is more
> like a supplement to the existing BYPASS_CACHE.
> 
> Maybe something more like BYPASS_ (or DISABLE_) SPECIAL_CACHES?
> 
> Are there any other "special caches" that we should consider here? If not,
> maybe this should be explicitly BYPASS_FONT_CACHE.
> 
> Also, please move this so that this group of flag bits are listed in
> sequential order; that makes it easier to see which bits are defined.

I gave it a more generic name. I only know that this is needed for FontCache, but I am not sure about other cases, maybe there are. I had investigated, but I could not guaranty that this is the only one. 

I changed it to LOAD_BYPASS_FONT_CACHE
Attached patch bug_1027579_v1.patch (obsolete) — Splinter Review
Attachment #8659243 - Attachment is obsolete: true
Attachment #8659243 - Flags: review?(michal.novotny)
Attachment #8660658 - Flags: review?(michal.novotny)
Attachment #8660658 - Flags: review?(jfkthame)
Comment on attachment 8660658 [details] [diff] [review]
bug_1027579_v1.patch

Review of attachment 8660658 [details] [diff] [review]:
-----------------------------------------------------------------

This seems OK to me, modulo some comment tidyup (see below).

However, note that I'm not a docshell peer (and have no experience there), so you should probably get the addition of this flag reviewed by someone from that module too.

::: netwerk/base/nsIRequest.idl
@@ +129,5 @@
>       */
>  
>      /**
> +     * For devtools it is necessary to disable caching completely and
> +     * LOAD_BYPASS_CACHE will not disable FondCache, because of the performance

s/FontCache/UserFontCache/

@@ +130,5 @@
>  
>      /**
> +     * For devtools it is necessary to disable caching completely and
> +     * LOAD_BYPASS_CACHE will not disable FondCache, because of the performance
> +     * reasons. This flag is used for disabling caching in this spacial case.

s/spacial/special/

@@ +131,5 @@
>      /**
> +     * For devtools it is necessary to disable caching completely and
> +     * LOAD_BYPASS_CACHE will not disable FondCache, because of the performance
> +     * reasons. This flag is used for disabling caching in this spacial case.
> +     * It should be used with LOAD_BYPASS_CACHE.

Hmm, the comment above this group of flags says that they "control the flow of data into the cache"; but that's not an accurate description in this case, AFAICS. This flag affects lookup of existing cache entries (which is actually the more important consideration here) as well as caching of new ones. So perhaps better to add the new flag separately, before that comment.

(It's also not clear to me that INHIBIT_PIPELINE really fits under the above comment either, in the way the next two flags do. Maybe that flag, added back in bug 599164, should have gone above that comment, too.)
Attachment #8660658 - Flags: review?(jfkthame) → review+
Comment on attachment 8660658 [details] [diff] [review]
bug_1027579_v1.patch

Review of attachment 8660658 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the change in nsIRequest.idl
Attachment #8660658 - Flags: review?(michal.novotny) → review+
Attached patch bug_1027579_v1.patch (obsolete) — Splinter Review
only incorporated comments from jfkthame.

Boris, can you review docShell part. Thanks!
Attachment #8660658 - Attachment is obsolete: true
Attachment #8661114 - Flags: review?(bzbarsky)
Comment on attachment 8661114 [details] [diff] [review]
bug_1027579_v1.patch

There isn't any obvious docshell stuff here...  Did you mean the font bits?

I think we should have a better name for this flag that's not font-specific, if it's going to be a general nsIRequest flag; on the other hand nothing else looks at the flag.  We should probably also keep the flags in nsIRequest.idl ordered. 

But past that, what exactly are these unspecified "performance issues" that mean we can't just use LOAD_BYPASS_CACHE?
Attachment #8661114 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #52)
> Comment on attachment 8661114 [details] [diff] [review]
> bug_1027579_v1.patch
> 
> There isn't any obvious docshell stuff here...  Did you mean the font bits?

jfkthame wanted someone who knows docshell to take a look.

> 
> I think we should have a better name for this flag that's not font-specific,
> if it's going to be a general nsIRequest flag; on the other hand nothing
> else looks at the flag.  We should probably also keep the flags in
> nsIRequest.idl ordered. 
> 

I have not found any other special case that would need a special cache-disable flag. I do not know font's code at all I will try to figure it out :)

> But past that, what exactly are these unspecified "performance issues" that
> mean we can't just use LOAD_BYPASS_CACHE?

comment #40 and comment #43.
> comment #40 and comment #43.

I thought the upshot of those was that just bypassing the font cache on LOAD_BYPASS_CACHE was fine?
Sorry, I misunderstood. 

this patch blocks loading of fonds from cache if LOAD_BYPASS_CACHE is set.
Attachment #8661114 - Attachment is obsolete: true
Attachment #8661737 - Flags: review?(jfkthame)
Attachment #8661737 - Flags: review?(bzbarsky)
Comment on attachment 8661737 [details] [diff] [review]
bug_1027579.patch

Review of attachment 8661737 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, r=me.

Given all the preceding discussion, I don't think you need an additional review from bz here.
Attachment #8661737 - Flags: review?(jfkthame)
Attachment #8661737 - Flags: review?(bzbarsky)
Attachment #8661737 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3560ae11e7eb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Did we fix not letting post-onload xhr load from the cache in this bug?
Flags: needinfo?(dd.mozilla)
(In reply to Andrew Overholt [:overholt] from comment #60)
> Did we fix not letting post-onload xhr load from the cache in this bug?

I am not sure. Maybe  bug 1182066 fix that. Do you have an example (you mean just do  xhr request when document onload is fired)?

I did test some xhr request, but maybe I miss some cases.

Somewhere in this bug I mentioned that it would be good to have list of all cases that do not work to test so that I do not miss something.
Flags: needinfo?(dd.mozilla)
Depends on: 1247669
You need to log in before you can comment on or make changes to this bug.