609683 - double messages ending up in imap offline store

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[David :Bienvenu]

If you look at an imap, you'll sometimes see two copies of the same message. Only one is used, but the extra one takes up space, and increases bandwidth usage and server load. After investigation, it turns out that this is caused by an attempt to stream the message while autosync is downloading the message. The stream attempt gets queued, but at that point, we don't yet have the message in the offline store. By the time the queued url does get played, we *do* have the message in the offline store, but it's too late. I need to figure out a way to get playing queued urls to do the same kind of work that nsImapMockChannel::AsyncOpen does (we open the channel when we try to run the url initially, because necko requires that, iirc).

Comment 1

[David :Bienvenu]

er, look at an imap offline store, that is. I'm hoping that fixing this will make it easier to diagnose some of the other offline store issues I'm seeing.

Comment 2

[David :Bienvenu]

turns out imap auto sync isn't playing the game w.r.t the channel at all, which is not good.

Comment 3

[David :Bienvenu]

One option would be to simply not run the imap url off the queue if it's a request to download the message for offline use, and we already have the message offline (though we'd need to make sure repairing corrupt offline stores still works).

Comment 4

[David :Bienvenu]

Attachment: fix for 3.1

This is the 3.1 flavor of the fix, which doesn't change any idl. For the trunk, I could add methods to nsIImapMockChannel, though there's really no reason the imap mock channel and the imap protocol can't be friends, since that's a completely closed system.

One case this doesn't fix is when some code is streaming one of the messages, and then imap auto sync comes along and decides to download multiple messages at once, and one of those messages is the currently streaming message. Fixing that would involve mutating the UIDs we download for offline, which might make the code that started the offline download unhappy.

Writing a unit test for this is probably going to be a bit tricky.

Comment 5

[David :Bienvenu]

something's not quite right with the loadgroup/docshell in the case where we end up reading from the local cache in ::TryToRunUrlLocally - I get a few assertions...

Comment 6

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #4)
> One case this doesn't fix is when some code is streaming one of the messages,
> and then imap auto sync comes along and decides to download multiple messages
> at once, and one of those messages is the currently streaming message.

bug 587528, bug 604620, and bug 608529 seem such case, because they look fetch by mail click case followed by fetch for auto sync just after the mail click.
Right?

Comment 7

[David :Bienvenu]

Attachment: fix assertions and unit tests

this passes the unit tests, and fixes the assertions I was seeing. I've seen a problem with RetryUrl which I need to investigate...but I'll try to write a unit test for a few of the scenarios this fix deals with.

Comment 8

[David :Bienvenu]

Attachment: proposed fix with unit test

This adds a unit test. The basic fix is to check when running a queued offline download url to see if it really needs to run. Without the patch, we get two downloads into the offline store; w/o it, we get one.

Comment 9

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #8)
> The basic fix is to check when running a queued offline download url to see
> if it really needs to run.

David, is it applicable to fetch for "Junk Move" at Inbox of offline-use=off?
Double "uid fetch xxx body[]" was seen in log attached to bug 613353 comment #17. It's for "fetch by Junk Move" of small text/plain mail(perhaps whole mail data is saved in Memory Cache by first fetch body[]).

Comment 10

[David :Bienvenu]

(In reply to comment #9)
> (In reply to comment #8)

> 
> David, is it applicable to fetch for "Junk Move" at Inbox of offline-use=off?
> Double "uid fetch xxx body[]" was seen in log attached to bug 613353 comment
> #17. It's for "fetch by Junk Move" of small text/plain mail(perhaps whole mail
> data is saved in Memory Cache by first fetch body[]).

That's a slightly different case and one not handled by this patch. I would have thought that case would be handled by the combination of the url queue and the fact that we look at the mem/disk cache when we actually run the url.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 489188 [details] [diff] [review]
proposed fix with unit test

Full review with more context at: http://reviews.visophyte.org/r/489188/

Exported for posterity:

Looks good. Tested via the xpcshell-test as it seems hard to reproduce easily.
Lets get this in and see how it goes.


on file: mailnews/imap/src/nsImapProtocol.cpp line 2008
>   mailnewsUrl->GetFolder(getter_AddRefs(folder));

Should probably add a check that we do actually get a folder.


on file: mailnews/imap/src/nsImapProtocol.cpp line 2018
>     mailnewsUrl->SetMsgIsInLocalCache(useLocalCache);

Is it necessary to set MsgIsInLocalCache if we're not doing a download for
offline action?


on file: mailnews/imap/src/nsImapProtocol.cpp line 2025
>       folderSink->SetUrlState(this, mailnewsUrl, PR_TRUE, NS_OK);

Maybe add a comment along the lines of fake the fact that we've run the URL?


on file: mailnews/imap/test/unit/test_imapStoreMsgOffline.js line 272
>     // If DownloadMessagesForOffline took a listener, we wouldn't have to

We should file a bug on that.

Comment 12

[David :Bienvenu]

fixed on trunk - I'll file a bug on making DownloadMessagesForOffline taking a listener.

Comment 13

[Ludovic Hirlimann [:Usul]]

(In reply to comment #12)
> fixed on trunk - I'll file a bug on making DownloadMessagesForOffline taking a
> listener.

bug number ?

Comment 14

[David :Bienvenu]

(In reply to comment #13)

> 
> bug number ?

I can't know until the bug is filed :-) filed Bug 619062