Closed Bug 167382 Opened 22 years ago Closed 22 years ago

IMAP recording and playback of offline msg move/copy broken

Categories

(SeaMonkey :: MailNews: Backend, defect)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

playback of offline msg move/copy is broken. We no longer get notified that a move/copy has completed, so we can no longer chain together the move/copies. The first one works, but aborts the whole going online process. It seems that OnStopCopy is only called if you go through the message copy service, but not if you go through the folder. I've gone through the code looking at code that relies on ::OnStopCopy getting called, and found two instances that no longer work - playing back offline move/copies, and doing a message | save as. If it's now the case that we MUST go through the msg copy service, we need to make sure that this is well documented, and that no code use the obsolete mechanism. Ideally, we'd be able to assert if the client code was doing something wrong. Or, if we should be able to use the folder copyMessages method directly, then the listener needs to get notified.
in the case of save as template for imap messages, we now leak the tmp file. Since the tmp file gets put in the current working directory, as near as I can tell, we'll leak them all over the file system.
if you want, I can take a look. Everything should be going through copyService. I have a patch to make deleting imap msgs also go through copy service.
thx, I'm still investigating - there are lots of strange things going on. I need to fix the offline thing myself, but I might hand off the other problems I find to you.
there's also a problem that you can only move one message while offline. Subsequent moves are not reflected in the ui - my guess is that some state is not cleared from the first move, so the subsequent moves are not allowed. Still investigating.
it turns out that when we do an offline move, we never end up clearing the copy request. Investigating.
Summary: IMAP playback of offline msg move/copy broken → IMAP recording and playback of offline msg move/copy broken
Attached patch patch for offline problem (obsolete) (deleted) — Splinter Review
Since my copyService change is responsible here, I thought I take a look and help in fixing it. Also we want this to go in before 1.2 ships. The fix is basically to notify completion to copy Service when we are done with copyOfflineMsgs().
The fix is to use copyService to do CopyFileMessage so that OnStopCopy notification gets sent out.
yes, thanks, I had the same fix. I also have another fix to fix playback.
Attached patch proposed fix (obsolete) (deleted) — Splinter Review
this fixes the recording, and playback of offline imap move/copies (the latter in the cross-server case).
Attachment #98404 - Attachment is obsolete: true
Navin, can you r= my last patch? I'll r= your second patch, and Seth will sr them both. I'll get adt approval to check mine in. You might want to file a separate bug for the tmp file leak. Can you also grep/lxr through the mail/news code for callers of ->CopyMessages and make sure the remaining callers are not passing in a listener? I believe I've found all of the ones that need to be fixed, but it would be good to double check. Thx.
Comment on attachment 98408 [details] [diff] [review] patch for save as template problem even if we don't return on failure CopyFileMessage(), we should still assert if it fails. or, if we don't care that it fails, let's do: (void)copyService->CopyFileMessage(m_fileSpec, templateFolder, nsnull, PR_TRUE, this, nsnull); fix that, and sr=sspitzer
Attachment #98408 - Flags: superreview+
don't overwrite rv, explicitly drop it.
Attachment #98438 - Attachment is obsolete: true
Comment on attachment 98408 [details] [diff] [review] patch for save as template problem r=bienvenu, with Seth's comment.
Attachment #98408 - Flags: review+
Comment on attachment 98440 [details] [diff] [review] patch incorporating Seth's comment sr=sspitzer any reason not to add (void) to the CopyMessages() calls, too? (assuming we don't care about the rv)
Attachment #98440 - Flags: superreview+
Comment on attachment 98440 [details] [diff] [review] patch incorporating Seth's comment r=naving, my part of the fix stays!
Attachment #98440 - Flags: review+
yes, please get approval for the save as file/template part of your fix.
I'll log a separate bug about save as template problem.
Comment on attachment 98408 [details] [diff] [review] patch for save as template problem There is already code below to deal with failed rv. So this is just as before.
Keywords: regression
Comment on attachment 98440 [details] [diff] [review] patch incorporating Seth's comment a=asa (on behalf of drivers) for checkin to 1.2a
Attachment #98440 - Flags: approval+
fix checked into the trunk.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
commerical 2003-01-02-05-trunk Xp 2003-01-02-08-trunk Mac OS 10.1.2, linux 2.2 verified the following: while offline: -can copy/move single or multiple mesgs from imap to local folder imap to imap -can save as file -can save as draft or save as template -under imap acnt -under local folders When online: -for the above test cases, mesgs are still there and don't disspear marking as verified
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: