1087320 - Don't propagate LOAD_FROM_CACHE to the whole load group on charset reload

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[peter]

Attachment: first-request.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.27 Safari/537.36

Steps to reproduce:

* Go to https://soundcloud.com
* login - only then should the requests not be cached
* Open your network panel and filter XHR only to see best
* go to any user profile (just as an example, problem exists in general)


Actual results:

* First page load has many requests to our API
* Consecutive page loads have almost no requests although server response indicates no caching (see my screenshots)

Screenshots: https://www.dropbox.com/sh/mh88v2csqxx437j/AADCH-E_I2eCOP0I1YpBn7hGa?dl=0

I discovered this while working on soundcloud and our acceptance tests failing in firefox. The too heavy caching starts with Firefox 33 and is still broken on 34 beta and 35 alpha. 32 and below are fine.


Expected results:

The consecutive page loads should also make the API calls again.

Comment 1

[peter]

Attachment: second-request.png

Comment 2

[peter]

Any news on this bug? It still affects soundcloud pretty strong.

Comment 3

[Michal Novotny [:michal]]

The images just prove that we correctly re-validate the requests when the server dictates it in the response. Could you please provide an exact URL of the entry that shouldn't be cached and we read the content from the cache?

Comment 4

[peter]

If you look at the screenshot of the first request you can see one highlighted request that has

Cache-Control: "private, max-age=0"

So the request should not in any way be cached. Yet in the screenshot of the second request right after that this request is not being made. I think that proves the issue pretty well.

I can't provide you an exact request URL here, because the issue only shows when you are logged in (only then do we tell the browser not cache) and I don't want to share my oauth token here. That is why I detailed the reproduction steps in the description thought.

Can I help you in any other way from my side?

Comment 5

[Michal Novotny [:michal]]

