User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8) Gecko/20071009 SeaMonkey/1.1.5 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8) Gecko/20071009 SeaMonkey/1.1.5 If a mail is opened twice and I delete it, SeaMonkey crashes. Reproducible: Always Steps to Reproduce: 1. Open the same mail twice, in two different windows. 2. Select the mail from the list of mail in the main window. 3. Press "delete". Actual Results: SeaMonkey crashed, and the mail was deleted. Expected Results: SeaMonkey shouldn't have crashed.

Confirmed with Tb trunk 2007/12/19 build. When 2 windows for a mail, crash occurred. Crash ID: bp-7954f507-b7b7-11dc-9f4e-001a4bd43ef6 But when two tab for a mail, crash didn't occur. This problem can not occur if independent mail window is opened via double click of a mail, because independent window is re-used then only one window for mail is used.

> 1. Open the same mail twice, in two different windows. I'm not sure what this means. How are you opening the same mail in two different windows?

(In reply to comment #2) > How are you opening the same mail in two different windows? "Open Message in New Tab" in context menu at Thread pane.

"Open Message in New Tab" should have been "Open Message in New Window". When two tabs, crash don't occur. Sorry for spam.

also... (gdb) frame 8 #8 0xb69aad10 in nsMsgMailSession::OnItemEvent (this=0x884e7e0, aFolder=0x90cf5ec, aEvent=0x8846f18) at /build/andrew/moz-debug/mozilla/mailnews/base/src/nsMsgMailSession.cpp:260 260 nsCOMPtr<nsIFolderListener> listener = mListeners[i]; (gdb) p i $1 = 9 (gdb) p mListeners.Count() [Switching to Thread -1208821344 (LWP 8102)] $2 = 8 So one of the listeners removed a listener (two, actually). Oops. Checking Count() each time wouldn't crash, but would still do the wrong thing if a listener removed an already-fired listener.

This has been seen quite a bit on branch (TB 2.0), http://talkback-public.mozilla.org/search/start.jsp?search=1&searchby=stacksig&match=contains&searchfor=nsMsgMailSession%3A%3AOnItemEvent&vendor=MozillaOrg&product=All&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid&rlimit=500 The patch I'm attaching changes the code to a) iterate backwards through the listeners so that if one is removed whilst we're going through then it doesn't matter, b) make sure that when we add a listener to the list it doesn't add null ones and c) we can then simplify the code and optimise it by calling the function on the value of the listener direct. I don't see a problem with calling the listeners in reverse order, there shouldn't be any dependencies there, and if there are, then its a bug. The patch doesn't apply on branch - I check later as to why, as I think this may be worth porting to branch.

> a) iterate backwards through the listeners so that if one is removed whilst > we're going through then it doesn't matter, Hmm. If you had listeners A, B and C and C removed A, wouldn't you then fire C again? If C removed A and B, wouldn't you still crash?

