I've been hoping that HTTP caching will work pretty much without modification under e10s (requests from the child work are served up by the parent transparently whether the data is cached or not in the parent/chrome process). But Honza seems to suggest that "it's not so simple." More info from Honza (or anyone else who knows the caching code) would be appreciated.

The DOM (and imagelib?) will try to hold cache tokens alive for content that is has loaded in some cases, I think.

I believe the DOM per se doesn't do anything of the sort. However there are various parts of the DOM that do things with cache entries (e.g. calling SetMetaDataElement on them) and I have no idea what the offline cache stuff does.

1. nsHTMLDocument is trying to cache charset. 2. nsContentSink is marking application cache items as foreign and needs the cache entry key for that (just optimization). 3. nsClassifierCallback is marking URLs as already classified. All these are using metadata on cache descriptors or access the descriptor itself. All are QI'ing channel to nsICacheChannel and gets nsICacheEntryDescriptor from it. CacheEntryDescriptorChild could be IPC'ed or done some different more efficient way, only for metadata and some r/o attributes. What I know about e10s concept it is intended to do all networking on the chrome thread, including cache entry initiations, reads and writes, right? How we open and read/write from/to cache entries I was writing in bug 536295 comment 1. Gathering of nsICacheEntryDescriptor instance happens in AsyncOpen (parent version) and reading from it starts in nsHttpChannel::ReadFromCache that should also happen on the parent process. Jason, from what exact comment you deduce it should not be that simple?

Honza, Yes, all the networking and cache stuff should be in the chrome process. It sounds like we'll need to figure out what to send to the child so it can fake cache descriptors well enough to support the child-side use-cases you describe. Other than that, hopefully caching won't need modification.

biesi told me some code that's likely to be in the child process will want to see expiration date info, so we may need to send it over the wire. (It might not be safe to try to get it from the parent at the time we need it, as channels drop their ref to a cache entry after OnStopRequest is called--though we could certainly delay that until after the child's OnStopRequest has returned if we need to).

What about WyciwygChannel? There is a lot of cache code so it should be in chrome process, but it is called from nsHTMLDocument and nsDocShell. So we need a protocol for nsWyciwygChannel?

> In bug #537164 I'm thinking about using synchronous calls to : > >- nsICacheEntryInfo::GetKey >- nsICacheEntryInfo::GetExpirationTime >- nsICacheEntryDescriptor::GetMetaDataElement > > They are all simple methods and making them async with callback > isn't IMHO good idea. Michal, So, as per comment 5, my impression is that you'll need to either 1) make sure that the cache entry has not been released on the chrome side at the time of these calls, i.e. OnStopRequest hasn't completed, or you've changed the parent code to release cache entries later (after the child's OnStopRequest has been called) 2) Copy the data that you need in SendOnStartRequest(), so the child already has it when you need it. If this is a common-case operation, #2 will be more efficient and thus preferred, all else being equal.

(from bsmedberg): I think that we definitely do not want to expose things like the "key" to the content process. Can we solve the problem as minimally as possible, by providing very specific methods for caching the charset? I don't know what "marking application cache items as foreign" means, but we probably also just want a specific method/message for that purpose. And... what does nsClassifierCallback do? That's the URL classifier service? I suspect we need to be running that service entirely in chrome and making specific requests from content (if content needs to know about url classifications at all). In any case, I really don't think we want to propagate the nsICache* interfaces as they currently exist into the content process.

