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

RESOLVED FIXED in Thunderbird 22.0


6 years ago
6 years ago


(Reporter: dlech, Assigned: dlech)


(Blocks: 2 bugs, {regression})

Thunderbird 22.0
Windows 7
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird21+ fixed, thunderbird-esr1721+ fixed)



(1 attachment, 1 obsolete attachment)



6 years ago
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

6 years ago
Created attachment 728694 [details] [diff] [review]
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.
Assignee: nobody → david
Attachment #728694 - Flags: review?(irving)
Blocks: 854161
No longer depends on: 854161
For general info, bug 816327 comment 18 has the critical explanation of why this is needed.
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).
Attachment #728694 - Flags: review?(irving) → review+
tracking-thunderbird21: --- → +
tracking-thunderbird-esr17: --- → 21+

Comment 4

6 years ago
Created attachment 730211 [details] [diff] [review]
patch v2

fixed nit
Attachment #728694 - Attachment is obsolete: true


6 years ago
Keywords: checkin-needed

Comment 5

6 years ago
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
Attachment #730211 - Flags: approval-comm-esr17?
Attachment #730211 - Flags: approval-comm-beta?
Attachment #730211 - Flags: approval-comm-aurora?
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0

Comment 7

6 years ago
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?
Blocks: 721316
Keywords: regression
Awesome work David. :-)
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.
Attachment #730211 - Flags: approval-comm-beta?
Attachment #730211 - Flags: approval-comm-beta-
Attachment #730211 - Flags: approval-comm-aurora?
Attachment #730211 - Flags: approval-comm-aurora+
Attachment #730211 - Flags: approval-comm-esr17? → approval-comm-esr17+
You need to log in before you can comment on or make changes to this bug.