As part of bug 1021843 I am proposing to remove image caching as per attachment 8778758 [details] [diff] [review]. This patch has r+ from Magnus as he said in bug 1021843 comment #45: Sure, r+ if it's working. It is in fact working: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=74db9b9aae20 As far as I can see, the code I am removing is dead code as I cannot get the code in mimemoz2.cpp to execute. I tried plain text messages with attached images and HTML messages with embedded images (attached to the message). I tried with all sorts of preferences: mail.imap.mime_parts_on_demand mail.imap.mime_parts_on_demand_threshold mail.server.default.mime_parts_on_demand I've tried with folders that had no local storage (although TB creates local storage and sometimes deletes it when TB closes) (Properties, Synchronization, "Select this folder for offline use"). The debug I'm seeing is always: === memCacheSession is NULL - NOT doing it!! So I'd like to shout a beer for the first person to give me a case that shows that this code is not dead. Aceman, do you drink beer?

Try this patch. I get: === bodystructure 3 - calling SetImageCacheSessionForUrl for imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E4 === SetImageCacheSessionForUrl - called SetImageCacheSession === GetImageCacheSession for imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E4 === memCacheSession is NOT null - doing it!! I get the beer. Silly me, I had browser.cache.use_new_backend_temp set to true (to use cache2 and see what it does), so the whole caching didn't work. So for folders which don't have local storage, the first time the message is visited, the images are cached. The code isn't dead. Now I have to see what happens with the image cache removed. To be continued ...

So do we want to remove the image cache? Or do you still propose the path that the full messages are cached anyway so you pull the images from them? In that case do you need to change the code in mimemoz2 to get the whole msg?

(In reply to :aceman from comment #2) > So do we want to remove the image cache? I want to remove the image cache, since so far, it's dead code (see following comment coming up) and it uses a function of "cache(1)" (blocking sync access) that is not available in "cache2". Any attempt to restructure MIME to make this async is highly dangerous, and can also not be tested, since I can't get the dead code to run in the first place. Magnus said in bug 1021843 comment #45: It's likely risky (compared to the benefit) to try to make it work in our current mime code. Joshua said on IRC: [2016-08-08 23:57:48] <jorgk> jcranmer: Anyway, cache2 doesn't have the sync facility that we would need for the image caching, so it's possibly safer to remove than to port ;-) [2016-08-08 23:58:02] <jcranmer> jorgk: I'm not going to disagree > Or do you still propose the path that the full messages are cached anyway so > you pull the images from them? Yes, of course, the idea is to cache the whole message an reprocess it in MIME. > In that case do you need to change the code > in mimemoz2 to get the whole msg? Without further looking into it, I'd say that it does this now if there is no image in the cache. Please read the next comment coming up.

No good working at 2 AM when you can't see the forest for the trees. So, the beer competition is still on, I didn't earn it. Sure, the cache is provided for the URL, see debug: === memCacheSession is NOT null - doing it!! but the essential debug that something is actually put into the cache is not coming out, see new patch and: === bodystructure 3 - calling SetImageCacheSessionForUrl for imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E4 === SetImageCacheSessionForUrl - called SetImageCacheSession === GetImageCacheSession for imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E4 === memCacheSession is NOT null - doing it!! === No cache entry for imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E4?part=1.2&type=image/jpeg&filename=IMG_C0030.jpg, so NOT DOING IT after all So a beer to anyone who can prove it's not dead.

Another thought: The comments around the area talk about "imglib" https://hg.mozilla.org/comm-central/annotate/296d8c85035498adcf926310f49e00f83fda03c6/mailnews/mime/src/mimemoz2.cpp#l1192 so I have the impression that code assumed something about how images are rendered in the M-C HTML renderer. That might have been so at Netscape 1.0, but I doubt that whatever the assumptions were, they hold true in today's HTML5 renderer especially given that M-C switched to "cache2" (nsHttpChannel.cpp).

Digging further: This code https://hg.mozilla.org/comm-central/annotate/296d8c85035498adcf926310f49e00f83fda03c6/mailnews/mime/src/mimemoz2.cpp#l1192 if from the CSV to Mercurial import. It has: nsCOMPtr<nsICacheEntryDescriptor> entry; From the comment I have the impression that it's trying to pre-cache images for the image loader here (first revision imported into Mercurial): https://hg.mozilla.org/mozilla-central/diff/9b2a99adc05e/modules/libpr0n/src/imgLoader.cpp#l1.314 That did indeed take a URL and return a cache entry. Note: nsCOMPtr<nsICacheEntryDescriptor> entry; where nsICacheEntryDescriptor is the old deprecated "cache(1)" interface, same as is still used in mimemoz2.cpp. The modern version https://dxr.mozilla.org/mozilla-central/rev/6cf0089510fad8deb866136f5b92bbced9498447/image/imgLoader.cpp#2116 has RefPtr<imgCacheEntry> entry; So there is no way that the code in mimemoz2.cpp will ever do anything useful any more. Most likely it *silently* stopped working when M-C redid their imgLoader.cpp and went from nsICacheEntryDescriptor to imgCacheEntry (imgLoader.h). So we can safely remove any remnants of the image cache from C-C right now and won't cause any difference in behaviour at all. I will conclude this investigation with looking at the load behaviour for images, that is, whether the message containing the image is at least cached. Coming up later.

This is the same patch as in bug 1021843. However, based on Joshua's comment in bug 1021843 comment #46 I noticed that SetAddToMemoryCache() is called a few times in the system, but there is no call to GetAddToMemoryCache(). There is also no use made of attribute addToMemoryCache: https://dxr.mozilla.org/comm-central/search?q=ddToMemoryCache&redirect=false Also, the member variable m_addContentToCache is not read other then in the unused Get function: https://dxr.mozilla.org/comm-central/search?q=m_addContentToCache So attribute addToMemoryCache (and friends) is another candidate for removal. I've noticed that HAVE_MIME_DATA_SLOT is never defined, so it and its friend LOCK_LAST_CACHED_MESSAGE can also go. That creates a clean slate for the migration to cache2 in bug 1021843.

Comment on attachment 8779758 [details] [diff] [review] Same patch as already reviewed in bug 1021843 + some additional removals. Review of attachment 8779758 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Please add "in IMAP" to the commit message:) So after this removal displaying images in non-offline msgs still works? The patch makes no replacement for the removed cache calls. So was the image cache just optional and already broken for a long time, as you say.