(In reply to peter from comment #4)
> If you look at the screenshot of the first request you can see one
> highlighted request that has
> 
> Cache-Control: "private, max-age=0"
> 
> So the request should not in any way be cached. Yet in the screenshot of the
> second request right after that this request is not being made. I think that
> proves the issue pretty well.

Sorry, I didn't understand the report correctly since I was confused with the 304 response on the second image and your statement that the server's response indicates no caching. In fact, the server just says that we should re-validate the entry before we use the cached content.

What happens is that docshell starts loading the document, it starts parsing the response and detects a charset change. It immediately cancels the channel, sets the LOAD_FROM_CACHE flag and reloads the document. The problem is that LOAD_FROM_CACHE flag is propagated to the loadgroup's flags since the channel is set as the default load request for that group. Any subresource then gets the LOAD_FROM_CACHE flag from the loadgroup, so we use the cached response without re-validating it with the server.

I'm not sure how this is supposed to work. I understand why docshell wants to reload the main document from cache without re-validation if possible, but the loadgroup behavior seems correct to me.

Unlike Peter, I can reproduce the same problem with Firefox 32 (rev 44234f451065).

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I'm not sure how this is supposed to work.

"badly".

What you really want to do is to fetch all the stuff that you already loaded before the charset change from cache but fetch other stuff normally.  

The fact that the only API necko gives us here is the blunt loadgroup flags hammer is also broken in all sorts of ways, of course.

All of which begs the question: why the heck are we ending up with charset-change reloads on soundcloud?  That's supposed to be a fallback for semi-broken pages, not something the sort of site that cares about cache beahvior would have to deal with.

And the reason there's a charset-change reload is that there's an inline <script> before the <meta charset="utf-8"> and that script is logner than 512 bytes.  The HTML validator flags this as a clear error, precisely because of this problem: http://validator.w3.org/check?uri=https%3A%2F%2Fsoundcloud.com%2F&charset=%28detect+automatically%29&doctype=Inline&group=0

Just fixing the HTML to be valid would fix all the problems here for soundcloud in particular.

Back to what we should do for sites that trigger charset reloads... my gut feeling is that we should read the document itself from cache, but not subresources.  Necko would need to provide an API to do that, though.

Comment 7

[peter]

Thanks Boris, we will move our <meta charset="utf-8"> tag further up in the page and keep you guys posted if that resolves our issue.

Comment 8

[peter]

I can verify that this fixes our issue.

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: wip1

Could the solution be something like this?  Not sure how about child docshells propagation.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8699505 [details] [diff] [review]
wip1

Adding more likely-fragile state flags to docshell is not great.

But apart from that, this is probably OK.

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> Comment on attachment 8699505 [details] [diff] [review]
> wip1
> 
> Adding more likely-fragile state flags to docshell is not great.

I hate flags myself too.  Maybe we could introduce a flag or property set only on the default channel and not globally on the load group.

> 
> But apart from that, this is probably OK.

If the above is found to be too complicated or impossible, would you r+ this "wip1" patch?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yes.

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: v1

Jason, please check the network and ipc changes.  Boris, please check the docshell part (this time really tiny!)

- there is a new property on nsICacheInfoChannel telling the channel to behave as if the LOAD_FROM_CACHE flags has been set, but w/o actually setting that flag
- this prevents LOAD_FROM_CACHE from being propagated to the whole load group while letting the doc go from cache when possible

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f22c9fa4e6a2


(Wouldn't be simplest to just always filter LOAD_FROM_CACHE from being propagated to load group with a/the merge mask?  Maybe we have cases we want that propagation to work tho)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8707090 [details] [diff] [review]
v1

>+      // property on the channel to not later propagete the flag on

"propagate"

r=me on the docshell bits.

Comment 16

[Jason Duell]

Comment on attachment 8707090 [details] [diff] [review]
v1

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

+r as long as you rename .loadFromCache to something better.

::: docshell/base/nsDocShell.cpp
@@ +10967,5 @@
>  
> +    case LOAD_RELOAD_CHARSET_CHANGE: {
> +      // Instead of setting LOAD_FROM_CACHE better set this special
> +      // property on the channel to not later propagete the flag on
> +      // the whole load group.

Shorter/clearer comment?

  // Use SetLoadFromCache (not LOAD_FROM_CACHE flag) since we only want to force cache load for this channel, not the whole loadGroup.

::: netwerk/base/nsICacheInfoChannel.idl
@@ +49,5 @@
> +  /**
> +   * Tells the channel to behave as if the LOAD_FROM_CACHE flag has been set,
> +   * but not exposing that flag externally.  We want to use this mechanism
> +   * to not set LOAD_FROM_CACHE under certain circumstances like charset
> +   * reloads on the whole load group.

"Tells the channel to behave as if the LOAD_FROM_CACHE flag has been set, but without affecting the loads for the entire loadGroup."

(I don't think you need to mention the specific charset reload use case)

::: netwerk/protocol/http/HttpChannelChild.h
@@ +244,5 @@
>    bool mSuspendParentAfterSynthesizeResponse;
>  
> +  // If true, we behave as if the LOAD_FROM_CACHE flag has been set.
> +  // Used to enforce that flag's behavior but not expose it externally.
> +  bool mLoadFromCache;

I don't like the name 'loadFromCache'.  I think it's too easy to assume at a glance that this will be set for all channels that result in a cache load, etc.  I think "mForceLoadCache" or "mForceTryCache" would be better (including for the new field in cacheInfoChannel.idl).  "ForceTry" is best IMO because that best describes what happens.

I also think it would be simpler to just have a single bool in HttpBaseChannel rather than two variables with the same name (but one is a bitfield) in HttpChannelChild/nsHttpChannel.  But if you really care about saving 7 bits, I won't argue that :)

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #16)
> Comment on attachment 8707090 [details] [diff] [review]
> v1
> 
> Review of attachment 8707090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> +r as long as you rename .loadFromCache to something better.

> I don't like the name 'loadFromCache'.  

And I don't like the "force try cache".  That doesn't say at all what's going on.  

> I think it's too easy to assume at a
> glance that this will be set for all channels that result in a cache load,
> etc.  I think "mForceLoadCache"

That more sound like what LOAD_ONLY_FROM_CACHE does.

>  or "mForceTryCache" would be better

And this one doesn't make sense, we always try the cache with default load flags ;)

Since this attribute/property makes the channel behave as if the LOAD_FROM_CACHE flag is set, you probably complain about the name of that flag.  To rename it as part of this bug is an overkill, but we can think of how to best rename LOAD_FROM_CACHE to reflect what it does and use it for the new property name.  (To IMHO just make our naming even more confusing ;))

Ideal name would be "prefer stalled cached content".  That is what we actually do.  If there is an entry, we don't check any of Expired, age, Last-modified, ETag, Cache-control headers and just use the entry.

> I also think it would be simpler to just have a single bool in
> HttpBaseChannel rather than two variables with the same name (but one is a
> bitfield) in HttpChannelChild/nsHttpChannel.  But if you really care about
> saving 7 bits, I won't argue that :)

Yep, this flag should be in base, but I want base to use bitfields (if not already) first.  child as well.  bug 1240547.

Comment 18

[Jason Duell]

After some bikeshedding on IRC, we've come up with 'mAllowStaleCacheContent"

Comment 19

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Probably doesn't need a new try run (just a rename).  Builds for me locally.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f22c9fa4e6a2

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/865eddafd4e9

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/865eddafd4e9