Closed Bug 1021843 (mail-cache2) Opened 10 years ago Closed 8 years ago

Migrate mailnews to use cache2

Categories

(MailNews Core :: Networking, defect)

Product:
MailNews Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird48 affected, thunderbird49? affected)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 51.0
Tracking Flags:
Tracking Status
thunderbird48 --- affected
thunderbird49 ? affected

People

(Reporter: jcranmer, Assigned: jorgk-bmo)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 15 obsolete files)

Debug patch.
6.93 KB, patch
Details | Diff | Splinter Review
Proposed solution (v1e).
63.28 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
bugmail-3.0.xpi
33.50 KB, application/x-xpinstall
Details
bugmail-3.1.xpi
37.42 KB, application/x-xpinstall
Details 
Perma-orange. Almost certainly a regression of https://hg.mozilla.org/mozilla-central/rev/d405928cb934
Keywords: intermittent-failure
Depends on: 1022133
Summary: TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/mailnews/news/test/unit/test_bug540288.js | NS_ERROR_NOT_IMPLEMENTED: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsINntpService.cacheSession] → Migrate mailnews to use cache2
Blocks: 913828
To summarize some discussions that I had with Honza in IRC:

1) he will add a sync-usable method on nsICacheStorage like "bool entryExists(uri, eid)" that can be used in the sync calls to IsMsgInMemCache

2) LoadContextInfo is fairly simple, and it may be just easier to roll our own rather than to try to do a version accessible from external linkage from netwerk.

3) Naming of sessions like CreateSession("NNTP-memory-only" has no counterpart in cache2, and probably is not necessary for our purposes.

4) "if it's just a mem cache, then when an entry exists, you get the onCacheEntryAvailable call from inside asyncOpenURI" might allow us to avoid doing some sync->async conversions if we stick to memcache
Depends on: 1023413
Alias: mail-cache2
Blocks: cc-backlog
Blocks: 1066998
I suspect we are going to want this no later than version 45
Is this going to be an addon-compat issue for us like it was for Firefox?
Is there any reason we would *not* want to do this in time for esr version 45?

Also, we just got bit by a fix to the old cache code via bug 1170646.
Component: Networking: NNTP → Networking
OS: Linux → All
See Also: → 1204381, 1199957
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #19)
> Is this going to be an addon-compat issue for us like it was for Firefox?
> Is there any reason we would *not* want to do this in time for esr version
> 45?

It's not clear to me that this would result on issues with addons. However, the difficulty with it is that several methods that are currently sync would have to be modified to be async. Addon issues might arise as a result of these changes, but we won't know until we do the work.

