Thunderbird crashes upon displaying specific message

This bug was filed from the Socorro interface and is
report bp-251126bf-1124-43b5-bd29-656872120202 .
=============================================================

#14 crash currently for version 10
regression of Bug 681188 - Switch from PRBool to bool ?

0 xul.dll xul.dll@0x8469f1
1 xul.dll `anonymous namespace'::SyncRunnable5<nsIImapMailFolderSink,nsIImapProtocol*,nsIMsgMailNewsUrl*,bool,bool,unsigned int>::Run mailnews/imap/src/nsSyncRunnableHelpers.cpp:282
2 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:631
3 xul.dll NS_ProcessNextEvent_P objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:245
4 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:134
5 xul.dll MessageLoop::RunInternal ipc/chromium/src/base/message_loop.cc:208
6 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:201
7 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:175
8 xul.dll nsBaseAppShell::Run widget/src/xpwidgets/nsBaseAppShell.cpp:189
9 xul.dll nsAppShell::Run widget/src/windows/nsAppShell.cpp:257
10 xul.dll nsAppStartup::Run toolkit/components/startup/nsAppStartup.cpp:228

As a topcrash regression, we should make an effort to resolve this for a near term release.

most comments cite doing imap folder subscriptions.
bp-62b0a6a5-2a42-4787-80c0-5be9f2120221 cites dataloss
"trying to subscribe to folders which Thuinderburd keeps losing in the subscription list total pain in the butt and a game breaker as far as I'm concerned"

Mac and linux @ SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run

I haven't been able to reproduce this. A protocol log might be helpful.

not a straight up regression of Bug 681188 - Switch from PRBool to bool - given that it landed in v10?

I suspect this would have come from switching to sync runnable, i.e., getting rid of xpcom proxies in Bug 675407

re: testcase

several crash reporters have been contacted. however, we should not count on finding a testcase from this population of users - historically it is difficult to impossible to get a cooperating victim

doesn't seem to be a persistent crash - only 1 in 4 people replied to me, and of those only 1 in 5 can reproduce.

ludo you have just gotten an email with screen shot for that person who can reproduce

I'm pretty sure this is caused by a null folder sink. The sync runnable changes just changed the stack signature, but I couldn't be surprised if the crash was around before (hard to know what the old stack would have looked like, since it's on the other side of the event loop now). It's very hard to see where subscribe UI would end up with a null folder sink that we're not checking for.

Oh, I should say, pretty sure this is a call to SetUrlState

from one of the crashes, this is the call site:

http://hg.mozilla.org/releases/comm-release/annotate/6ccee99d387c/mailnews/imap/src/nsImapProtocol.cpp#l1602

Since we're checking for m_imapMailFolderSink is null before calling this, we might have some sort of race condition, if I'm understanding the crash info correctly.

also
@0x0 | nsImapMailFolder::SetUrlState(nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int)
and
nsImapMailFolder::SetUrlState(nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int)

*** Bug 763888 has been marked as a duplicate of this bug. ***

bienvenu,

bp-40cf7199-6e99-4ac0-ab7f-3f81c2120717 is a user who is crashing to beat the band - many times a day. Mostly this signature, but also a smattering of others. like ...

nsRefPtr<nsCanvasPatternAzure>::~nsRefPtr<nsCanvasPatternAzure>() | nsMsgFilterAfterTheFact::~nsMsgFilterAfterTheFact()
bp-8547793c-5222-4166-b3c1-481402120717

mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | ToNewUnicode(nsACString_internal const&)
bp-5b2f0c03-4543-48de-a895-61fb22120717

Collect
bp-075b305d-75c3-4637-8b5c-6ad582120718

#23 crash TB14

TB14 signature xul.dll@0x85be1d | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run()

It looks like we just had somebody report this crash in Launchpad too:

https://launchpad.net/bugs/1052306

According to the bug report data, the reporter has just submitted https://crash-stats.mozilla.com/report/index/bp-2db3eea4-c46d-4d64-8b6e-e47f92120918, and they say that the crash happens on a specific mail

Created attachment 664374
possible fix

Comment on attachment 664374
possible fix

mReceiver seems to be null. We should not pass null to constructor of ImapMailFolderSinkProxy.

Comment on attachment 664374
possible fix

I'm going to pass this review on to :bienvenu since he has been intimately involved in the discussions so far. If you don't get a review in the next week or so, feel free to pass it back to me (or irving or standard8) and we'll figure something out.

Comment on attachment 664374
possible fix

could you review this instead of bienvenu?

Comment on attachment 664374
possible fix

This is probably fine, but Irving is a bit closer to the imap code than I am at the moment.

Comment on attachment 664374
possible fix

Review of attachment 664374:
-----------------------------------------------------------------

Looks OK to me; there's a lot of code that changes behaviour if m_imapMailFolderSink is null so we'll need to keep an eye on trunk in case this moves the crash somewhere else.

https://hg.mozilla.org/comm-central/rev/2a62a0c386a8

Comment on attachment 664374
possible fix

[Approval Request Comment]
Regression caused by (bug #): bug 675407
User impact if declined: crash. Actually, this is top 17 crash on 16.0.1.
Testing completed (on c-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): low. This adds nullptr check. also, m_imapMailFolderSink has already nullptr check.

comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/5629f96d8cd4
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/3cb3fc93b072

Tb15.0.1 signature xul.dll@0x8c0cdb | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run()

crash stats shows most crashing has stopped.
less than a dozen crashes in past weeks for 17.0 for `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run()
bp-a7116160-4238-440b-ba41-4f3952130129 17.0.2
bp-633908f4-4d37-414c-bddd-953cc2130105 17.0