(In reply to comment #8) > > a) iterate backwards through the listeners so that if one is removed whilst > > we're going through then it doesn't matter, > > Hmm. If you had listeners A, B and C and C removed A, wouldn't you then fire C > again? If C removed A and B, wouldn't you still crash? That's possible, but its not normally how listeners work. Normally a listener (or something connected to it) only removes itself, e.g. when it gets a message that it interprets as it is no longer necessary (e.g. close the window because the email has just been deleted). I think you'll find that's the general assumption for listeners around the code base.

Comment on attachment 295127 [details] [diff] [review] Trunk patch Should we be using the newish nsTObserverArray?

Comment on attachment 295127 [details] [diff] [review] Trunk patch Actually I'm happy with this way on branch, even if we manage to get nsTObserverArray working on trunk.

This changes the code to use nsTObserverArray. Note that we can't get the full benefit of it as we have to respect the listener flags array as well. This means that we still have to just iterate backwards through the arrays unless we came up with a completely new enumerator for it, which I think would mean a new class as well (the enumerators in nsTObserverArray have their position adjusted as elements are added and removed).

Comment on attachment 295500 [details] [diff] [review] New Trunk Patch There's no advantage over an nsCOMArray if you don't use the iterators. More in a sec.

To use an nsTObserverArray effectively in this case, we need to define a new struct to hold both the listener and its flags, something like this: struct folderListener { nsCOMPtr<nsIFolderListener> mListener; PRUint32 mNotifyFlags; folderListener(nsIFolderListener aListener, PRUint32 aNotifyFlags) : mListener(aListener), mNotifyFlags(aNotifyFlags) {} folderListener(folderListener aListener) : mListener(aListener.mListener), mNotifyFlags(aListener.mNotifyFlags) {} ~folderListener() {} int operator==(nsIFolderListener *aListener) { return mListener == aListener; } } The operator== allows us to use IndexOf.

is nsMsgDBFolder::NotifyFolderEvent(nsIAtom*) the proper top of stack notation for the bug summary?

just the method name (without the parameters). Mark's patch would actually fix crashes with a bunch of different stacks (OnItemRemoved, OnItemAdded, OnItemPropertyFlagChanged...) and talkback/crashreporter won't always have the top frame (see the crashreporter stack from comment 1).

New trunk patch, using a struct along the lines that Neil suggested. I also decided it was time to expand our unit tests into mailnews/base (and easily check the code is working properly).

potentials: bug 385097 bug 352171 deleting last message in folder from open message window with TB Header Tools Extension 0.6.6 [@ nsMsgMailSession::OnItemEvent (seems like there should be more) might this also hit bug 409960? nsImapFlagAndUidState::AddUidFlagPair

(In reply to comment #18) > potentials: > bug 385097 > bug 352171 deleting last message in folder from open message window with TB > Header Tools Extension 0.6.6 [@ nsMsgMailSession::OnItemEvent > (seems like there should be more) Anything with nsMsgMailSession::On* in its top level and is an access violation is a likely candidate for being fixed by this bug. > might this also hit bug 409960? nsImapFlagAndUidState::AddUidFlagPair Highly unlikely given that nsMsgMailSession doesn't appear in the stack. More likely a problem in the imap protocol code somewhere.

Comment on attachment 295834 [details] [diff] [review] New Trunk Patch v2 >+ folderListener newListener(listener, notifyFlags); >+ >+ return mListeners.AppendElementUnlessExists(newListener) ? NS_OK : NS_ERROR_FAILURE; I'm not convinced by this change... I wish bienvenu was still around on IRC :-\ >+#define NOTIFY_FOLDER_LISTENERS(propertyflag_, propertyfunc_) \ I think you should use NOTIFY_FOLDER_LISTENERS4 instead. Either that or #define NOTIFY_FOLDER_LISTENERS(propertyflag_, propertyfunc_, params_) \ ... fL.mListener->propertyfunc_ params_; \ ... NOTIFY_FOLDER_LISTENERS(propertyChanged, OnItemPropertyChanged, (item, property, oldValue, newValue));

Branch patch that fixes bit-rot. Carrying forward Neil's r, requesting sr.

Comment on attachment 295964 [details] [diff] [review] [checked in] Branch patch this looks good to me Mark.

Comment on attachment 295964 [details] [diff] [review] [checked in] Branch patch Requesting branch approval for low risk mailnews (SM/TB) patch that fixes some crashes seen on the branch builds. Just iterates through the listeners in a different order, and protects against null listeners.

(In reply to comment #20) > (From update of attachment 295834 [details] [diff] [review]) > >+ folderListener newListener(listener, notifyFlags); > >+ > >+ return mListeners.AppendElementUnlessExists(newListener) ? NS_OK : NS_ERROR_FAILURE; > I'm not convinced by this change... Meaning you think that since it never returned an error before, it shouldn't start now? Or that we should append duplicate elements like the old code did?

(In reply to comment #24) > (In reply to comment #20) > > (From update of attachment 295834 [details] [diff] [review] [details]) > > >+ folderListener newListener(listener, notifyFlags); > > >+ > > >+ return mListeners.AppendElementUnlessExists(newListener) ? NS_OK : NS_ERROR_FAILURE; > > I'm not convinced by this change... > > Meaning you think that since it never returned an error before, it shouldn't > start now? > > Or that we should append duplicate elements like the old code did? Either, really ;-)

Comment on attachment 295834 [details] [diff] [review] New Trunk Patch v2 >+ return mListeners.AppendElementUnlessExists(newListener) ? NS_OK : NS_ERROR_FAILURE; r=me but I'd prefer this changed to unconditionally append the listener (and remove the second operator== of course).

Revised as per Neil's comment about adding listeners, also simplified as per Neil's previous comment and used one #define rather than two.

Comment on attachment 296824 [details] [diff] [review] [checked in] New Trunk Patch v3 thx, Mark. While you're here, if you feel like it, you could fix the variable names to the idl methods to be aListener instead of listener, aNotifyFlags instead of notifyFlags, etc.

(In reply to comment #28) > (From update of attachment 296824 [details] [diff] [review]) > thx, Mark. While you're here, if you feel like it, you could fix the variable > names to the idl methods to be aListener instead of listener, aNotifyFlags > instead of notifyFlags, etc. I'm going to deal with that in a follow up patch that I'll post on this bug. Hence keeping this bug open a little longer, although this issue should now be fixed on trunk (branch still waiting approval).

Probably should have done this at the same time as the trunk patch, but here's the suggested update to the interface parameter names. I also added some documentation to nsIMsgMailSession as it was easy.

Comment on attachment 295964 [details] [diff] [review] [checked in] Branch patch approved for 1.8.1.12, a=dveditz for release-drivers

Comment on attachment 297070 [details] [diff] [review] [checked in] Fix interface parameter names thx, Mark, r/sr=me, modulo one you missed, I think (aEvent): + void OnItemEvent(in nsIMsgFolder aItem, in nsIAtom event);

Comment on attachment 297070 [details] [diff] [review] [checked in] Fix interface parameter names Checked in with nit addressed

All checked in -> fixed.

Same on OS X and other platforms. I can't verify the fix with the latest nightly builds due to bug 416232. Will do that as soon as it will be possible.

Ok, the crash is gone with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12pre) Gecko/20080111 Thunderbird/2.0.0.12pre Mnenhy/0.7.5.0 ID:2008020703 Having two windows open with the deleted messages closes them automatically after some seconds with focus. Mark, wouldn't it be better to close them immediately when deleting the messages? Shall I file a new bug or is there already one? Do you know?

Verified that the crash does not occur in the 2.0.0.12 rc1: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12) Gecko/2008021304 Thunderbird/2.0.0.12. I can repro it cleanly in 2.0.0.9.

Henrik, you verified it in a 1.8.1 branch build, which means you should have set the "verified1.8.1.12" keyword, not changed the resolution. Since this is a trunk bug as well, it should only be verified fixed for overall bug status when it is verified in trunk builds. I'm changing the status back to being resolved fixed.

Tb trunk(2008/2/13 build, MS Win-XP SP2) crashed when next sinister test. (1) Open 5 independent mail windows for a mail, via "Open Message in New Window". (2) Delete the mail at thread pane => No crash, 5 windows remained. (3) On each click of independent window, window was closed. (4) When final window(first opened window in my test) was clicked, Tb crashed. Crash ID: bp-eeea83ca-daa9-11dc-977a-001a4bd43ef6 Should we open new bug?

Al, sorry but Mnenhy set an user agent override. So the real user agent was hidden. :( I verified this bug with a trunk build. Otherwise I wouldn't have changed it to verified. Once again verified with version 3.0a1pre (2008020703) WADA, yes please file a new bug if none is already filed with this top stack frame. It would be nice if you could CC me to this bug.

(In reply to comment #41) > Crash ID: bp-eeea83ca-daa9-11dc-977a-001a4bd43ef6 > Should we open new bug? No, its already covered by bug 415601.

Thanks, Henrik. A simple mistake. I appreciate the help here!