Crash when deleting mail, if two windows for the mail are opened [@ nsMsgMailSession::OnItemEvent]

VERIFIED FIXED in mozilla1.9beta3

Status

MailNews Core
Backend
--
critical
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Stefan Persson, Assigned: standard8)

Tracking

({fixed-seamonkey1.1.8, verified1.8.1.12})

Trunk
mozilla1.9beta3
fixed-seamonkey1.1.8, verified1.8.1.12
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Crash when deleting mail → Crash when deleting mail, if two windows for the mail are opened

Comment 2

10 years ago
> 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.

Comment 5

10 years ago
Created attachment 295073 [details]
stacktrace

Comment 6

10 years ago
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.
Assignee: mail → nobody
Component: MailNews: Main Mail Window → MailNews: Backend
Product: Mozilla Application Suite → Core
QA Contact: backend
Version: unspecified → Trunk
(Assignee)

Comment 7

10 years ago
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.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #295127 - Flags: review?(neil)

Comment 8

10 years ago
> 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?

Updated

10 years ago
Severity: normal → critical
(Assignee)

Comment 9

10 years ago
(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

10 years ago
Comment on attachment 295127 [details] [diff] [review]
Trunk patch

Should we be using the newish nsTObserverArray?

Comment 11

10 years ago
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.
Attachment #295127 - Flags: review?(neil) → review+
(Assignee)

Comment 12

10 years ago
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).
Attachment #295500 - Flags: review?(neil)

Comment 13

10 years ago
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.
Attachment #295500 - Flags: review?(neil) → review-

Comment 14

10 years ago
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?

Comment 16

10 years ago
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).
Summary: Crash when deleting mail, if two windows for the mail are opened → Crash when deleting mail, if two windows for the mail are opened [@ nsMsgMailSession::OnItemEvent]
(Assignee)

Comment 17

10 years ago
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).
Attachment #295500 - Attachment is obsolete: true
Attachment #295834 - Flags: review?(neil)
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

(Assignee)

Comment 19

10 years ago
(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

10 years ago
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));
(Assignee)

Comment 21

10 years ago
Created attachment 295964 [details] [diff] [review]
[checked in] Branch patch

Branch patch that fixes bit-rot. Carrying forward Neil's r, requesting sr.
Attachment #295127 - Attachment is obsolete: true
Attachment #295964 - Flags: superreview?(mscott)
Attachment #295964 - Flags: review+

Comment 22

10 years ago
Comment on attachment 295964 [details] [diff] [review]
[checked in] Branch patch

this looks good to me Mark.
Attachment #295964 - Flags: superreview?(mscott) → superreview+
(Assignee)

Comment 23

10 years ago
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.
Attachment #295964 - Flags: approval1.8.1.12?
(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

10 years ago
(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

10 years ago
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).
Attachment #295834 - Flags: review?(neil) → review+
(Assignee)

Comment 27

10 years ago
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.
Attachment #295834 - Attachment is obsolete: true
Attachment #296824 - Flags: superreview?(bienvenu)
Attachment #296824 - Flags: review+

Comment 28

10 years ago
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.
Attachment #296824 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Updated

10 years ago
Attachment #296824 - Attachment description: New Trunk Patch v3 → [checked in] New Trunk Patch v3
(Assignee)

Comment 29

10 years ago
(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).
(Assignee)

Comment 30

10 years ago
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.
Attachment #297070 - Flags: superreview?(bienvenu)
Attachment #297070 - Flags: review?(bienvenu)
Comment on attachment 295964 [details] [diff] [review]
[checked in] Branch patch

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #295964 - Flags: approval1.8.1.12? → approval1.8.1.12+
(Assignee)

Updated

10 years ago
Flags: in-testsuite+
(Assignee)

Updated

10 years ago
Attachment #295964 - Attachment description: Branch patch → [checked in] Branch patch
(Assignee)

Updated

10 years ago
Keywords: fixed-seamonkey1.1.8, fixed1.8.1.12

Comment 32

10 years ago
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);
Attachment #297070 - Flags: superreview?(bienvenu)
Attachment #297070 - Flags: superreview+
Attachment #297070 - Flags: review?(bienvenu)
Attachment #297070 - Flags: review+
(Assignee)

Comment 33

10 years ago
Comment on attachment 297070 [details] [diff] [review]
[checked in] Fix interface parameter names

Checked in with nit addressed
Attachment #297070 - Attachment description: Fix interface parameter names → [checked in] Fix interface parameter names
(Assignee)

Comment 34

10 years ago
All checked in -> fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

9 years ago
Duplicate of this bug: 385097
(Assignee)

Updated

9 years ago
Duplicate of this bug: 352171
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.
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9beta3
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?
Status: RESOLVED → VERIFIED
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.
Keywords: fixed1.8.1.12 → verified1.8.1.12
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago9 years ago
Resolution: --- → 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.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 43

9 years ago
(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!
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.