Last Comment Bug 410320 - Crash when deleting mail, if two windows for the mail are opened [@ nsMsgMailSession::OnItemEvent]
: Crash when deleting mail, if two windows for the mail are opened [@ nsMsgMai...
Status: VERIFIED FIXED
: fixed-seamonkey1.1.8, verified1.8.1.12
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.9beta3
Assigned To: Mark Banner (:standard8)
:
:
Mentors:
: 352171 385097 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-31 04:27 PST by Stefan Persson
Modified: 2008-07-31 04:30 PDT (History)
8 users (show)
standard8: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
stacktrace (2.47 KB, text/plain)
2008-01-02 01:45 PST, Andrew Schultz
no flags Details
Trunk patch (7.53 KB, patch)
2008-01-02 11:17 PST, Mark Banner (:standard8)
neil: review+
Details | Diff | Splinter Review
New Trunk Patch (9.16 KB, patch)
2008-01-05 06:34 PST, Mark Banner (:standard8)
neil: review-
Details | Diff | Splinter Review
New Trunk Patch v2 (17.75 KB, patch)
2008-01-07 14:07 PST, Mark Banner (:standard8)
neil: review+
Details | Diff | Splinter Review
[checked in] Branch patch (7.40 KB, patch)
2008-01-08 09:57 PST, Mark Banner (:standard8)
standard8: review+
mscott: superreview+
dveditz: approval1.8.1.12+
Details | Diff | Splinter Review
[checked in] New Trunk Patch v3 (17.41 KB, patch)
2008-01-13 05:40 PST, Mark Banner (:standard8)
standard8: review+
mozilla: superreview+
Details | Diff | Splinter Review
[checked in] Fix interface parameter names (11.32 KB, patch)
2008-01-14 14:19 PST, Mark Banner (:standard8)
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review

Description Stefan Persson 2007-12-31 04:27:36 PST
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.
Comment 1 WADA 2007-12-31 07:54:43 PST
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.
Comment 2 Andrew Schultz 2008-01-02 00:45:12 PST
> 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?
Comment 3 WADA 2008-01-02 01:22:13 PST
(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. 
Comment 4 WADA 2008-01-02 01:24:03 PST
"Open Message in New Tab" should have been "Open Message in New Window".
When two tabs, crash don't occur. Sorry for spam.
Comment 5 Andrew Schultz 2008-01-02 01:45:12 PST
Created attachment 295073 [details]
stacktrace
Comment 6 Andrew Schultz 2008-01-02 01:55:03 PST
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.
Comment 7 Mark Banner (:standard8) 2008-01-02 11:17:34 PST
Created attachment 295127 [details] [diff] [review]
Trunk patch

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.
Comment 8 Andrew Schultz 2008-01-02 11:34:37 PST
> 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?
Comment 9 Mark Banner (:standard8) 2008-01-02 14:09:41 PST
(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 10 neil@parkwaycc.co.uk 2008-01-03 05:26:48 PST
Comment on attachment 295127 [details] [diff] [review]
Trunk patch

Should we be using the newish nsTObserverArray?
Comment 11 neil@parkwaycc.co.uk 2008-01-04 09:28:40 PST
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.
Comment 12 Mark Banner (:standard8) 2008-01-05 06:34:12 PST
Created attachment 295500 [details] [diff] [review]
New Trunk Patch

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 13 neil@parkwaycc.co.uk 2008-01-05 09:40:17 PST
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.
Comment 14 neil@parkwaycc.co.uk 2008-01-05 09:47:43 PST
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.
Comment 15 Wayne Mery (:wsmwk, NI for questions) 2008-01-07 09:44:51 PST
is nsMsgDBFolder::NotifyFolderEvent(nsIAtom*) the proper top of stack notation for the bug summary?
Comment 16 Andrew Schultz 2008-01-07 11:36:15 PST
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).
Comment 17 Mark Banner (:standard8) 2008-01-07 14:07:23 PST
Created attachment 295834 [details] [diff] [review]
New Trunk Patch v2

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).
Comment 18 Wayne Mery (:wsmwk, NI for questions) 2008-01-07 22:10:19 PST
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

Comment 19 Mark Banner (:standard8) 2008-01-08 00:11:05 PST
(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 20 neil@parkwaycc.co.uk 2008-01-08 08:09:24 PST
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));
Comment 21 Mark Banner (:standard8) 2008-01-08 09:57:27 PST
Created attachment 295964 [details] [diff] [review]
[checked in] Branch patch

Branch patch that fixes bit-rot. Carrying forward Neil's r, requesting sr.
Comment 22 Scott MacGregor 2008-01-08 11:11:16 PST
Comment on attachment 295964 [details] [diff] [review]
[checked in] Branch patch

this looks good to me Mark.
Comment 23 Mark Banner (:standard8) 2008-01-08 11:18:33 PST
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.
Comment 24 Dan Mosedale (:dmose) 2008-01-08 18:55:37 PST
(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?
Comment 25 neil@parkwaycc.co.uk 2008-01-09 08:45:20 PST
(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 26 neil@parkwaycc.co.uk 2008-01-12 15:59:55 PST
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).
Comment 27 Mark Banner (:standard8) 2008-01-13 05:40:39 PST
Created attachment 296824 [details] [diff] [review]
[checked in] New Trunk Patch v3

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 28 David :Bienvenu 2008-01-13 08:28:18 PST
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.
Comment 29 Mark Banner (:standard8) 2008-01-13 14:10:51 PST
(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).
Comment 30 Mark Banner (:standard8) 2008-01-14 14:19:10 PST
Created attachment 297070 [details] [diff] [review]
[checked in] Fix interface parameter names

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 31 Daniel Veditz [:dveditz] 2008-01-14 15:41:59 PST
Comment on attachment 295964 [details] [diff] [review]
[checked in] Branch patch

approved for 1.8.1.12, a=dveditz for release-drivers
Comment 32 David :Bienvenu 2008-01-21 11:48:49 PST
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 33 Mark Banner (:standard8) 2008-01-21 13:29:58 PST
Comment on attachment 297070 [details] [diff] [review]
[checked in] Fix interface parameter names

Checked in with nit addressed
Comment 34 Mark Banner (:standard8) 2008-01-21 13:32:44 PST
All checked in -> fixed.
Comment 35 timeless 2008-02-06 03:12:56 PST
*** Bug 385097 has been marked as a duplicate of this bug. ***
Comment 36 Mark Banner (:standard8) 2008-02-06 04:32:04 PST
*** Bug 352171 has been marked as a duplicate of this bug. ***
Comment 37 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2008-02-07 14:59:51 PST
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.
Comment 38 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2008-02-08 15:15:35 PST
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?
Comment 39 Al Billings [:abillings] 2008-02-13 18:12:14 PST
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.
Comment 40 Al Billings [:abillings] 2008-02-13 18:14:34 PST
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.
Comment 41 WADA 2008-02-13 19:16:53 PST
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?
Comment 42 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2008-02-13 22:58:12 PST
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.
Comment 43 Mark Banner (:standard8) 2008-02-13 23:49:32 PST
(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.
Comment 44 Al Billings [:abillings] 2008-02-14 15:21:33 PST
Thanks, Henrik. A simple mistake. I appreciate the help here!

Note You need to log in before you can comment on or make changes to this bug.