(In reply to :aceman from comment #8) > Please add "in IMAP" to the commit message:) I will. > So after this removal displaying images in non-offline msgs still works? No behaviour whatsoever changes due to this patch. It was all dead code, skilfully obscured so that even Kent fell for it an wanted to do a sync->async conversion for this part. Joshua also didn't notice that the call to SetAddToMemoryCache() he doubted in bug 1021843 comment #46 does in fact do absolutely nothing. > The > patch makes no replacement for the removed cache calls. So was the image > cache just optional and already broken for a long time, as you say. As I understand it, a long time ago, C-C knew the internals of the M-C image loader and attempted to load images parsed in MIME into the cache used by the other guy. That's fatal design (not so fatal when it was one program, Netscape Navigator) which was meant to break and as far as I can see broke silently since at some point we stopped writing to the other guy's cache since there were no entries to write to. Feel free to retrace my steps, it's well documented here.

just for total completeness, it might be useful to test with nntp. if you have a news.mozilla.com account, subscribe to mozilla.test.multimedia and see how the last two messages behave. one has an inline image and one has an image attachment.

(In reply to alta88 from comment #10) > just for total completeness, it might be useful to test with nntp. Thanks for the suggestion. It works fine, I tested with |set NSPR_LOG_MODULES=nntp:5,timestamp|. The articles get loaded once, on repeated visit they get loaded from the cache. I think I detailed sufficiently enough in comment #6 that since the M-C image loader today uses a different cache, MIME cannot pre-cache images as it (maybe) used to do once upon a time. Your suggestion is very useful for bug 1021843 where IMAP and NNTP cache(1) will be replaced by cache2.

Comment on attachment 8779758 [details] [diff] [review] Same patch as already reviewed in bug 1021843 + some additional removals. Review of attachment 8779758 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, r=mkmelin