387579 - crash while doing nothing... [@ nsMsgCompose::OnSendNotPerformed]

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Eyal Rozenberg]

Talkback:

http://talkback-public.mozilla.org/search/start.jsp?search=2&type=iid&id=TB33910695X

There I was, minding my own business and working with other apps while sm trunk build 2007070201 was in the background, and it crashes on me.

Comment 1

[timeless]

Incident ID: 33910695
Stack Signature	nsMsgCompose::OnSendNotPerformed 8153060a
Product ID	MozillaTrunk
Build ID	2007070201
Trigger Time	2007-07-10 12:21:57.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	mail.dll + (0007252d)
URL visited	
User Comments	nothing! sm mailnews was in the background
Since Last Crash	2212 sec
Total Uptime	4126 sec
Trigger Reason	Access violation
Source File, Line No.	d:\builds\tinderbox\seamonkeytrunk\winnt_5.2_clobber\mozilla\mailnews\compose\src\nsmsgcompose.cpp, line 2981
Stack Trace 	
nsMsgCompose::OnSendNotPerformed  [mozilla/mailnews/compose/src/nsmsgcompose.cpp, line 2981]
nsMsgCompose::ProcessSignature  [mozilla/mailnews/compose/src/nsmsgcompose.cpp, line 3707]
nsMsgComposeAndSend::DoDeliveryExitProcessing  [mozilla/mailnews/compose/src/nsmsgsend.cpp, line 3753]
LocateMessageFolder  [mozilla/mailnews/compose/src/nsmsgcopy.cpp, line 463]
nsMsgCopyService::CopyMessages  [mozilla/mailnews/base/src/nsmsgcopyservice.cpp, line 457]
nsMsgLocalMailFolder::CopyAllSubFolders  [mozilla/mailnews/local/src/nslocalmailfolder.cpp, line 1788]
nsMsgLocalMailFolder::EndMessage  [mozilla/mailnews/local/src/nslocalmailfolder.cpp, line 2738]
nsMsgLocalMailFolder::EndCopy  [mozilla/mailnews/local/src/nslocalmailfolder.cpp, line 2488]
nsMsgCopyService::FindRequest  [mozilla/mailnews/base/src/nsmsgcopyservice.cpp, line 415]
nsMsgCopyService::CopyMessages  [mozilla/mailnews/base/src/nsmsgcopyservice.cpp, line 480]
nsMsgFolderCache::Commit  [mozilla/mailnews/base/src/nsmsgfoldercache.cpp, line 358]
LocateMessageFolder  [mozilla/mailnews/compose/src/nsmsgcopy.cpp, line 522]
nsMsgSendReport::DisplayReport  [mozilla/mailnews/compose/src/nsmsgsendreport.cpp, line 268]
nsMsgComposeAndSend::nsMsgComposeAndSend  [mozilla/mailnews/compose/src/nsmsgsend.cpp, line 241]

I'm not certain about the reason for the crash, but the code I'm highlighting *is* broken. and I want it fixed.