Obviously we should do this as soon as possible. But there are other critical priorities as well, so I doubt this is going to get attention prior to TB 45.
(In reply to Kent James (:rkent) from comment #20)
> (In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment
> #19)
> > Is this going to be an addon-compat issue for us like it was for Firefox?
> > Is there any reason we would *not* want to do this in time for esr version
> > 45?
> 
> It's not clear to me that this would result on issues with addons. However,
> the difficulty with it is that several methods that are currently sync would
> have to be modified to be async. 

what about comment 17 where you write 
"1) he will add a sync-usable method on nsICacheStorage like "bool entryExists(uri, eid)" that can be used in the sync calls to IsMsgInMemCache""

Is that only applicable to places where we might want to *internally* have TB use sync?
Flags: needinfo?(rkent)
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #21)
> 
> Is that only applicable to places where we might want to *internally* have
> TB use sync?

I don't really understand what you are getting at.

Although it has been months since I looked at the code, what I recall is that cache is mostly used to keep a cache of messages when offline storage is not enabled (which is a non-default configuration). I really doubt if any addons manipulate that cache.
Flags: needinfo?(rkent)
Blocks: 1221758
Blocks: 1241622
(In reply to Honza Bambas (:mayhemer) from Bug 1241622 comment #2)
> Note that soon (this year) we will remove cache v1 code altogether.
Just disabled another test due to this. It would be good if someone more knowledgeable about mailnews/ took a look at migrating TB soon (for TB52), given comment 23.
Blocks: 1277354
Indeed, let's not wait til nightly becomes TB52. Such a fundamental change should have two full betas *before* the "release version" TB52 hits beta => we should target TB50 nightly at the latest.
status-thunderbird48: --- → affected
status-thunderbird49: --- → affected
tracking-thunderbird49: --- → ?
Just disabled more tests due to this, see bug 1288327.

Kent, sorry, can you give some more details, that is, expand on comment #17. Where is this cache stuff used in TB? Start looking somewhere around nsNntpService::GetCacheSession()
https://dxr.mozilla.org/comm-central/source/mailnews/news/src/nsNntpService.cpp#1753
and it's callers?

That's the only place we use NS_CACHESERVICE_CONTRACTID. This is the replacement, cache2, right:
"@mozilla.org/netwerk/cache-storage-service;1"
Flags: needinfo?(rkent)
I knew nothing about this cache stuff, but I looked at it for a few hours two years ago when this bug surfaced. What I recall from then is that the cache is used in IMAP to store downloaded messages (and probably also in News though that was not my focus). The difficult in adapting is that cache2 is async while the older cache is sync, so our code makes sync assumptions.

So I have no special knowledge or experience to offer here. I would guess that it is several day's work to do the code work for the migration.
Flags: needinfo?(rkent)
I'll take a look. It would be good to replace it before it gets killed and we're busted. We had enough notice.
Assignee: nobody → mozilla
I started looking at this. Following comment #17 the patch shows the rough shape of the solution. Note that NNTP also uses cache (nsNntpService::GetCacheSession, etc.).

TODO:
- Fully implement LoadContextInfo, see netwerk/base/LoadContextInfo.cpp
- nsMailboxService::IsMsgInMemCache and nsNntpService::IsMsgInMemCache
  to use nsICacheStorage::exists.
- replace calls to nsCacheSession::AsyncOpenCacheEntry with
  calls to nsICacheStorage::AsyncOpenURI and adapt callback.
- pref("browser.cache.use_new_backend_temp", false); - change to true.
Status: NEW → ASSIGNED
Just for the record: How does an entry actually get into the cache? Here is the mechanism.
nsCacheSession::AsyncOpenCacheEntry(old)/nsICacheStorage::AsyncOpenURI(new) takes a callback whose method OnCacheEntryAvailable is called regardless of whether the entry is in the cache or not, which is indicated to OnCacheEntryAvailable by a parameter. If NS_ERROR_CACHE_KEY_NOT_FOUND is passed, then the entry wasn't in the cache. If the cache was opened for read/write (old: nsICache::ACCESS_READ_WRITE, new: nsICacheStorage::OPEN_NORMALLY), the callback can fill the data into the cache, see for example:
https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#9044
https://dxr.mozilla.org/comm-central/source/mailnews/news/src/nsNNTPProtocol.cpp#861
I'll still have to find the spot where the data is actually put into the cache.
Digging through the code I found more problems:
The old cache allows sync access which seems to be used here:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.cpp#1173

This seems to be related to some "image" cache, note:
https://dxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgMailNewsUrl.idl#103
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.cpp#1165
https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#5079
which uses the same "cache session" as the "normal" IMAP cache.
Kent and Magnus, I've been looking at the image caching for a while now. I tried to see when/how/whether it works by watching it here:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.cpp#1166
memCacheSession always came back as null and that cache stuff was never run
(plain text message, a few JPG attachments, show attachments inline gets me there).

In order to not burden ourselves with porting this to cache2, I suggest to remove image caching. That means remove this bit in mimemoz2.cpp I pointed you at, and remove all the stuff here:
https://dxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgMailNewsUrl.idl#103
and all the get/set functions that go with it. The same goes for nsImapMailFolder::SetImageCacheSessionForUrl here:
https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#5079
and here:
https://dxr.mozilla.org/comm-central/source/mailnews/imap/public/nsIImapMessageSink.idl#66

Opinions?

If we don't worry about image caching (which I don't know how to trigger anyway), then the list of actions laid out in comment #29 should get us there.
Flags: needinfo?(rkent)
Flags: needinfo?(mkmelin+mozilla)
Attached patch Part 1: Removing image caching. (obsolete) — — Splinter Review 

        Attachment #8778758 -
        Flags: feedback?(rkent)

        Attachment #8778758 -
        Flags: feedback?(mkmelin+mozilla)

    
  

        Attachment #8778609 -
        Attachment description: WIP (v1). Compiles but doesn't link ;-) → Part 2: Convert async part - WIP (v1). Compiles but doesn't link ;-)

    
    

    
  
What "images" is it about? Images in messages? Or on websites browsed in TB?

    
    

    
  
(In reply to :aceman from comment #34)
> What "images" is it about? Images in messages? Or on websites browsed in TB?
Please read comment #32. Images in messages which are parsed in MIME code. However, I could never get the images to be cached. Test case:
Plain text message, a few JPG attachments, "show attachments inline" gets into mime_image_begin but the cache stuff isn't run. See:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeiimg.cpp#76
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimeiimg.cpp#123

Embedded images (as in: referenced from a HTML part) don't seem to trigger this code.

So as far as I can see, I'm removing dead code. Once our tests work again, I'll submit to try.

    
    

    
  
(In reply to Jorg K (GMT+2, PTO during summer) from comment #29)
> TODO:
> (snip)

In addition to conversion from cache to cache2, resolving bug such as bug 629738it is needed.
Memory/Disk Cache use by Thunderbird for IMAP mime-parts-on-demand is currentlly partially broken ;
  Disk Cache use works as expected only when "entire mail data" is fetched by mime-parts-on-demand upon mail disply.
  (mail data consists of text part only, or consists of text parts and displayable image parts only)

    
    

    
  
WADA: I found the IDL files, see my patch (attachment 8778609 [details] [diff] [review]), but thanks for this reference:
https://developer.mozilla.org/en-US/docs/Mozilla/HTTP_cache

    
    

    
  
(In reply to Jorg K (GMT+2, PTO during summer) from comment #30)
> I'll still have to find the spot where the data is actually put into the cache.
Found it, it's all in nsICacheEntry.idl (which replaces nsICacheEntryDescriptor.idl):
https://dxr.mozilla.org/comm-central/source/mozilla/netwerk/cache2/nsICacheEntry.idl#117
In fact, nsICacheEntryDescriptor had a method with the same name and we already call it here:
https://dxr.mozilla.org/comm-central/source/mailnews/news/src/nsNNTPProtocol.cpp#844
https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#9019

    
    

    
  
(In reply to Jorg K (GMT+2, PTO during summer) from comment #30)
> Just for the record: How does an entry actually get into the cache? 

Depends on how you want to open (or get) an entry.  All of the following applies strictly to cache2.

1) you want to create a new one (i.e. delete any existing and get an empty one to fill):
- use nsICacheEntry entry = nsICacheStorage.openTruncate(URL, ""), this way you delete any existing entry and get a new entry immediately
- then, nsIOutputStream outputStream = entry.openOutputStream(0)
- you write to that output stream and finally close it
- that's all
- you also can store metadata to the entry with entry.setMetaDataElement("key", "value") ; this is something that HTTP caching uses heavily, you can store various out-of-bound data about the content

2) you only want to read an entry if it exists in the cache:
- this is only and only done asynchronously (the main change between cache1 and cache2)
- implement class myOpenCallback : nsICacheEntryOpenCallback
- myOpenCallback.onCacheEntryCheck is called for existing entries and let's you decide if you want to use the entry or not (this is optimization mainly for how nsHttpChannel is using this API) ; let it return ENTRY_WANTED
- myOpenCallback.onCacheEntryAvailable is then called with either aResult == NS_OK and an entry you can read from, or aResult = NS_ERROR_XXX when it failed (you bail)
- then do: nsICacheStorage.asyncOpenURI(URL, "", OPEN_READONLY, new myOpenCallback)
- when this succeeds, you will be getting your callbacks

3) you want to either create a new one or use existing:
- opening is similar to (2) and working with the entry to (1)
- you use OPEN_NORMALLY flag
- when an entry doesn't exist in the cache you get a new one: myOpenCallback.onCacheEntryAvailable is called with aResult == NS_OK and aNew == true, aEntry is an empty entry to write to, same way as in the first case

That should be about it.  There is no need to call entry.close(), there is no notion of an entry being closed anymore.

Methods of nsICacheEntryOpenCallback are called on the same thread where asyncOpenURI has been called.

Also note, that there is no notion of "write access" in cache2.  You either get a new entry or an entry that is already filled, indicated by the aNew argument in onCacheEntryAvailable.  Everybody can write or read an entry at will ; this may sound dangerous, but it just makes a lot of things easier to implement and perform better.

And an advanced note: cache2 allows concurrent access - there can be one consumer that writes and others that read simultaneously.  The "readers" don't get the entry until the "writer" opens the output stream on the entry or calls metaDataReady() on the entry.  This is just for completeness, just in case you should hit this feature.

>Here is
> the mechanism.
> nsCacheSession::AsyncOpenCacheEntry(old)/nsICacheStorage::AsyncOpenURI(new)
> takes a callback whose method OnCacheEntryAvailable is called regardless of
> whether the entry is in the cache or not, which is indicated to
> OnCacheEntryAvailable by a parameter. If NS_ERROR_CACHE_KEY_NOT_FOUND is
> passed, then the entry wasn't in the cache. If the cache was opened for
> read/write (old: nsICache::ACCESS_READ_WRITE, new:
> nsICacheStorage::OPEN_NORMALLY), the callback can fill the data into the
> cache, see for example:
> https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.
> cpp#9044
> https://dxr.mozilla.org/comm-central/source/mailnews/news/src/nsNNTPProtocol.
> cpp#861

Please use annotated links.  When changes are made to these files, the reference quickly becomes completely useless.

I think you are hitting the "sync and blocking" way of cache1 and "async and non-blocking" way of cache2.  The code simply must be turned to async approach.

Also check comments in all cache2 .idl files, may help ;)


> I'll still have to find the spot where the data is actually put into the
> cache.

Search for entry.openOutputStream(0) spot.  That output stream is used to push data to the entry.  Closing the stream is the last operation that needs to be done.


(In reply to Jorg K (GMT+2, PTO during summer) from comment #31)
> Digging through the code I found more problems:
> The old cache allows sync access which seems to be used here:
> https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.
> cpp#1173
> 
> This seems to be related to some "image" cache, note:
> https://dxr.mozilla.org/comm-central/source/mailnews/base/public/
> nsIMsgMailNewsUrl.idl#103
> https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.
> cpp#1165
> https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/
> nsImapMailFolder.cpp#5079
> which uses the same "cache session" as the "normal" IMAP cache.

I think this has already been answered.  This all code also must be turned to async.

    
    

    
  
And to complete the docs, to actually read data from a cache entry:

Enhance on the case (2) from previous comment 40, for an existing entry:
- myOpenCallback.onCacheEntryCheck called, make it return ENTRY_WANTED
- myOpenCallback.onCacheEntryAvailable is then called with aResult == NS_OK and an entry you can read from
- do nsICacheStorage.asyncOpenURI(URL, "", OPEN_READONLY, new myOpenCallback)
- you _asynchronously_ (but in some cases it may happen before asyncOpenURI returns, make sure the code counts on it) get onCacheEntryAvailable call
- to read content from aEntry do: nsIInputStream inputStream = aEntry.openInputStream(0), then read the stream (it supports ReadSegments), don't forget to close it when you are done
- you can also access metadata on the entry with nsACString value = aEntry.getMetaDataElement("key")

    
    

    
  
Comment on attachment 8778758 [details] [diff] [review]
Part 1: Removing image caching.

I'm not shocked that it doesn't work atm. Ideally these mailnews urls should be cached but it's not a critical thing.
Flags: needinfo?(mkmelin+mozilla)

        Attachment #8778758 -
        Flags: feedback?(mkmelin+mozilla) → feedback+

    
    

    
  
(In reply to Magnus Melin from comment #42)
> I'm not shocked that it doesn't work atm. Ideally these mailnews urls should
> be cached but it's not a critical thing.
Can you clarify? This bug is about changing from "cache(1)" to "cache2". "cache2" is purely async, and our use of "cache(1)" is also mostly async apart from this MIME beauty which I can't get to execute.

So the messages will still be cached with the new scheme, but I propose to make our conversion effort smaller and ditch that image stuff. Are you prepared to turn the f+ into an r+ once I get a positive try run?

    
    

    
  
(In reply to Jorg K (GMT+2, PTO during summer) from comment #43)
> Can you clarify? This bug is about changing from "cache(1)" to "cache2".
> "cache2" is purely async, and our use of "cache(1)" is also mostly async
> apart from this MIME beauty which I can't get to execute.
> 
> So the messages will still be cached with the new scheme, but I propose to
> make our conversion effort smaller and ditch that image stuff. Are you
> prepared to turn the f+ into an r+ once I get a positive try run?

So it is rather a question if after the r+ there should be a new bug filed to get the image caching back. Or if we can let it remain unimplemented. Maybe jcranmer knows when it ever gets executed.
Flags: needinfo?(Pidgeot18)

    
    

    
  
Yes I understood you meant the images only. 
Sure, r+ if it's working. 

It's likely risky (compared to the benefit) to try to make it work in our current mime code. We could use a cache in a future js mime implementation if anyone cares enough to do the work.

    
    

    
  
Comment on attachment 8778758 [details] [diff] [review]
Part 1: Removing image caching.

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ -2645,5 @@
>                  m_runningUrl->SetStoreResultsOffline(false);
>                  nsCOMPtr<nsIMsgMailNewsUrl> mailurl = do_QueryInterface(m_runningUrl);
>                  if (mailurl)
>                  {
>                    mailurl->SetAddToMemoryCache(false);

I'm worried offhand about removing the below code but not this line of code, but I would need to see the results of actual experiments with IMAP code doing parts-on-demand and without an offline cache backing it up to figure out its actual effects.

    
    

    
  
Comment on attachment 8778758 [details] [diff] [review]
Part 1: Removing image caching.

Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=74db9b9aae20

As discussed on IRC and mentioned in comment #46, this needs more testing with
1) mail.server.default.mime_parts_on_demand set
2) where a folder doesn't have local storage:
   Properties, Synchronization, "Select this folder for offline use".

The idea is to ensure that the message gets cached properly and that the images are not downloaded every time. As Joshua put it:
<jcranmer> the main issue is whether or not you're going to regress from
           "cache images individually" to "absolutely no caching,
           redownload every time"
<jcranmer> as opposed to "cache the rfc822 bits and repass through libmime"
Flags: needinfo?(rkent)
Flags: needinfo?(Pidgeot18)

    
  
Depends on: 1293808

    
    

    
  
Comment on attachment 8778758 [details] [diff] [review]
Part 1: Removing image caching.

Upgrading Magnus' f+ to r+ since it worked, see comment #45.
Read analysis in bug 1293808 comment #6 to see that the C-C attempt to cache images worked together with an ancient version of M-C's imgLoader.cpp.
Since that M-C processing has changed, the C-C interface to it can just be removed.

        Attachment #8778758 -
        Flags: feedback?(rkent) → review+

    
    

    
  
Comment on attachment 8778758 [details] [diff] [review]
Part 1: Removing image caching.

This will land in bug 1293808.

        Attachment #8778758 -
        Attachment is obsolete: true

    
    

    
  


  
This shows the extent of the problem. 12x "FIX THIS" and 2x "IMPLEMENT THIS" are left to do.

        Attachment #8778609 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP (v3). Compiles and links ;-) (obsolete)
        — 
          — Splinter Review
      

    


  
Not working, down to 10x "FIX THIS".

        Attachment #8782160 -
        Attachment is obsolete: true

    
    

    
  


  
8x "FIX THIS". In reality it's half that, since IMAP and NNTP have the same "FIX THIS" to be solved ;-)

        Attachment #8785622 -
        Attachment is obsolete: true

    
    

    
  


  
With this patch, news articles display and IMAP messages stored in folders *without* local storage (folder properties, Synchronization, "Select this folder for offline use" unchecked) also display. Note that folders with local storage don't use caching. 7x "FIX THIS" left (down 2, up one), so we're getting there.

        Attachment #8785629 -
        Attachment is obsolete: true

    
    

    
  


  
NNTP seems to work 100% now. First time the message is written to the cache, subsequent displays fetch it from the cache. Still need to look at IsMsgInMemCache().

IMAP needs a little more work.

Down to 3x "FIX THIS".

        Attachment #8785676 -
        Attachment is obsolete: true

    
    

    
  


  
Caching works for NNTP.

Caching works for (some?) single part IMAP messages. Other messages get repeatedly fetched from the server despite being cached, well, only partly, read on.

nsImapMockChannel::ReadFromMemCache() decides not to use the cached version due to various reasons, here some debug:
=== nsImapMockChannel::OnCacheEntryAvailable: succeeded
=== nsImapMockChannel::OnCacheEntryAvailable: size=47411
=== nsImapMockChannel::OnCacheEntryAvailable: READ
=== annotation: Modified View Inline, forcing back to use cache
=== message 371414, cache 47411, forcing back to use cache
=== nsImapMockChannel::OnCacheEntryAvailable: read status 0

So the first reason is that the message has metadata ContentModified=="Modified View Inline" instead of expected "Not Modified". The second reason is that the cached data is much shorter than the message size. Most likely a heavy attachment didn't get cached. After forcing it back twice, the message is display OK, but seems to lack the attachment. Needs investigation.

Also still problematic: IsMsgInMemCache() (3x "FIX THIS").

        Attachment #8785685 -
        Attachment is obsolete: true

    
    

    
  
Looking further into it, the IMAP caching works better than I thought. 
At some stage the size of another part is used to check the size:
=== message 371414, cache 47411, forcing back to use cache
371414 is the size of the second part, which erroneously impedes that what was perfectly cached for the first part being used. When double clicking onto the attachment with mail.imap.mime_parts_on_demand turned off, both parts get read correctly from the cache.
As Wada pointed out in comment #37, I've run into bug 629738 and cutting over to the new cache hasn't fix that ;-)

    
    

    
  


  
Restored stripping of part after the ? in NNTP URI. Need to fix this for IMAP, too.

        Attachment #8785722 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP (v8). NNTP and IMAP working (obsolete)
        — 
          — Splinter Review
      

    


  
IMAP working as well as before, with problems not using the cache at times, when it should. Doesn't happen with all multipart messages, but with some. Seems to be bug 629738, and as per comment #55 various things lead to the rejection of the cache entry.

        Attachment #8785757 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP (v9). NNTP and IMAP working. (obsolete)
        — 
          — Splinter Review
      

    


  
(patch only differs in debug from v8, see interdiff.)

Almost green try run with this patch:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3cf3d19bd5e0d2000eeddb9f81c1b733afe7e5b2

Only one Xpcshell test failure in mailnews/news/test/unit/test_bug540288.js.
This test assumes access to the old cache via:
    MailServices.nntp
                .cacheSession
                .asyncOpenCacheEntry(kCacheKey, Ci.nsICache.ACCESS_WRITE, {
This needs to be fixed.

        Attachment #8785764 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Proposed solution (v1a). (obsolete)
        — 
          — Splinter Review
      

    


  
This is the same as WIP (v9) but
- test test_bug540288.js fixed.
- debug print removed.
- impossible code path removed.

Try on all platforms:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cec8d0366484

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Proposed solution (v1b). (obsolete)
        — 
          — Splinter Review
      

    


  
Restored truncation after ? for NNTP URL's (had gotten lost).

        Attachment #8785949 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Proposed solution (v1c). (obsolete)
        — 
          — Splinter Review
      

    


  
(eliminated trailing space and unused variable.)

Full try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bcf069ae370e

        Attachment #8785970 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Proposed solution (v1d). (obsolete)
        — 
          — Splinter Review
      

    


  
Fixed Mac compile error (unused variable).
Noticed that OnCacheEntryAvailable() passes in 'aNew' to indicate whether it's a new empty cache entry. So no need to get size.

New Mac try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5d79c8d4a2b3

        Attachment #8785974 -
        Attachment is obsolete: true

    
  
Blocks: 629738

    
    

    
  
No luck today. Windows failed right at the end:
Failure to install packages
Makefile:29: recipe for target 'mozmill-virtualenv' failed

Trying again:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=078d527a85ce

    
    

    
  
Comment on attachment 8785994 [details] [diff] [review]
Proposed solution (v1d).

The tries have been green enough to ask for review.
Note: There seems to be a Linux x64 debug failure in test-message-commands-on-msgstore.js but I ran that test locally on a Windows debug build and it was fine:
mozmake SOLO_TEST=folder-display/test-message-commands-on-msgstore.js mozmill-one

===

Now the notes for the reviewer:

This is a pretty much substitution of the following:
nsICacheEntryDescriptor --> nsICacheEntry
nsICacheSession         --> nsICacheStorage
nsICacheListener        --> nsICacheEntryOpenCallback
AsyncOpenCacheEntry()   --> AsyncOpenURI()
Doom()                  --> AsyncDoom()

The LoadContextInfo stuff was copied from netwerk/base.

Other changes:
- Cache keys are no longer strings but nsIURIs which are manipulated differently.
- Cache entries are now always read/write.
- To check whether an entry is in the cache, use the "aNew" parameter.
  One could also check the data length.
- IsMsgInMemCache() lost an argument and calls using this argument were removed.
  In my try run
  https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3cf3d19bd5e0d2000eeddb9f81c1b733afe7e5b2
  I had debug code to notify of this case, but apparently this code wasn't run:

  printf ("=== nsImapMailFolder::FetchMsgPreviewText: resetting %d to 0\n", msgInMemCache?1:0);
  printf ("=== nsImapService::StreamHeaders: resetting %d to 0\n", msgInMemCache?1:0);
  printf ("=== nsNntpService::StreamHeaders: resetting %d to 0\n", msgInMemCache?1:0);
  msgInMemCache = false; // FIX THIS: IsMsgInMemCache does no longer return the entry for sync use.

  StreamHeaders() is actually not used in C++ code, only in tests:
  https://dxr.mozilla.org/comm-central/search?q=treamHeaders&redirect=false
  FetchMsgPreviewText() is only used twice (plus in tests), so a little performance decrease won't hurt there:
  https://dxr.mozilla.org/comm-central/search?q=etchMsgPreviewText&redirect=false

Surprisingly it all still works as well as before. I considered addressing bug 629738, but that confuses the task in this bug.

Testing:

I tested NNTP with news.mozilla.com / mozilla.test.multimedia. There are some multipart messages with images in there.
I tested IMAP with a folder without local storage and messages with attached and attached/embedded images. When I tested with a message with a PDF attachment, I ran into bug 629738, which I'll address next.

I thought you could test with WIP (v9), but since a few things have changed since that revision, so I will attach a debug patch in the next hour to be applied on top, so you can see what's going on.

        Attachment #8785994 -
        Flags: review?(mkmelin+mozilla)

    
    

    
  
Comment on attachment 8785994 [details] [diff] [review]
Proposed solution (v1d).

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

I could annotate this more, but I think you get the idea ;-)

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ -9351,5 @@
>      NS_ENSURE_SUCCESS(rv,rv);
>  
> -    nsCOMPtr<nsICacheEntryDescriptor> cacheEntry;
> -    bool msgInMemCache = false;
> -    rv = msgService->IsMsgInMemCache(url, this, getter_AddRefs(cacheEntry), &msgInMemCache);

This was removed since IsMsgInMemCache() can no longer return the cache entry for sync use.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +9070,5 @@
>    // Open a cache entry with key = url
> +  nsCOMPtr <nsIURI> newUri;
> +  m_url->Clone(getter_AddRefs(newUri));
> +  nsAutoCString path;
> +  newUri->GetPath(path);

Changed manipulation of the URI here and elsewhere in NNTP.

::: mailnews/imap/src/nsImapService.cpp
@@ -1244,5 @@
> -    NS_ENSURE_SUCCESS(rv, rv);
> -    nsCOMPtr<nsIURI> url(do_QueryInterface(imapUrl));
> -    nsCOMPtr<nsICacheEntryDescriptor> cacheEntry;
> -    bool msgInMemCache = false;
> -    rv = IsMsgInMemCache(url, folder, getter_AddRefs(cacheEntry), &msgInMemCache);

This was removed since IsMsgInMemCache() can no longer return the cache entry for sync use.

::: mailnews/news/src/nsNntpService.cpp
@@ -1500,5 @@
> -                        action, getter_AddRefs(url));
> -  NS_ENSURE_SUCCESS(rv, rv);
> -  nsCOMPtr<nsICacheEntryDescriptor> cacheEntry;
> -  bool msgInMemCache = false;
> -  rv = IsMsgInMemCache(url, folder, getter_AddRefs(cacheEntry), &msgInMemCache);

This was removed since IsMsgInMemCache() can no longer return the cache entry for sync use.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Debug patch.
        — 
          — Splinter Review
      

    


  
With this patch you get to see what's going on.
You need to have news and IMAP folders set up *without* local storage.

Example news:
First visit:
=== cacheStorage->AsyncOpenURI NNTP
=== |news://news.mozilla.com:119/mailman.239.1465106208.17099.test-multimedia%40lists.mozilla.org|
=== nsNNTPProtocol::OnCacheEntryAvailable: WRITE
=== nsNNTPProtocol::OnCacheEntryAvailable: WRITE END
Second visit:
=== cacheStorage->AsyncOpenURI NNTP
=== |news://news.mozilla.com:119/mailman.239.1465106208.17099.test-multimedia%40lists.mozilla.org|
=== nsNNTPProtocol::OnCacheEntryAvailable: READ, size=8523

Example IMAP:
1) Simple message:
First visit:
=== cacheStorage->AsyncOpenURI IMAP, part (0)
=== |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E3|
=== nsImapMockChannel::OnCacheEntryAvailable: WRITE
=== nsImapMockChannel::OnCacheEntryAvailable: WRITE END
Second visit:
=== cacheStorage->AsyncOpenURI IMAP, part (0)
=== |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E3|
=== nsImapMockChannel::OnCacheEntryAvailable: READ, size=1288

2) Multipart with images:
First visit:
=== cacheStorage->AsyncOpenURI IMAP, part (0)
=== |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E2|
=== nsImapMockChannel::OnCacheEntryAvailable: WRITE
=== nsImapMockChannel::OnCacheEntryAvailable: WRITE END
Second visit:
=== cacheStorage->AsyncOpenURI IMAP, part (1)
=== |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E2?part=1.2&filename=aeeejaapagoehjok.jpg|
=== nsImapMockChannel::OnCacheEntryAvailable - reading part, not found
=== cacheStorage->AsyncOpenURI IMAP, part (0)
=== |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E2|
=== nsImapMockChannel::OnCacheEntryAvailable: READ, size=134481
=== cacheStorage->AsyncOpenURI IMAP, part (1)
=== |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E2?part=1.3&filename=dcaambcjgldehddn.jpg|
=== nsImapMockChannel::OnCacheEntryAvailable - reading part, not found
=== cacheStorage->AsyncOpenURI IMAP, part (0)
=== |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E2|
=== nsImapMockChannel::OnCacheEntryAvailable: READ, size=134481

3) Failing example with PDF attachment:
First visit:
=== cacheStorage->AsyncOpenURI IMAP, part (0)
=== |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E9|
=== nsImapMockChannel::OnCacheEntryAvailable: WRITE
=== nsImapMockChannel::OnCacheEntryAvailable: WRITE END
Second visit:
=== cacheStorage->AsyncOpenURI IMAP, part (0)
=== |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.AAA%20Test%20no%20local%20storage%3E9|
=== nsImapMockChannel::OnCacheEntryAvailable: READ, size=46890
=== nsImapMockChannel::OnCacheEntryAvailable: READ not successful
(here the cache entry is rejected, see comment #55).

Enjoy!

        Attachment #8785939 -
        Attachment is obsolete: true

    
    

    
  
I forgot to say:
2) Multipart with attached images, attachments viewed inline. Otherwise bug 629738 strikes big time (see bug 629738 comment #16).
2a) Multipart with HTML-embedded (and attached) images works as well.

    
    

    
  
