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)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b2
People
(Reporter: asuth, Assigned: asuth)
Details
Attachments
(1 file)
2.63 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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)
Updated•16 years ago
|
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Comment 1•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #351520 -
Flags: superreview?(bienvenu)
Attachment #351520 -
Flags: superreview+
Attachment #351520 -
Flags: review?(bienvenu)
Attachment #351520 -
Flags: review+
Comment 2•16 years ago
|
||
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 | ||
Updated•16 years ago
|
Assignee | ||
Comment 3•16 years ago
|
||
Patch checked in: https://hg.mozilla.org/comm-central/rev/1ba87088a1bb
You need to log in
before you can comment on or make changes to this bug.
Description
•