854162 - Should use nsImapMailFolder::HasMsgOffline() instead of checking for nsMsgMessageFlag::Offline in nsAutoSyncState.cpp

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[David Lechner (:dlech)]

As stated in Bug 854161, nsMsgMessageFlag::Offline is broken in IMAP.

There are 2 uses in the autosync code that need to be fixed.

The breakage is causing the repeated downloading of messages such as those described in bug 816327.

Comment 1

[David Lechner (:dlech)]

Attachment: patch v1

by replacing the test for nsMsgMessageFlag::Offline with nsImapMailFolder::HasMsgOffline(), we don't change the functionality of autosync at all.

I have run all of the imap tests to verify that this does not break autosync. Also, I have manually verified that this patch fixes bug 816327.

Comment 2

[Mark Banner (:standard8)]

For general info, bug 816327 comment 18 has the critical explanation of why this is needed.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 728694 [details] [diff] [review]
patch v1

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

Stealing review from Irving as I was curious and started looking at this patch and to help him along a bit. I think this is fine for landing, though it does beg the question that if we've got a message where the body is stored in a different folder, shouldn't the offline bit be set?

That might actually be a bit difficult from a sync & perf perspective due to not knowing where the other message headers are that are the same id without going searching.

So I think this is fine to take for bug 816327 (and onto the ESR branch, as I assume this will largely fix that bug), and I think we might just need to reconsider what we do wrt nsMsgMessageFlag::Offline via bug 854161.

::: mailnews/imap/src/nsAutoSyncState.cpp
@@ +352,5 @@
>    {
> +    bool hasMessageOffline;
> +    folder->HasMsgOffline(mExistingHeadersQ[mProcessPointer], &hasMessageOffline);
> +    if (!hasMessageOffline)
> +       msgKeys.AppendElement(mExistingHeadersQ[mProcessPointer]);

nit: should be 2-space indent (it has 3).

Comment 4

[David Lechner (:dlech)]

Attachment: patch v2

fixed nit

Comment 5

[David Lechner (:dlech)]

Comment on attachment 730211 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #): 721316
User impact if declined: Poor performance for users of Gmail IMAP
Testing completed (on c-c, etc.): All IMAP tests on local Windows machine
Risk to taking this patch (and alternatives if risky):

See comment #3

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/0589b7ee5974

Comment 7

[:aceman]

Looks cool, if this solves the redownloading reported in other bugs. Can you close the referenced bugs like bug 816327 when this is tested by the reporters?

Comment 8

[Atul Jangra [:atuljangra]]

Awesome work David. :-)

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 730211 [details] [diff] [review]
patch v2

Ok, with the next merge happening on Monday, I think we should put this into Aurora so that it will be in central, aurora and beta for the next cycle. If it looks good from a user perspective, then we can put it into ESR for the cycle after.

Comment 10

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/421dd14083af

Comment 11

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr17/rev/f26d5d22997c