Last Comment Bug 854162 - Should use nsImapMailFolder::HasMsgOffline() instead of checking for nsMsgMessageFlag::Offline in nsAutoSyncState.cpp
: Should use nsImapMailFolder::HasMsgOffline() instead of checking for nsMsgMes...
: regression
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: x86_64 Windows 7
-- normal (vote)
: Thunderbird 22.0
Assigned To: David Lechner (:dlech)
Depends on:
Blocks: 816327 854161 721316
  Show dependency treegraph
Reported: 2013-03-23 21:23 PDT by David Lechner (:dlech)
Modified: 2013-05-01 03:21 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch v1 (2.50 KB, patch)
2013-03-23 21:33 PDT, David Lechner (:dlech)
standard8: review+
Details | Diff | Splinter Review
patch v2 (2.50 KB, patch)
2013-03-27 10:04 PDT, David Lechner (:dlech)
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta-
standard8: approval‑comm‑esr17+
Details | Diff | Splinter Review

Description User image David Lechner (:dlech) 2013-03-23 21:23:10 PDT
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 User image David Lechner (:dlech) 2013-03-23 21:33:31 PDT
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.
Comment 2 User image Mark Banner (:standard8) 2013-03-27 03:32:55 PDT
For general info, bug 816327 comment 18 has the critical explanation of why this is needed.
Comment 3 User image Mark Banner (:standard8) 2013-03-27 03:50:28 PDT
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 User image David Lechner (:dlech) 2013-03-27 10:04:32 PDT
Created attachment 730211 [details] [diff] [review]
patch v2

fixed nit
Comment 5 User image David Lechner (:dlech) 2013-03-27 10:09:47 PDT
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 User image Ryan VanderMeulen [:RyanVM] 2013-03-27 13:16:16 PDT
Comment 7 User image :aceman 2013-03-28 00:51:32 PDT
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 User image Atul Jangra [:atuljangra] 2013-03-28 02:34:24 PDT
Awesome work David. :-)
Comment 9 User image Mark Banner (:standard8) 2013-03-28 04:45:08 PDT
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 User image Mark Banner (:standard8) 2013-03-28 05:58:23 PDT
Comment 11 User image Mark Banner (:standard8) 2013-05-01 03:21:26 PDT

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