173137 - Mail reappears if you delete while compacting local folders.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Navin Gupta]

Comment 1

[Navin Gupta]

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. 

Comment 2

[Navin Gupta]

Attachment: 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.

Comment 3

[Navin Gupta]

Attachment: 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.

Comment 4

[Navin Gupta]

Can I get reviews for last patch ? thx.

Comment 5

[Navin Gupta]

Attachment: 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.

Comment 6

[Navin Gupta]

This patch also prevents changing read/flagged status besides deleting.

Comment 7

[Cavin Song]

Comment on attachment 102055 [details] [diff] [review]
proposed fix

r=cavin.

Comment 8

[David :Bienvenu]

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?

Comment 9

[Navin Gupta]

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. 

Comment 10

[David :Bienvenu]

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...

Comment 11

[Navin Gupta]

I can put an alert if that is what you want. 

Comment 12

[David :Bienvenu]

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.

Comment 13

[Navin Gupta]

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

Comment 14

[Navin Gupta]

Attachment: patch addressing david's suggestion

I just check for lock now instead of acquiring/releaseing and throw an alert if
delete fails.

Comment 15

[Navin Gupta]

I changed the comment "try to get the lock..." locally.

Comment 16

[Navin Gupta]

cc robinf for alert wording review. thx robin

Comment 17

[David :Bienvenu]

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?

Comment 18

[Navin Gupta]

fix checked in. your case was covered.

Comment 19

[Aleksey Nogin]

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.

Comment 20

[Navin Gupta]

fixed warning although srcFolder should never be null. 

Comment 21

[robinf]

Sorry for not responding sooner. The error message wording looks fine.

Comment 22

[Navin Gupta]

*** Bug 177076 has been marked as a duplicate of this bug. ***