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.

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 <bzbarsky@mit.edu> which was bz's last fix for this error.

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.

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.

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

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.

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.

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?

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.

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 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.

I'll try running with this patch.

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 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) { ... }

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.

Set the checkin-needed tag.

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.

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

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

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 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

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 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.

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

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); }

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

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

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 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.

(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 on attachment 299150 [details] [diff] [review] Refactored patch according to comment #26 thx for the patch!

(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?

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.

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.

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

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