Comment on attachment 8785994 [details] [diff] [review]
Proposed solution (v1d).

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

Can't say I've read it in much detail, but I think you know what you're doing and nothing major jumps out
r=mkmelin with the below addressed

::: mail/app/profile/all-thunderbird.js
@@ +863,1 @@
>  pref("browser.cache.use_new_backend",       0);

hmm, this seems like a dead pref(?)

@@ +863,2 @@
>  pref("browser.cache.use_new_backend",       0);
> +pref("browser.cache.use_new_backend_temp",  true);

we can just remove it as true is default

::: mailnews/base/src/MailnewsLoadContextInfo.cpp
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// This was copied from M-C netwerk/base.

anywhere specific? and please remove the "M-C"

::: mailnews/base/src/MailnewsLoadContextInfo.h
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// This was copied from M-C netwerk/base.

same here

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +9364,3 @@
>          rv = GetMsgPreviewTextFromStream(msgHdr, inputStream);
> +    }
> +    else if (!aLocalOnly)

please add braces

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +9095,5 @@
>          && !anchor.EqualsLiteral("?header=attach") && !anchor.EqualsLiteral("?header=src"))
>          mTryingToReadPart = true;
>        else
> +        path.SetLength(anchorIndex);
> +        newUri->SetPath(path);