(In reply to comment #8) > I don't know what "marking application cache items as foreign" means, but we > probably also just want a specific method/message for that purpose. This probably will get fixed in bug 536295. "marking application cache items as foreign" is vital for application cache loads where a web page referred a different manifest URL (an app cache id) then it had been loaded from (it had been found in an app cache with a different manifest URL, i.e. a different id, before it started to load). We must mark such entry as foreign that makes it not to load from a bad cache again.

This patch changes handling of expiration time and cache charset in nsDocshell and nsHTMLDocument. > 1) make sure that the cache entry has not been released on the chrome side at > the time of these calls, i.e. OnStopRequest hasn't completed, or you've changed > the parent code to release cache entries later (after the child's OnStopRequest > has been called) This isn't done yet. First I'd like to know if this is the right way to go. The URI classifier service needs to be run in chrome process. WyciwygChannel will probably use its own in-memory caching. I'll file separate bugs for both cases.

Comment on attachment 428307 [details] [diff] [review] patch Yes, this looks like a reasonable approach. Please document for the two attributes that they're equivalent to .cacheToken.expirationTime / .cacheToken.getMetaDataElement("charset")

- more verbose description of attributes cacheTokenExpirationTime and cacheTokenCachedCharset - now it is ensured that we have a cache entry in time of calling SetCacheTokenCachedCharset()

Comment on attachment 444763 [details] [diff] [review] patch v2 Just few nits: >@@ -949,22 +940,21 @@ nsHTMLDocument::StartDocumentLoad(const >- if(cacheDescriptor) { >+ if(cachingChan) { Maybe add space between if and ( ? >+++ b/docshell/base/nsDocShell.cpp >@@ -9485,29 +9485,27 @@ nsDocShell::AddToSessionHistory(nsIURI * >+ nsCOMPtr<nsICachingChannel> cacheChannel; > if (aChannel) { >- nsCOMPtr<nsICachingChannel> >- cacheChannel(do_QueryInterface(aChannel)); >+ cacheChannel = do_QueryInterface(aChannel); Check indention here. >+HttpChannelChild::SetCacheTokenCachedCharset(const nsACString &aCharset) >+ if (!SendSetCacheTokenCachedCharset(PromiseFlatCString(aCharset))) { Is there really a need for PromiseFlatCString?

(In reply to comment #13) > >@@ -949,22 +940,21 @@ nsHTMLDocument::StartDocumentLoad(const > >- if(cacheDescriptor) { > >+ if(cachingChan) { > > Maybe add space between if and ( ? Fixed > >+++ b/docshell/base/nsDocShell.cpp > >@@ -9485,29 +9485,27 @@ nsDocShell::AddToSessionHistory(nsIURI * > >+ nsCOMPtr<nsICachingChannel> cacheChannel; > > if (aChannel) { > >- nsCOMPtr<nsICachingChannel> > >- cacheChannel(do_QueryInterface(aChannel)); > >+ cacheChannel = do_QueryInterface(aChannel); > > Check indention here. Fixed > >+HttpChannelChild::SetCacheTokenCachedCharset(const nsACString &aCharset) > >+ if (!SendSetCacheTokenCachedCharset(PromiseFlatCString(aCharset))) { > > Is there really a need for PromiseFlatCString? Yes, SendSetCacheTokenCachedCharset() takes const nsCString&

Comment on attachment 446088 [details] [diff] [review] patch v3 Got +r from Honza, just need +sr from biesi.

Comment on attachment 446088 [details] [diff] [review] patch v3 +++ b/docshell/base/nsDocShell.cpp + if (expTime <= now) please fix the typo here (i.e. remove one of the two spaces after <=) +++ b/netwerk/base/public/nsICachingChannel.idl need a new IID +++ b/netwerk/protocol/http/src/HttpChannelChild.cpp +HttpChannelChild::GetCacheTokenExpirationTime(PRUint32 *_retval) +{ + NS_ENSURE_ARG_POINTER(_retval); don't bother checking out parameters for null. javascript can't pass null, and in the unlikely case that C++ code passes null, they can use a debugger. +++ b/netwerk/protocol/http/src/HttpChannelParent.cpp + if (mCacheDescriptor) + mCacheDescriptor->SetMetaDataElement("charset", + PromiseFlatCString(charset).get()); please put {} around a multiline if body + PRUint32 expirationTime; + chan->GetCacheTokenExpirationTime(&expirationTime); you ought to initialize expirationTime, the channel does not necessarily have a cache token

It seems one symptom of this bug is that pages with images load ok once, but reloading the page will load it without those same images.

I've unbitrotted the patch. This is mostly good, but there are a couple issues: 1) We need to implement IsFromCache(). I'll attach a patch I wrote that does so. With it we now see images loading correctly from cache. 2) I really don't like us having nsHttpChannelChild implement nsICachingChannel. It means that we have to implement a whole list of functions that we cannot provide sensible implementations for: GetCacheToken SetCacheToken GetOfflineCacheToken SetOfflineCacheToken GetCacheKey SetCacheKey GetCacheAsFile SetCacheAsFile GetCacheForOfflineUse SetCacheForOfflineUse GetOfflineCacheClientID SetOfflineCacheClientID GetCacheFile I think it would be much cleaner to either create a new interface ("nsICacheInfoChannel"?) that contains only IsFromCache (which we'd duplicate), GetCacheTokenExpirationTime and Set/GetCacheTokenCachedCharset (or add those functions to an existing interface--nsIHttpChannelInternal?), and remove nsICachingChannel support from HttpChannelChild. Biesi/Michal, any opinion on this? I'd be fine with putting them in nsIHttpChannelInternal unless there's a reason why that wouldn't work. (Perhaps we should put them in a separate nsICacheInfoChannel.idl, in case we ever support caching in a protocol other than HTTP?) 3) The nsHttpChannel.cpp implementations of the Get/SetCacheTokenXXX functions all return NS_ERROR_NOT_AVAILABLE if !mCacheEntry. I'd like us to be able to do something similar in the child. Perhaps we could set a bool flag in the child if mCacheEntry is set in the nsHttpChannel? If there's some other way to tell, like if we mark whether RecvOnStartReq has been called yet, that'd be fine too. 4) Do we have any idea how much of an I/O hit we'll incur from having SetCacheTokenCachedCharset update and dirty the cache meta-data at a possibly much later time than we do now? I suppose there's nothing we can do about it, since the logic to call it is on the child.

If this looks fine, roll it into the v4 patch and combine with the interface and error-check fixes, and we'll be good to go.

(In reply to comment #18) > 1) We need to implement IsFromCache(). I'll attach a patch I wrote that does > so. With it we now see images loading correctly from cache. It's in the new patch. > 2) I really don't like us having nsHttpChannelChild implement > nsICachingChannel. It means that we have to implement a whole list of > functions that we cannot provide sensible implementations for: > > GetCacheToken > SetCacheToken > GetOfflineCacheToken > SetOfflineCacheToken > GetCacheKey > SetCacheKey > GetCacheAsFile > SetCacheAsFile > GetCacheForOfflineUse > SetCacheForOfflineUse > GetOfflineCacheClientID > SetOfflineCacheClientID > GetCacheFile > > I think it would be much cleaner to either create a new interface > ("nsICacheInfoChannel"?) that contains only IsFromCache (which we'd duplicate), > GetCacheTokenExpirationTime and Set/GetCacheTokenCachedCharset (or add those > functions to an existing interface--nsIHttpChannelInternal?), and remove > nsICachingChannel support from HttpChannelChild. Biesi/Michal, any opinion on > this? I'd be fine with putting them in nsIHttpChannelInternal unless there's a > reason why that wouldn't work. (Perhaps we should put them in a separate > nsICacheInfoChannel.idl, in case we ever support caching in a protocol other > than HTTP?) I've created a new interface nsICacheInfoChannel and moved these methods from nsICachingChannel there. Interface nsICachingChannel inherits from nsICacheInfoChannel, so now nsHttpChannel implements nsICachingChannel and HttpChannelChild implements only nsICacheInfoChannel. Is this OK? > 3) The nsHttpChannel.cpp implementations of the Get/SetCacheTokenXXX functions > all return NS_ERROR_NOT_AVAILABLE if !mCacheEntry. I'd like us to be able to > do something similar in the child. Perhaps we could set a bool flag in the > child if mCacheEntry is set in the nsHttpChannel? If there's some other way to > tell, like if we mark whether RecvOnStartReq has been called yet, that'd be > fine too. Fixed. > 4) Do we have any idea how much of an I/O hit we'll incur from having > SetCacheTokenCachedCharset update and dirty the cache meta-data at a possibly > much later time than we do now? I suppose there's nothing we can do about it, > since the logic to call it is on the child. Hmm, I have no idea...

Good stuff, Michal! Having nsCachingChannel inherit from nsCacheInfoChannel is a good idea. I've fixed up the patch from bitrot. Will land tomorrow so I can baby-sit it.

This patch partially fixed some of bug 331032