723888 - crash SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol* ... typically working with folder subscribe

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

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

Comment 1

[Wayne Mery (:wsmwk)]

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

Comment 2

[David :Bienvenu]

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

Comment 3

[Wayne Mery (:wsmwk)]

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

Comment 4

[David :Bienvenu]

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

Comment 5

[Wayne Mery (:wsmwk)]

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

Comment 6

[Wayne Mery (:wsmwk)]

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

Comment 7

[David :Bienvenu]

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.

Comment 8

[David :Bienvenu]

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

Comment 9

[David :Bienvenu]

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.

Comment 10

[Wayne Mery (:wsmwk)]

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

Comment 12

[Wayne Mery (:wsmwk)]

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

Comment 13

[Wayne Mery (:wsmwk)]

#23 crash TB14

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

Comment 14

[Chris Coulson]

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

Comment 15

[Makoto Kato [:m_kato]]

Attachment: possible fix

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 664374 [details] [diff] [review]
possible fix

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

Comment 17

[Kent James (:rkent)]

Comment on attachment 664374 [details] [diff] [review]
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 18

[Makoto Kato [:m_kato]]

Comment on attachment 664374 [details] [diff] [review]
possible fix

could you review this instead of bienvenu?

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 664374 [details] [diff] [review]
possible fix

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

Comment 20

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 664374 [details] [diff] [review]
possible fix

Review of attachment 664374 [details] [diff] [review]:
-----------------------------------------------------------------

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.

Comment 21

[Makoto Kato [:m_kato]]

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

Comment 22

[Makoto Kato [:m_kato]]

Comment on attachment 664374 [details] [diff] [review]
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.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

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

Comment 24

[Wayne Mery (:wsmwk)]

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

Comment 25

[Wayne Mery (:wsmwk)]

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