Closed Bug 468040 Opened 16 years ago Closed 16 years ago

nsImapFolder's notifyDownloadedLines / m_downloadMessageForOfflineUse flag breaks when confronted with autosync and gloda

Categories

(MailNews Core :: Backend, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: asuth, Assigned: asuth)

Details

Attachments

(1 file)

It was noted that gloda interferes with the autosync mechanism.

In a nutshell, the m_downloadMessageForOfflineUse (which exists on nsImapMailFolder) flag controls whether m_offlineHeader gets populated.  If m_offlineHeader does not get populated, nsImapMailFolder::NormalEndMsgWriteStream will not call nsMSgDBFolder::EndNewOfflineMessage, who is the dude that calls MarkOffline and sets the various offsets and such.

There appear to be at least two problems here:

1) The auto-sync mechanism is never setting this flag.  nsImapMailFolder::DownloadMessagesForOffline calls SetNotifyDownloadedLines and then dispatches to nsImapService::DownloadMessagesForOffline.  The latter never calls SetNotifyDownloadedLines, and that (the service one) is what nsAutoSyncState::DownloadMessagesForOffline calls (not the folder one).

Since the folder one attempts to acquire the folder semaphore and autosync already has that one, autosync is using the right call.  The call to SetNotifyDownloadedLines either needs to migrate to the service call, or autosync state needs to set it to.  Given that the imap service clearly already knows about the flag (see the next problem), I'd suggest migrating it to the service.

2) The gloda streaming results in clearing this flag, even though I think streaming from offline should not actually interact with that datapath.  As long as nsImapService::StreamMessage has an imapMessageSink, it calls SetNotifyDownloadedLines with the result of the ShouldStoreMsgOffline.  In contrast, nsImapService::DisplayMessage only calls SetNotifyDownloadedLines "if (imapMessageSink && !hasMsgOffline)" (but using similar shouldStoreMsgOffline logic).

Given that we only have one open IMAP connection per folder and so can only perform one streaming operation *that is not from the offline store* at a time, it sounds like the flag is sane but the StreamMessage case should perform like the DisplayMessage case and know that the flag doesn't come into the picture for something that's already offline.

Incidentally, nsImapMailFolder::HeaderFetchCompleted was calling SetNotifyDownloadedLines(PR_TRUE) which should explain why the initial fetch of the N most recent messages or whatever in a folder generally succeeded.


Because I am so nice, I am providing a patch that does the stuff I say above.  I am okay if someone else wants to steal the patch, should it turn out to not be immediately correct... because I am just that nice.

I feel moderately confident this will not unleash any new horrors on the world, given that I instrumented a stupid number of things with printf and saw that the gloda and autosync streaming code paths were quite safely disparate.  Note that I said "new" horrors; whatever happened when two people tried to stream non-offline messages without doing the right thing semaphore-wise will still happen.
Flags: blocking-thunderbird3?
Attachment #351520 - Flags: superreview?(bienvenu)
Attachment #351520 - Flags: review?(bienvenu)
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Comment on attachment 351520 [details] [diff] [review]
v1 make autosync streaming set notify flag, stop gloda-style streaming from clearing it

this looks the right thing to do - I'll apply the patch and run with it for a bit.
Attachment #351520 - Flags: superreview?(bienvenu)
Attachment #351520 - Flags: superreview+
Attachment #351520 - Flags: review?(bienvenu)
Attachment #351520 - Flags: review+
Comment on attachment 351520 [details] [diff] [review]
v1 make autosync streaming set notify flag, stop gloda-style streaming from clearing it

I've tried this with the patch in  Bug 468155 as well...
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Keywords: checkin-needed
Patch checked in: https://hg.mozilla.org/comm-central/rev/1ba87088a1bb
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.