Ursula Sarracini (:ursula) Reporter
	Description • 7 years ago

Bug 1373258 converted moz-page-thumbs protocol to C++ as a first step to completing Bug 1184701, and in the process had to add some special logic in imgLoader.cpp to check if the image we're loading is a thumbnail with that protocol, and if so, load it directly from the cache instead of decoding it again. We should implement some proper caching in nsICachingChannel for the thumbnail channel instead of adding this special case logic to imgLoader.cpp

	Honza Bambas (:mayhemer)
	Comment 1 • 7 years ago

I don't think personally this sounds reasonable ("implement some proper caching in nsICachingChannel"), but please give me a reference to the special code you've added to imgLoader so we can find what could be moved or how else to implement this.  Note that only nsHttpChannel implements nsICachingChannel (AFAIK) and it's definitely not involved in loads of moz-page-thumbs URI content.  Hence, I don't think what you suggests make sense, but I understand the concern.  I'll try to help here.

	Ursula Sarracini (:ursula) Reporter
	Comment 2 • 7 years ago

The code hasn't landed yet, but basically in imgLoader.cpp in LoadImage, after we set the request flags here: https://dxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#2197, I do an extra check to see if aURI's scheme is "moz-page-thumbs" and if so, add nsIRequest::LOAD_FROM_CACHE to the request flags. And then also again here: https://dxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#2473 in LoadImageWithChannel. Actually, Andrew might be a better person to ask about the reasoning behind this change.

I'm happy to provide more context about Bug 1373258 and Bug 1184701 to find an alternative solution for this if needed. Thanks for looking into this Honza!

	Andrew Osmond [:aosmond] (he/him)
	Comment 3 • 7 years ago

Imagelib needs to know if a given resource has changed to decide if we can reuse our cache, or if we need to refetch the data. Without support for this, we need to fetch/decode the resource anew each time imgLoader::LoadImage(WithChannel) is called. Obviously this is something we want to avoid if we don't have to do it :). Decoding isn't free, and causes flickering due to redrawing, etc. This is done via two means:

1) If the sourcing channel for the URI implements nsICacheInfoChannel (which in turn implies nsICachingChannel for nsHttpChannel), we can use that, see:
http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/image/imgLoader.cpp#2885

2) We have an exception for file URIs, where we check the file modified date:
http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/image/imgLoader.cpp#1850

At present, we get a SubstitutingURL from the caller for file-backed URIs such as moz-page-thumbs, etc. This way we are able to get the underlying file and check its modified time, even though the channel itself doesn't support any caching functionality.

Going forward, my understanding is that we can't rely upon this. The content process won't be using file URIs at all because we will lose access to the file system. So option 2 + SubstituingURL isn't going to be a workable solution.

In order to get this working today, I suggested ursula add an exception for moz-page-thumb URIs where we always accept the cache entry if it is available. This is fine for now because the thumbnails probably won't change much anyways, and are not expected to be always identical to the content they preview.

However this isn't the preferable solution (to me, in any event :)). The channels/URI combinations that need caching should actually support nsICachingChannel/nsICacheInfoChannel so everything "just works." This will probably involve IPC calls to the parent to check the modified date on our behalf, etc. I'm not sure how it works for HTTP today, but I can only imagine it will look similar. If there needs to be additional work on the consumer side (e.g. imagelib) to support such a mechanism, I'm more than happy to lend a hand.

	Honza Bambas (:mayhemer)
	Comment 4 • 7 years ago

(In reply to Ursula Sarracini (:ursula) from comment #2)
> The code hasn't landed yet, but basically in imgLoader.cpp in LoadImage,
> after we set the request flags here:
> https://dxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#2197, I
> do an extra check to see if aURI's scheme is "moz-page-thumbs" and if so,
> add nsIRequest::LOAD_FROM_CACHE to the request flags. And then also again
> here:
> https://dxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#2473 in
> LoadImageWithChannel. Actually, Andrew might be a better person to ask about
> the reasoning behind this change.


perma links for ref (please use them next time):

https://dxr.mozilla.org/mozilla-central/rev/19b32a138d08f73961df878a29de6f0aad441683/image/imgLoader.cpp#2197

https://dxr.mozilla.org/mozilla-central/rev/19b32a138d08f73961df878a29de6f0aad441683/image/imgLoader.cpp#2473



(In reply to Andrew Osmond [:aosmond] from comment #3)
> I'm not sure how it works for HTTP today, 

The http channel parent side send down the info if it was loaded from cache or not (we know it at the moment we send down onstartrequest).  The information on the child side is read-only, that is the reason why there is nsICacheInfoChannel and nsICachingChannel (derived from that first interface).  nsICacheInfoChannel only provides some information that is known on both parent and child, while nsICachingChannel can also provide some control over how the cache is handled (you don't need that to fix this bug).


I haven't looked at how moz-page-thumbs IPC split is going to be implemented, but if the child side implements nsICacheInfoChannel you can just always return true as a start.


(There is unfortunately no digest-like or unique-id-like sync between the parent cache (being it the http cache or a thumb "cache") and the image loader cache.  The channel can easily say "I'm from cache on the parent" - suggesting imglib to use its decoded cached data - but that decoded content in the image loader cache can easily be from a different data than what's now being served by the channel.)

	Andrew Osmond [:aosmond] (he/him)
	Comment 5 • 7 years ago

I'm looking at what can be done here, as it doesn't just affect moz-page-thumb (moz-extension, etc).