Last Comment Bug 723888 - crash SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol* ... typically working with folder subscribe
: crash SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol* ... typically wor...
Status: VERIFIED FIXED
[dataloss?][regression:tb10]
: crash, regression, testcase-wanted, topcrash
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: x86 Windows NT
: -- critical (vote)
: Thunderbird 19.0
Assigned To: Makoto Kato [:m_kato]
:
Mentors:
: 763888 (view as bug list)
Depends on:
Blocks: 675407
  Show dependency treegraph
 
Reported: 2012-02-03 04:14 PST by Wayne Mery (:wsmwk, NI for questions)
Modified: 2014-03-12 08:54 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
[@ xul.dll@0x8469f1 | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run() ]
[@ SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run ]
[@ xul.dll@0x846a42 | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run() ]
[@ xul.dll@0x846d41 | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run() ]
[@ xul.dll@0x83490f | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run() ]
[@ xul.dll@0x8349e9 | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run() ]
[@ xul.dll@0x8387b8 | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run() ]
[@ nsImapMailFolder::SetUrlState(nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int) ]
[@ @0x0 | nsImapMailFolder::SetUrlState(nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int) ]
[@ xul.dll@0x84d734 | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run() ]
[@ xul.dll@0x85be1d | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run() ]
[@ xul.dll@0x902c65 | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run() ]
[@ xul.dll@0x8c0cdb | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run() ]
QA Whiteboard:
Iteration: ---
Points: ---
-
-
fixed
fixed


Attachments
possible fix (1.76 KB, patch)
2012-09-24 23:36 PDT, Makoto Kato [:m_kato]
irving: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Wayne Mery (:wsmwk, NI for questions) 2012-02-03 04:14:04 PST
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, NI for questions) 2012-03-06 19:07:07 PST
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 2012-03-06 21:19:34 PST
I haven't been able to reproduce this. A protocol log might be helpful.
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2012-03-07 05:08:36 PST
not a straight up regression of Bug 681188 - Switch from PRBool to bool - given that it landed in v10?
Comment 4 David :Bienvenu 2012-03-07 08:45:56 PST
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, NI for questions) 2012-03-14 05:31:35 PDT
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, NI for questions) 2012-04-03 11:06:40 PDT
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 2012-04-04 09:39:06 PDT
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 2012-04-04 09:51:31 PDT
Oh, I should say, pretty sure this is a call to SetUrlState
Comment 9 David :Bienvenu 2012-04-09 06:51:37 PDT
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, NI for questions) 2012-05-25 13:33:18 PDT
also 
@0x0 | nsImapMailFolder::SetUrlState(nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int) 
and
nsImapMailFolder::SetUrlState(nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int)
Comment 11 David :Bienvenu 2012-06-12 08:00:19 PDT
*** Bug 763888 has been marked as a duplicate of this bug. ***
Comment 12 Wayne Mery (:wsmwk, NI for questions) 2012-07-18 21:10:49 PDT
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, NI for questions) 2012-08-22 04:31:15 PDT
#23 crash TB14

TB14 signature xul.dll@0x85be1d | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run()
Comment 14 Chris Coulson 2012-09-18 00:59:38 PDT
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] 2012-09-24 23:36:38 PDT
Created attachment 664374 [details] [diff] [review]
possible fix
Comment 16 Makoto Kato [:m_kato] 2012-09-24 23:38:33 PDT
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) 2012-09-25 07:26:52 PDT
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] 2012-10-11 19:08:21 PDT
Comment on attachment 664374 [details] [diff] [review]
possible fix

could you review this instead of bienvenu?
Comment 19 Mark Banner (:standard8) 2012-10-22 15:45:33 PDT
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) 2012-10-25 08:22:12 PDT
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] 2012-10-31 01:24:15 PDT
https://hg.mozilla.org/comm-central/rev/2a62a0c386a8
Comment 22 Makoto Kato [:m_kato] 2012-11-01 19:41:43 PDT
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) - (needinfo me!) 2012-11-02 11:18:44 PDT
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, NI for questions) 2012-11-04 06:44:50 PST
Tb15.0.1 signature xul.dll@0x8c0cdb | `anonymous namespace''::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, unsigned int>::Run()
Comment 25 Wayne Mery (:wsmwk, NI for questions) 2013-01-30 18:53:16 PST
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

Note You need to log in before you can comment on or make changes to this bug.