Status: NEW → ASSIGNED
Summary: Mail reappears if you delete while compacting. → Mail reappears if you delete while compacting local folders.
The problem here is that before compacting we ask the db for all messages. But as we are compacting we can delete and that changes the db but the compact doesn't know about it and when compact finishes we get all messages in the folder before compact started.
Created attachment 102049 [details] [diff] [review] proposed fix Obtain the lock before opening an outputstream to the folder to set the hdr flag on the db (msf file) and berkeley mailbox.
Created attachment 102052 [details] [diff] [review] proposed fix Obtain the lock before opening an outputstream to the folder to set the hdr flag on the db (msf file) and berkeley mailbox.
Attachment #102049 - Attachment is obsolete: true
Can I get reviews for last patch ? thx.
Created attachment 102055 [details] [diff] [review] proposed fix Obtain the lock before opening an outputstream to the folder to set the hdr flag on the db (msf file) and berkeley mailbox.
Attachment #102052 - Attachment is obsolete: true
This patch also prevents changing read/flagged status besides deleting.
Comment on attachment 102055 [details] [diff] [review] proposed fix r=cavin.
Attachment #102055 - Flags: review+
I guess I would have expected the fix to be to have the nsLocalMailFolder delete/move call try to grab the folder lock, and release it when the delete finished, instead of having every little delete at the db level try to get the folder lock. If I understand correctly, with this fix, you could do a move, and have the copy succeed, but the delete fail, if, for example, a compact came along between the move and the delete. It seems like it would be easier to recover from errors if we made the delete/move atomic with respect to the folder lock, instead of having the lowest level get and release the folder lock. Is there some reason that didn't work?
I tried to fix problems more than delete, like when you change the msgHdr flag (read/flagged/label etc). They will also have the same problem as delete. That is why it makes sense to put the lock there.
what happens with your patch if the user deletes/marks read/flags a message while the compact is going on? Do they get an error message, or does it just fail silently? I'm assuming it can't succeed, since getting the folder lock should fail if a compact is going on. If it fails silently, I'm not quite sure how that's really so much better than the current situation...
I can put an alert if that is what you want.
what do you think? do you think silently failing is a better user experience? Or putting up an alert? Or disabling those commands that can't be run because a compact is going on? The last one might be hard, because you'd have to make sure the commands are re-enabled when the compact is finished.
We can put an alert saying some other operation is writing to folder. I think that would be better than failing silently. For me as a user failing silently is better(percentage progess is showing something is going on), but I don't think everyone would know what is going on. Then there would be bugs delete not working. We can throw alert for delete. For other cases it looks like we are not propagating error code, so it may be difficult unless we return rv
Created attachment 102237 [details] [diff] [review] patch addressing david's suggestion I just check for lock now instead of acquiring/releaseing and throw an alert if delete fails.
I changed the comment "try to get the lock..." locally.
cc robinf for alert wording review. thx robin
Comment on attachment 102237 [details] [diff] [review] patch addressing david's suggestion looks good, sr=bienvenu - just to be sure, the message is not deleted from the ui/db in this case? And after the operation finishes, everything is OK? (it looks like it should be, but testing it would be good - for example, can you still delete messages after this happens?
Attachment #102237 - Flags: superreview+
fix checked in. your case was covered.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
This commit have added a "might be used uninitialized" warning on Brad TBox: +mailnews/local/src/nsLocalMailFolder.cpp:2654 + `nsresult result' might be used uninitialized in this function Indeed, if srcFolder happens to be NULL, then nsMsgLocalMailFolder::EndMove may end up testing NS_SUCCEEDED on an uninitialized nsresult - see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/local/src/nsLocalMailFolder.cpp&rev=1.384&mark=2654,2681,2684,2691#2652 P.S. See bug 59652 for more on "might be used uninitialized" warnings.
fixed warning although srcFolder should never be null.
Sorry for not responding sooner. The error message wording looks fine.
*** Bug 177076 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.