looks wrong, you really do need braces here if the else is two lines!

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +872,4 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Open a cache entry with key = url, no extension.
> +  nsCOMPtr <nsIURI> uri;

nsCOMPtr<nsIURI>

same below

        Attachment #8785994 -
        Flags: review?(mkmelin+mozilla) → review+

    
    

    
  
(In reply to Magnus Melin from comment #69)
> > +pref("browser.cache.use_new_backend_temp",  true);
> we can just remove it as true is default
OK. pref("browser.cache.use_new_backend", 0); can go, too, right?

> > +// This was copied from M-C netwerk/base.
> anywhere specific? and please remove the "M-C"
Yes, as I said netwerk/base, netwerk being a top directory.

> >        else
> > +        path.SetLength(anchorIndex);
> > +        newUri->SetPath(path);
> 
> looks wrong, you really do need braces here if the else is two lines!
Ouch!!

    
    

    
  
Now I got it:
This was copied from netwerk/base/LoadContextInfo.cpp/h ;-)

    
    

    
  


  
Fixed review issues. Thanks for the quick review!

        Attachment #8785994 -
        Attachment is obsolete: true

        Attachment #8786487 -
        Flags: review+

    
    

    
  
https://hg.mozilla.org/comm-central/rev/c679c89e86ba
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0

    
    

    
  
