1332606 - startup crash in [thunk]:nsIImapMailFolderSink::`vcall''{148, {flat}}'' }'' via `anonymous namespace'::SyncRunnable1

Status: RESOLVED DUPLICATE of bug 1335638

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

#2 crash for 51.0b2.  Average 3 crashes per user for both 51.0b1 and 51.0b2. But not many users reporting - less than 0.5%. Still, might be a v52 blocker.

Only seen on beta, so don't have a regression range, and do not know whether this will happen for v52.   Perhaps regressed  by bug 1299116 or bug 870864?

bp-1be49a6e-6718-47b0-99dd-2f0e42170117.
0 	xul.dll	[thunk]:nsIImapMailFolderSink::`vcall'{148, {flat}}' }'	
1 	xul.dll	`anonymous namespace'::SyncRunnable1<nsIImapServerSink, nsACString_internal&>::Run	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsSyncRunnableHelpers.cpp:118
2 	xul.dll	`anonymous namespace'::DispatchSyncRunnable	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsSyncRunnableHelpers.cpp:269
3 	xul.dll	ImapServerSinkProxy::GetOriginalUsername(nsACString_internal&)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsSyncRunnableHelpers.cpp:451
4 	xul.dll	nsImapProtocol::GetImapUserName()	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapProtocol.cpp:606
5 	xul.dll	nsImapProtocol::CanHandleUrl(nsIImapUrl*, bool*, bool*)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapProtocol.cpp:2302
6 	xul.dll	nsImapIncomingServer::GetImapConnection(nsIImapUrl*, nsIImapProtocol**)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapIncomingServer.cpp:742
7 	xul.dll	nsImapIncomingServer::GetImapConnectionAndLoadUrl(nsIImapUrl*, nsISupports*)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapIncomingServer.cpp:443
8 	xul.dll	nsImapMockChannel::ReadFromImapConnection()	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapProtocol.cpp:9293
9 	xul.dll	mozilla::net::CacheEntry::InvokeAvailableCallback(mozilla::net::CacheEntry::Callback const&)	netwerk/cache2/CacheEntry.cpp:889 


Note, there are other [thunk]:nsIImapMailFolderSink::`vcal crashes, but these also exist on 45.x. For example [thunk]:nsIImapMailFolderSink::`vcall''{56, {flat}}'' }''  https://crash-stats.mozilla.com/signature/?product=Thunderbird&signature=%5Bthunk%5D%3AnsIImapMailFolderSink%3A%3A%60vcall%27%27%7B56%2C%20%7Bflat%7D%7D%27%27%20%7D%27%27&date=%3E%3D2016-12-20T13%3A06%3A13.000Z&date=%3C2017-01-20T13%3A06%3A13.000Z

Comment 1

[Jorg K (CEST = GMT+2)]

Looking at
https://crash-stats.mozilla.com/report/index/cfad9a3b-3c3d-4a35-9f43-0b0522170119
https://crash-stats.mozilla.com/report/index/bb5324a7-325e-41e5-a9ab-7a0fd2170120

crashes is in
mailnews/imap/src/nsSyncRunnableHelpers.cpp:118

  NS_IMETHOD Run() {
    mResult = (mReceiver->*mMethod)(mArg1); <=== 118
    mozilla::MonitorAutoLock(mMonitor).Notify();
    return NS_OK;
  }

Should we check mReceiver and *mMethod? Kent?

Comment 2

[Kent James (:rkent)]

The runnable is on a different thread than the caller, and so any errors are not going to get back to the caller. Most of these uses don't check error return.

So, what to do? We really need to find the root cause of this. Here is what I would suggest.

Take the places where SyncRunnable1 is used, and add the checks there. What check? We really need to figure out where the crash is occurring, and we need a stack trace to get that. I came across an interesting macro yesterday which I believe is for this:

MOZ_DIAGNOSTIC_ASSERT

that IIUC crashes in aurora and nightly. Try that.

So what you would do is to add the check upstream one call, which is to NS_SYNCRUNNABLEMETHOD1 and NS_SYNCRUNNABLEATTRIBUTE. Use MOZ_DIAGNOSTIC_ASSERT but also return an error (which will mostly be ignored) on release builds.

Comment 3

[Wayne Mery (:wsmwk)]

FWIW see also bug 723888

Comment 4

[Wayne Mery (:wsmwk)]

#2 for 52.0b1. But at present time, probably not a blocker for release. Still, it's startup crash.

I forgot kent had already commented here - so jorg has something in bug 1335578 comment 3.

For now, presuming these have the same root cause, so setting  bug 1335638 and bug 1335578 as dependent.

Comment 5

[Jorg K (CEST = GMT+2)]

Kent, can you help us out here. Sadly I can't find the time and you already have a plan (comment #2).

Comment 6

[Wayne Mery (:wsmwk)]

If bug 1328814 caused this, then we should back it out from esr and beta until a proven solution is found, because what was gained for bug 1328814 is not worth the disruption.
https://crash-stats.mozilla.com/topcrashers/?product=Thunderbird&version=45.7.0
https://crash-stats.mozilla.com/topcrashers/?product=Thunderbird&version=52.0b1 (the 3 crash signatures taken together exceed the #1 ranking crash)

Comment 7

[Jorg K (CEST = GMT+2)]

Well, all that the patch did was ignore one error status
https://hg.mozilla.org/comm-central/rev/f3c3430dddfc3e15ede7d7d98abc1943a0ed1aef
of finding an IMAP folder which no longer existed because it was renamed or moved.

I don't quite understand how that causes crashes elsewhere. But we can back it out. That's software development by statistics ;-(

Comment 8

[Jorg K (CEST = GMT+2)]

Further to comment #7: The code change only kicks in where we deal with IMAP URIs pointing to folders that don't exist, something that happens in a rename or move situation.

The best way forward here would be to find a user who is prepared to use Daily for a while. If that crashes, I can supply a special Daily with that change backed out. If it don't crash any more, we found the problem. If it keeps crashing, we need to go looking elsewhere.

Comment 9

[Wayne Mery (:wsmwk)]

(In reply to Jorg K (GMT+1) from comment #8)
> 
> The best way forward here would be to find a user who is prepared to use
> Daily for a while. If that crashes, I can supply a special Daily with that
> change backed out. If it don't crash any more, we found the problem. If it
> keeps crashing, we need to go looking elsewhere.

Good idea. Attempts have been made in the past week to engage users. I have 3 solid contacts so far, but working with users is a very slow process - so it takes many days to get data for bug reports.  The data can also be hard to navigate because the nightly builds change over time, so whichever backed out patch(s) you feel is best to test, now is the time to create it so testing can proceed.

gsmith is a user also sees vcall''{100, {flat}}. In the last hour he reports:
* 4 nightly builds I had him test do not crash. A) 2016-12-10, 2016-12-09, B) 2016-09-01, 2016-08-31 - ATM I don't recall why I picked those but it was related to checkins in those time gaps
* for 2016-12-10, 2016-12-09 he sees "Daily has blocked a file from loading into this message". For the other earlier two builds the image isn't offered for display.
* 52.0b1 crashes

Comment 10

[Wayne Mery (:wsmwk)]

bug 1335638 comment 3 is good description from user

Comment 11

[Jorg K (CEST = GMT+2)]

Sure, "Daily has blocked a file from loading into this message" is Magnus' new compose Windows magic of blocking file:// based imaged.

Bug 1328814 landed on 2017-01-10 08:04 +0000. So get him to test the the Daily from the day before and the one from that day. You can confirm that you're doing the right thing by renaming an IMAP folder. On the older Daily that will destroy the preview pane (as you well know), on the newer Daily the preview pane will remain working. If the older one doesn't crash, but the newer one does, then I'll believe you.

No need for a specially compiled version.

Comment 12

[Kent James (:rkent)]

I'm proposing we add a diagnostic in bug 1336641 that should allow us to get better feedback on aurora and nightly of this type of crash.

For windows users running TB 45 who can reproduce this, I've done a build of tb45 that backs out a patch Wayne is suspicious of. Try https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-esr45-win32/1486164728/thunderbird-45.2.0.en-US.win32.installer.exe and see if that makes the issue go away.

Comment 13

[Jorg K (CEST = GMT+2)]

This was caused by bug 1328814 and will be fixed in 45.7.1 by backing out that bug.

Other versions will be fixed differently, see bug 1335638. I'm closing this now since we have four open bugs for the same issue which is well understood.