146 class nsCOMArray : public nsCOMArray_base
245     PRBool AppendObject(T *aObject) {
246         return nsCOMArray_base::AppendObject(NS_STATIC_CAST(nsISupports*, aObject));
167   nsCOMArray<nsIMsgSendListener>            mExternalSendListeners;
909                      nsresult rv = mExternalSendListeners.AppendObject(aMsgSendListener );
910                      return rv;

for reference, this stuff has already been fixed once before in this file.

here's the url to read:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/compose/src/nsMsgCompose.cpp&mark=2981,909-910,916,4248&rev=1.520#2981

note especially 1.368 <[e-mail]> which was bz's last fix for this error.

Comment 2

[timeless]

fwiw, bz notes that nsTObserverArray is probably what should be used for any observer list (it's a weak reference holder, but it handles mutations). The code used for the for loop for all of the callbacks is essentially wrong because all sorts of bad things can happen during mutations.

I'm not sure that I can actually explain the crash with any of the changes I'm asking for, but at least the return fixes need to be made.

Comment 3

[Ivo van Dongen]

I'll take a look at this, don't know exactly what the problem is yet (has been quite a while since the patch). One problem though, I'm just about to leave for a holiday. This will have to wait until I return in August, sorry.

Comment 4

[Eyal Rozenberg]

August is now over... Ivo: any progress?

Comment 5

[Ivo van Dongen]

I'm afraid I've been to optimistic about my spare time. At the moment I don't have the time to set up a build env and look at what the problem is (if I can even figure it out..). I'll be happy to look at it in a while, but I think that will be in at least a month or so. I'll do my best.

Comment 6

[Ivo van Dongen]

Sorry it has taken so long, but I've finally freed up some time. I'm setting up the build env now and I'll submit the patch asap.

Comment 7

[Ivo van Dongen]

Ok, I've taken a look and if I understand it correctly you are asking for two things:
 - Fix the returns in AddMsgSendListener and RemoveMsgSendListener
 - Use nsTObserverArray instead of nsCOMArray for the send listeners
Right?

The first wasn't an issue, but I'm a bit stuck on the second. I've changed the type and adapted the methods. For example:

NS_IMETHODIMP nsMsgCompose::OnSendNotPerformed(const char *aMsgID, nsresult aStatus)
{
  nsTObserverArray<nsIMsgSendListener>::ForwardIterator iter(mExternalSendListeners);
  nsCOMPtr<nsIMsgSendListener> mExternalSendListener;
  while ((mExternalSendListener = iter.GetNext())) {
    mExternalSendListener->OnSendNotPerformed(aMsgID, aStatus);
  }
  return NS_OK;
}

I'm sure I'm doing somthing terribly stupid here because it totally crashes on the "while ((mExternalSendListener = iter.GetNext())) ..." Perhaps somebody could give me some pointers?

Comment 8

[Mike Schroepfer]

Per discussion in m.d.a.seamonkey minusing this for 1.9 - if it is a core bug and seamonkey or tbird need this in the 1.9 timeframe please do re-nominate.

Comment 9

[Ivo van Dongen]

I've been working on a patch and I think I've got it. I'll test it a bit more before submitting. I do think that this needs to be included in 1.9 as it's a seriously annoying bug.

Comment 10

[Ivo van Dongen]

Attachment: Patch involving the suggested modifications (nsTObserverArray and return types)

Comment 11

[timeless]

Comment on attachment 288503 [details] [diff] [review]
Patch involving the suggested modifications (nsTObserverArray and return types)

>+  if(mExternalSendListeners.AppendObserver(aMsgSendListener)) {

space before (

+  nsTObserverArray<nsIMsgSendListener>::ForwardIterator iter(mExternalSendListeners);
+  nsCOMPtr<nsIMsgSendListener> mExternalSendListener;

only class member (m) variables get the m prefix, so this should be:

nsCOMPtr<nsIMsgSendListener> externalSendListener;

(Both of these comments are general and apply more than once)

I think you can ask bienvenu for the next review.

Comment 12

[Ivo van Dongen]

Attachment: Cleaned patch

Comment 13

[David :Bienvenu]

I'll try running with this patch.

Comment 14

[David :Bienvenu]

Comment on attachment 289908 [details] [diff] [review]
Cleaned patch

this looks OK, except for the K&R braces style. I can't really test this since I don't have any external listeners, but it doesn't seem to break the normal case.

Comment 15

[David :Bienvenu]

Comment on attachment 289908 [details] [diff] [review]
Cleaned patch

please fix the braces style before landing this - braces should be on their own line, lined up with the if or while line - e.g.,
if (a)
{
  ...
}

Comment 16

[Ivo van Dongen]

Attachment: Same patch only with the braces like you wanted them

I've cleaned up the braces like you asked. Other than that the patch hasn't changed so I hope it can be committed now? I've build it and it still seems fine, but if another (super)review is needed please let me know.

Comment 17

[Ivo van Dongen]

Set the checkin-needed tag.

Comment 18

[David :Bienvenu]

if someone lands the last patch before I can get to it, please don't include the (unintentional?) idl whitespace change - I don't think we want to remove that blank line.

Comment 19

[Ivo van Dongen]

Attachment: Removed whitespace change

Removed the whitespace in the idl. Thanks for the quick reply btw.

Comment 20

[Reed Loden [:reed]]

Checking in mailnews/compose/src/nsMsgCompose.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v  <--  nsMsgCompose.cpp
new revision: 1.540; previous revision: 1.539
done
Checking in mailnews/compose/src/nsMsgCompose.h;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.h,v  <--  nsMsgCompose.h
new revision: 1.112; previous revision: 1.111
done

Comment 21

[Reed Loden [:reed]]

Backed out due to bustage.

../../../dist/include/xpcom/nsCOMPtr.h:736: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsQueryInterfaceWithError&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:744: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(nsGetServiceByCID) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:752: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsGetServiceByCIDWithError&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:760: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(nsGetServiceByContractID) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:768: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsGetServiceByContractIDWithError&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:776: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsCOMPtr_helper&) [with T = nsIMsgSendListener]
/builds/tinderbox/Tb-Trunk/Darwin_8.8.4_Depend/mozilla/mailnews/compose/src/nsMsgCompose.cpp: In member function 'virtual nsresult nsMsgCompose::OnSendNotPerformed(const char*, nsresult)':
/builds/tinderbox/Tb-Trunk/Darwin_8.8.4_Depend/mozilla/mailnews/compose/src/nsMsgCompose.cpp:3163: error: no match for 'operator=' in 'externalSendListener = iter. nsAutoTObserverArray<T, N>::ForwardIterator::GetNext [with T = nsIMsgSendListener, unsigned int N = 0u]()'
../../../dist/include/xpcom/nsCOMPtr.h:702: note: candidates are: nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsCOMPtr<T>&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:710: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(T*) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:719: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const already_AddRefed<T>&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:728: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(nsQueryInterface) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:736: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsQueryInterfaceWithError&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:744: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(nsGetServiceByCID) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:752: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsGetServiceByCIDWithError&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:760: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(nsGetServiceByContractID) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:768: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsGetServiceByContractIDWithError&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:776: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsCOMPtr_helper&) [with T = nsIMsgSendListener]
/builds/tinderbox/Tb-Trunk/Darwin_8.8.4_Depend/mozilla/mailnews/compose/src/nsMsgCompose.cpp: In member function 'virtual nsresult nsMsgCompose::OnGetDraftFolderURI(const char*)':
/builds/tinderbox/Tb-Trunk/Darwin_8.8.4_Depend/mozilla/mailnews/compose/src/nsMsgCompose.cpp:3176: error: no match for 'operator=' in 'externalSendListener = iter. nsAutoTObserverArray<T, N>::ForwardIterator::GetNext [with T = nsIMsgSendListener, unsigned int N = 0u]()'
../../../dist/include/xpcom/nsCOMPtr.h:702: note: candidates are: nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsCOMPtr<T>&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:710: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(T*) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:719: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const already_AddRefed<T>&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:728: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(nsQueryInterface) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:736: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsQueryInterfaceWithError&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:744: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(nsGetServiceByCID) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:752: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsGetServiceByCIDWithError&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:760: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(nsGetServiceByContractID) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:768: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsGetServiceByContractIDWithError&) [with T = nsIMsgSendListener]
../../../dist/include/xpcom/nsCOMPtr.h:776: note:                 nsCOMPtr<T>& nsCOMPtr<T>::operator=(const nsCOMPtr_helper&) [with T = nsIMsgSendListener]

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 295383 [details] [diff] [review]
Removed whitespace change

-  nsCOMArray<nsIMsgSendListener>            mExternalSendListeners;
+  nsTObserverArray<nsIMsgSendListener>      mExternalSendListeners;

...

+  if (mExternalSendListeners.AppendObserver(aMsgSendListener)) 
+  {
+    NS_ADDREF(aMsgSendListener);
+    return NS_OK;

If you want to hold a strong reference, then you could just do:

nsTObserverArray<nsCOMPtr<nsIMsgSendListener> > mExternalSendListeners;

and forget about the manual ref counting.

+  while ((externalSendListener = iter.GetNext())) 
+  {
+    externalSendListener->OnStartSending(aMsgID, aMsgSize);
+  }

This is wrong. The documentation says HasMore *MUST* precede a call to GetNext.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/glue/nsTObserverArray.h&rev=1.11&mark=300-302#281

Comment 23

[Ivo van Dongen]

Attachment: Added nsCOMPtr references and used HasMore

I've worked in Mark's suggestions. I hope it will do now. I've tested it myself with a simple plugin and it seems to work fine (no crashes).

Comment 24

[David :Bienvenu]

Comment on attachment 296541 [details] [diff] [review]
Added nsCOMPtr references and used HasMore

thx for the patch, looks OK to me, except that you don't need braces in the while loops where you use the iterator. I'll do an sr, and let Mark say for sure if that's what he had in mind.

Comment 25

[Ivo van Dongen]

My pleasure. I can fix the braces for you, bu I'll won't be in until Friday. Thanks so far.

Comment 26

[Mark Banner (:standard8)]

Comment on attachment 296541 [details] [diff] [review]
Added nsCOMPtr references and used HasMore

Almost there, but not quite right.

-  nsresult rv = mExternalSendListeners.AppendObject( aMsgSendListener );
-  return rv;
+
+  if (mExternalSendListeners.AppendElement(aMsgSendListener)) 
+  {
+    NS_ADDREF(aMsgSendListener);
+    return NS_OK;
+  }
+
+  return NS_ERROR_FAILURE;

As the template now includes a nsCOMPtr, you don't need the manual NS_ADDREF here (it'll work, but you're actually double counting), so you can just replace this with:

return mExternalSendListeners.AppendElement(aMsgSendListener) ? NS_OK : NS_ERROR_FAILURE;

Ditto with RemoveMsgSendListener.

+  nsTObserverArray<nsCOMPtr<nsIMsgSendListener> >::ForwardIterator iter(mExternalSendListeners);
+
+  while (iter.HasMore()) 
+  {
+    iter.GetNext()->OnStartSending(aMsgID, aMsgSize);
+  }

This is almost there, you need to keep the nsCOMPtr that you had in the original version. The reason being if nsMsgCompose has the last reference to a listener, and the reference removes itself in the callback function, then you may end up deleting the listener before it returns. So you want:

nsTObserverArray<nsIMsgSendListener>::ForwardIterator iter(mExternalSendListeners);
nsCOMPtr<nsIMsgSendListener> externalSendListener;

while (iter.HasMore())
{
  externalSendListener = iter.GetNext();
  externalSendListener->OnStartSending(aMsgID, aMsgSize);
}

Comment 27

[Ivo van Dongen]

Ok, thanks for the explanation. I'll fix this on Friday and make sure the braces are right this time around :).

Comment 28

[Ivo van Dongen]

Attachment: Refactored patch according to comment #26

I've worked in the changes Mark suggested. This seems to work much better indeed.

Comment 29

[Ivo van Dongen]

Attachment: A test extension for the sendlisteners

I've created a simple test extension if anybody wants to test the patch. It's also available at: https://ops.svn.sourceforge.net/svnroot/ops/trunk/etc/Thunderbird Patches in case you want to build it for yourself (using ant).

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 299150 [details] [diff] [review]
Refactored patch according to comment #26

Ok, this looks good now. Thanks for the updates.

I just want to check David is happy, so setting sr request on him.

Comment 31

[Mark Banner (:standard8)]

(In reply to comment #29)
> I've created a simple test extension if anybody wants to test the patch. It's
> also available at:
> https://ops.svn.sourceforge.net/svnroot/ops/trunk/etc/Thunderbird Patches in
> case you want to build it for yourself (using ant).

If you want to, you can create a slightly more permanent test that we can use in our automated testing process. I'll give you some background and leave it up to you. In any case, thanks for your work on fixing this bug.

Here's some background:

http://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests

There is one example already in the source code here:

http://mxr.mozilla.org/seamonkey/source/mailnews/base/test/unit/test_nsMsgMailSession_Listeners.js

and another example in attachment 298983 [details] [diff] [review] (search for the test_nsAbManager1.js file, for reference its on bug 410158) currently waiting final review.

These two examples examples are slightly more complicated that the nsMsgCompose cases, but should help you with some ideas.

Also http://mxr.mozilla.org/seamonkey/source/mailnews/compose/test/unit/test_nsMsgCompose1.js currently does a very basic test with a very small part of the nsMsgCompose functionality, but shows how to create and initialise the nsMsgCompose instance for these tests.

If you do write a test, please do it as test_nsMsgCompose2.js as I'm just about to fill the existing one up with my patch on bug 413230.

Comment 32

[David :Bienvenu]

Comment on attachment 299150 [details] [diff] [review]
Refactored patch according to comment #26

thx for the patch!

Comment 33

[Mark Banner (:standard8)]

(In reply to comment #32)
> (From update of attachment 299150 [details] [diff] [review])
> thx for the patch!
> 
Ivo, do you want me to check this in on your behalf?

Comment 34

[Ivo van Dongen]

If (In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 299150 [details] [diff] [review] [details])
> > thx for the patch!
> > 
> Ivo, do you want me to check this in on your behalf?
> 
If you would be so kind. I'll get back to you on the tests. At the moment I'm running a bit out of time, so I'll have to discuss it with my employer first. Thanks so far for all the help.

Comment 35

[Mark Banner (:standard8)]

Patch checked in on Ivo's behalf:

Checking in mailnews/compose/src/nsMsgCompose.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v  <--  nsMsgCompose.cpp
new revision: 1.547; previous revision: 1.546
done
Checking in mailnews/compose/src/nsMsgCompose.h;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.h,v  <--  nsMsgCompose.h
new revision: 1.116; previous revision: 1.115

Note on bustage fix to add back in nsCOMArray.h (I checked in a patch between this one being reviewed and now which caused nsCOMArray.h to be required).

Bug is now fixed. Thanks for the patch.

Comment 36

[Mark Banner (:standard8)]

Attachment: xpcshell testcase

I've put together a test case for this, based on some of the others we've got around.

Comment 37

[Mark Banner (:standard8)]

(In reply to comment #36)
> Created an attachment (id=305025) [details]
> xpcshell testcase

Checked in.