Great to see this fixed - thanks for tackling it!

    
    

    
  
Yep, great work Jörg!
Hardware: x86_64 → All

    
    

    
  
(In reply to Jorg K (GMT+2, PTO during summer) from comment #65)
> Note: There seems to be a Linux x64 debug failure in
> test-message-commands-on-msgstore.js but I ran that test locally on a
> Windows debug build and it was fine:
> mozmake SOLO_TEST=folder-display/test-message-commands-on-msgstore.js
> mozmill-one

*Not* seen here
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=c679c89e86ba8617f840c9defa89a304a17f86ce
so we're cool ;-)

    
    

    
  
Thanks!  Another nail to the old cache code coffin.

    
    

    
  
Shows up in the tests now (Mac OSX) also after bug 1299116 was fixed:

> TEST-UNEXPECTED-FAIL | mailnews/news/test/unit/test_bug540288.js | xpcshell return code: 0
> TEST-UNEXPECTED-FAIL | mailnews/news/test/unit/test_bug540288.js | streamListener.onStopRequest - [streamListener.onStopRequest : 23] 2152398861 == 0
> TEST-UNEXPECTED-FAIL | mailnews/news/test/unit/test_bug540288.js | streamListener.onStopRequest - [streamListener.onStopRequest : 28] "" == "From: John Doe <john.doe@example.com>\\nDate: Sat, 24 Mar 1990 10:59:24 -0500\\nNewsgroups: test.subscribe.simple\\nSubject: H2G2

    
    

    
  
Hmm, right. I fixed test_bug540288.js since it failed completely. Let's open a new bug for this. I don't have a Mac, so I don't know how much I can contribute to fixing that.

    
  
Depends on: 1299557

    
    

    
  
I don't have a Mac either, and I don't see this on either Windows or Linux.
Filed bug 1299557 as a follow-up to look into this.

    
    

    
  
(In reply to rsx11m from comment #78)
> Shows up in the tests now (Mac OSX) also after bug 1299116 was fixed:
Actually, you confused me now.

When I pushed this fix here:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=c679c89e86ba8617f840c9defa89a304a17f86ce&selectedJob=45144
it does *not* show up on OS X 10.10 opt.

After bug 1299116 landed
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=e5fd1b18853f50d5340a585cc86364d602eb9499&selectedJob=45190
it does show up.

So maybe it's intermittent, I've raised bug 1299562.

    
  
Depends on: 1299562
No longer depends on: 1299557

    
    

    
  
With the push of the patch here, not that for https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=c679c89e86ba8617f840c9defa89a304a17f86ce&selectedJob=45160 it doesn't show up for the first [X] but for the second [X], thus agreed = intermittent unless something changed in m-c between those.

    
    

    
  
nicely done.

word of note, for those that dearly like the bugmail addon, cache2 breaks it.  And it's not actively maintained

    
    

    
  
Wayne: According to bug 1241622 comment #2, core support for the old cache implementation will be removed "soon (this year)" and thus there isn't much choice other than moving to cache2 for everybody and dealing with the fallout... (unless you want to fork the old code).

    
    

    
  
I literally mean "nicely done" work to everyone involved. I know cache2 had to happen, and wanted it to happen before 52, so it's OK, I'm not complaining. Just putting the word out there. And I'm sure other breakages will happen as well.

    
    

    
  
Oh, ok. I need to work on my communications skills. :-)

    
    

    
  
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #83)
> for those that dearly like the bugmail addon, cache2 breaks
> it.  And it's not actively maintained.
I looked at it. A few hours work on a rainy afternoon to convert it to cache2. Check:
https://hg.mozilla.org/comm-central/rev/c679c89e86ba#l23.8

(In reply to rsx11m from comment #84)
> Wayne: According to bug 1241622 comment #2, core support for the old cache
> implementation will be removed "soon (this year)" ...
We don't know when it will be removed exactly, I heard that "appcache" (https://html.spec.whatwg.org/multipage/browsers.html#offline) still relies on it.

    
    

    
  
https://hg.mozilla.org/comm-central/rev/2356b665560a
Follow up: Comment correction/additions. rs=documentation-only

    
    

    
  

      
      
      
      

        Attached file
          
        bugmail-3.0.xpi
        — 
      

    


  
(In reply to Jorg K (GMT+2) from comment #87)
> I looked at it. A few hours work on a rainy afternoon to convert it to
> cache2.
Done.

    
    

    
  

      
      
      
      

        Attached file
          
        bugmail-3.1.xpi
        — 
      

    


  
OK, this version works from TB 45 onwards, 3.0 only worked from TB 52.

    
  
Depends on: 1320551

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: