87872 - Renaming a local folder and exit would crash mail program

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P2)

Description

[Cavin Song]

BUILD ID: 20001062503

Steps to reporduce:
1. Launch mail program and log into your mail account.
2. click on a local folder and rename it (say, from 'aaa' to 'bbb').
3. Exit mail program.

The mail program would then crash. The local folder was renamed ok though.

Comment 1

[Håkan Waara]

Tried this several times and was never able to reproduce.

build 2001062504

Comment 2

[Navin Gupta]

ya, this was not reproducible always and cavin seems to have found the fix 
when it does happen. I think it is ok to remove that old code. reassign back 
to cavin , he can check it in with his other changes. 

Comment 3

[(not reading, please use [e-mail] instead)]

I never think it is good to call Release() on yourself.  that scares me.

it smells like someone couldn't properly fix the ownership model.  

sure enough, that line of code is from jefft.  

my guess is someone has a reference to the folder, so jefft call's Release() to 
force the ref cnt to 0 so the dtor gets called.

can you attach your stack trace when you crash? that will help figure out has 
the reference.

here are his checkin comments:

"fixed bug 17765 - Rename not implemented for Pop3, bug 19097 -- copying 
messages cause corrupted messages if the message size is greater than 4 k; 
r=putterman; make sure all children node were deleted when rename a folder, 
also rename the directory if it has subfolders; added m_leftOver to keep 
tracking partial completed line"

if you remove the Release() call, see if it causes those problems to come back.
(make sure to try to rename folders with children)

the proper fix will be to break the ref.

Comment 4

[Navin Gupta]

PropagateDelete should delete the children. I think we will have to change the
way it works by calling rename on the parent so that the propagateDelete can
delete the folder to be renamed. I think this may also be cause of bug 65303.
taking it back from cavin. 

Comment 5

[scottputterman]

If you can come up with a safe fix for this we should consider it for the branch.

Comment 6

[Navin Gupta]

A safe fix would be to just remove the Release(); We already know that we
leak the folder once we rename a folder (bug 87974) inspite of Release(). 

Comment 7

[Cavin Song]

Yes, Navin, removing the Release() call seems to fix the problem. I tried it
without problems when I was fixing #41944.

Comment 8

[scottputterman]

If you can get the r=/sr= and agreement that this is correct, we can try to get
this for a branch build next week.

Comment 9

[scottputterman]

If you can try to get this on the trunk so we can test this, that would be great.

Comment 10

[Navin Gupta]

Attachment: proposed fix

Comment 11

[Navin Gupta]

We already leak on rename of local folder. Removing the Release();
so that it doesn't crash. looking for review from cavin and david.  

Comment 12

[Navin Gupta]

Attachment: ignore the last patch. this is the correct one.

Comment 13

[Cavin Song]

r=cavin.

Comment 14

[David :Bienvenu]

sr=bienvenu

Comment 15

[Navin Gupta]

fix landed on trunk

Comment 16

[Sheela Ravindran]

I was able to reproduce crash on win98 while renaming the local folder and 
closing the application. The 4th time I renamed and exit the application I saw 
the crash. After that I tried another 6 times and failed to see the crash on 
2001-07-02-06 trunk build on win98.
I was unable to reproduce the crash on mac(2001070206) and linux(2001062904)

However, based on seeing it once on win98 and having tried to rename the local 
folder for about 10 times did not crash using the latest trunk build only
2001070606-win98, mac, linux.  Still needs verification on branch when it is 
checked in.


verifiedtrunk

Comment 17

[scottputterman]

removing nsbranch and marking fixed since it's on the trunk.

Comment 18

[fenella]

Linux (2001-07-24-05-0.9.2)
Win32 (2001-07-24-06-0.9.2)
Mac (2001-07-24-03-0.9.2)
I verified it on the branch.