Remove image caching from mimemoz2.cpp

RESOLVED FIXED in Thunderbird 51.0

Status

MailNews Core
MIME
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

unspecified
Thunderbird 51.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

11 months ago
Created attachment 8779558 [details] [diff] [review]
Debug patch.

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?
Flags: needinfo?(acelists)
(Assignee)

Comment 1

11 months ago
Created attachment 8779576 [details] [diff] [review]
WIP: Debug patch (v2).

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 ...
Attachment #8779558 - Attachment is obsolete: true
Flags: needinfo?(acelists)

Comment 2

11 months ago
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?
(Assignee)

Comment 3

11 months ago
(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.
(Assignee)

Comment 4

11 months ago
Created attachment 8779655 [details] [diff] [review]
WIP: Debug patch (v3).

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.
Attachment #8779576 - Attachment is obsolete: true
(Assignee)

Comment 5

11 months ago
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).
(Assignee)

Comment 6

11 months ago
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.
(Assignee)

Comment 7

11 months ago
Created attachment 8779758 [details] [diff] [review]
Same patch as already reviewed in bug 1021843 + some additional removals.

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.
Attachment #8779758 - Flags: review?(mkmelin+mozilla)

Comment 8

11 months ago
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.
(Assignee)

Comment 9

11 months ago
(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.

Comment 10

11 months ago
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.
(Assignee)

Comment 11

11 months ago
(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 12

10 months ago
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
Attachment #8779758 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 13

10 months ago
https://hg.mozilla.org/comm-central/rev/90226acddf80
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
You need to log in before you can comment on or make changes to this bug.