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)
SeaMonkey
MailNews: Backend
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: Bienvenu, Assigned: Bienvenu)
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
Bienvenu
:
review+
sspitzer
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
naving
:
review+
sspitzer
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•22 years ago
|
||
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.
Comment 2•22 years ago
|
||
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.
Assignee | ||
Comment 3•22 years ago
|
||
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.
Assignee | ||
Comment 4•22 years ago
|
||
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.
Assignee | ||
Comment 5•22 years ago
|
||
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
Comment 6•22 years ago
|
||
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().
Comment 7•22 years ago
|
||
The fix is to use copyService to do CopyFileMessage so that OnStopCopy
notification gets sent out.
Assignee | ||
Comment 8•22 years ago
|
||
yes, thanks, I had the same fix. I also have another fix to fix playback.
Assignee | ||
Comment 9•22 years ago
|
||
this fixes the recording, and playback of offline imap move/copies (the latter
in the cross-server case).
Attachment #98404 -
Attachment is obsolete: true
Assignee | ||
Comment 10•22 years ago
|
||
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 11•22 years ago
|
||
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+
Assignee | ||
Comment 12•22 years ago
|
||
don't overwrite rv, explicitly drop it.
Attachment #98438 -
Attachment is obsolete: true
Assignee | ||
Comment 13•22 years ago
|
||
Comment on attachment 98408 [details] [diff] [review]
patch for save as template problem
r=bienvenu, with Seth's comment.
Attachment #98408 -
Flags: review+
Comment 14•22 years ago
|
||
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 15•22 years ago
|
||
Comment on attachment 98440 [details] [diff] [review]
patch incorporating Seth's comment
r=naving, my part of the fix stays!
Attachment #98440 -
Flags: review+
Assignee | ||
Comment 16•22 years ago
|
||
yes, please get approval for the save as file/template part of your fix.
Comment 17•22 years ago
|
||
I'll log a separate bug about save as template problem.
Comment 18•22 years ago
|
||
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.
Updated•22 years ago
|
Keywords: regression
Comment 19•22 years ago
|
||
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+
Assignee | ||
Comment 20•22 years ago
|
||
fix checked into the trunk.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 21•22 years ago
|
||
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
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•