Open Bug 558452 Opened 13 years ago Updated 9 months ago

crash [@ nsImapProtocol::SetupWithUrl ][@ nsImapProtocol::SetupWithUrl] with null m_runningUrl - aURL isn't nsIImapUrl

Categories

(MailNews Core :: Networking: IMAP, defect)

defect

Tracking

(Not tracked)

People

(Reporter: wsmwk, Unassigned)

References

Details

(Keywords: crash, testcase-wanted, Whiteboard: [patchlove][needs investigation of threading])

Crash Data

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #507088 +++

bp-49c34c1a-a481-46e7-a00d-7362b2100105 (phorton)
0	thunderbird.exe	nsImapProtocol::SetupWithUrl	 mailnews/imap/src/nsImapProtocol.cpp:742
1	thunderbird.exe	nsImapProtocol::LoadImapUrl	mailnews/imap/src/nsImapProtocol.cpp:1979
2	thunderbird.exe	nsImapIncomingServer::LoadNextQueuedUrl	mailnews/imap/src/nsImapIncomingServer.cpp:537
3	xpcom_core.dll	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
4	xpcom_core.dll
5	xpcom_core.dll	nsProxyObjectCallInfo::Run	xpcom/proxy/src/nsProxyEvent.cpp:181 

no crashes found with comments
nsresult nsImapProtocol::SetupWithUrl(nsIURI * aURL, nsISupports* aConsumer)
742 m_runningUrl->GetMockChannel(getter_AddRefs(m_mockChannel)); 

m_runningUrl is null
Would appear that way, but how could it when we have this a few lines up

711 m_runningUrl = do_QueryInterface(aURL, &rv);
712 if (NS_FAILED(rv)) return rv;
well, either the code is threaded, or one of the intermediate lines calls something interesting or calls out to something which reenters something interestng.
So I got that one after a week-end where I had left the computer sleeping.
Again on waking the computer up !!
Ludo, do you have a new crash-stat link?
Duplicate of this bug: 582165
also @0x0 | nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*) on windows? bp-9cb5f8db-453d-4357-a677-501502100723
Summary: crash [@ nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*)] → crash [@ nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*)] (windows), [@ nsImapProtocol::SetupWithUr] (Mac),
Wayne, Bug 582165 is difference crash even if crash function is same.
Summary: crash [@ nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*)] (windows), [@ nsImapProtocol::SetupWithUr] (Mac), → crash [@ nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*)][@ nsImapProtocol::SetupWithUrl] with null m_runningUrl - aURL isn't nsIImapUrl
Crash Signature: [@ nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*)] [@ nsImapProtocol::SetupWithUrl]
I just crashed bp-ec6aa5ee-496c-444f-a6a4-d1ad72120308 but no protocol log :(
Crash Signature: [@ nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*)] [@ nsImapProtocol::SetupWithUrl] → [@ nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*)] [@ nsImapProtocol::SetupWithUrl]
~top 140 crash.  I think a good bug to revisit after TB17 releases.


(In reply to Ludovic Hirlimann [:Usul] from comment #4)
> So I got that one after a week-end where I had left the computer sleeping.

bp-29a89b11-a814-4a5d-9468-d57ca2121031 says same thing - coming back from sleep. 
my comment 11 (20120302030054 13.0a1) probably also was after wake from sleep.
both point at  m_runningUrl->GetMockChannel  (ref: comment 1 and comment 2)


perhaps a different crash - bp-e759bab7-fc5f-4934-a293-734392121028  and bp-a9bf02a8-716d-4a8f-9ecc-bb6272121110 hit a different area of code -- http://hg.mozilla.org/releases/comm-release/annotate/ddf904aa1957/mailnews/imap/src/nsImapProtocol.cpp#l8085
  8085  return m_imapProtocolSink->GetUrlWindow(mailnewsUrl, aMsgWindow);
in nsImapProtocol::GetMsgWindow.  Which, coincidentally, has crashes with (rare) signature nsImapProtocol::GetMsgWindow(nsIMsgWindow**) as in bp-24d57332-6420-4727-b64b-146e32121110 and nsImapProtocol::GetMsgWindow as in bp-70a52ef7-0b50-4dc3-915c-3e2a22121106
Attached patch proposed fixSplinter Review
There's already plenty of if (m_runningUrl) checks in the file, also later in the same method...
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #680458 - Flags: review?(irving)
Comment on attachment 680458 [details] [diff] [review]
proposed fix

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +673,5 @@
>      }
>      nsCOMPtr<nsIImapIncomingServer> imapServer = do_QueryInterface(server);
>      nsCOMPtr<nsIStreamListener> aRealStreamListener = do_QueryInterface(aConsumer);
> +    if (m_runningUrl)
> +      m_runningUrl->GetMockChannel(getter_AddRefs(m_mockChannel));

Would it make more sense to exit early (around line 644) if m_runningUrl is null? As far as I can tell, a null pointer here implies we're trying to run a non-IMAP URL, which doesn't make sense.
Attachment #680458 - Flags: review?(irving) → review-
Comment on attachment 680458 [details] [diff] [review]
proposed fix

I think that's done already, no? (rv would be failed). Otherwise we'd at least crash with mailnewsUrl - http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#644

So something is happening in between that nulls out m_runningURL - i don't know the larger picture, but i assume it's a threading issue.
Attachment #680458 - Flags: review- → review?(irving)
(In reply to Magnus Melin from comment #15)
> Comment on attachment 680458 [details] [diff] [review]
> proposed fix
> 
> I think that's done already, no? (rv would be failed). Otherwise we'd at
> least crash with mailnewsUrl -
> http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.
> cpp#644
> 
> So something is happening in between that nulls out m_runningURL - i don't
> know the larger picture, but i assume it's a threading issue.

Can you look into where else in the code we mess with m_runningURL and why, to see if we can understand and/or prevent the race condition?
Well there is at least a nulling out in nsImapProtocol::ReleaseUrlState http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#973

But other than that, i can't really tell.
Comment on attachment 680458 [details] [diff] [review]
proposed fix

David, I can't make sense of the threading in this bug. Can you take a look at it?
Attachment #680458 - Flags: review?(irving) → review?(mozilla)
Flags: needinfo?(mozilla)
First time ever I got it. But right before getting https://crash-stats.mozilla.com/report/index/7ce8f2d8-8930-43b6-90f7-034d92131202 this was happening:

2013-12-02 16:25:44.464719 UTC - -2019558592[7634da80]: ReadNextLine [stream=765b0220 nb=10 needmore=0]
2013-12-02 16:25:44.464740 UTC - -2019558592[7634da80]: 888d8800:imap.gmail.com:S-[Gmail]/Stellarium:CreateNewLineFromSocket: + idling
2013-12-02 16:35:17.204418 UTC - -1220475136[b7222240]: proposed url = INBOX folder for connection INBOX has To Wait = FALSE can run = TRUE
2013-12-02 16:35:17.204692 UTC - 1968171840[71372c00]: 713de000:imap.gmail.com:S-INBOX:SendData: DONE
2013-12-02 16:35:17.205045 UTC - -1220475136[b7222240]: proposed url = [Gmail]/Stellarium folder for connection INBOX has To Wait = FALSE can run = FALSE
2013-12-02 16:35:17.205103 UTC - -1220475136[b7222240]: proposed url = [Gmail]/Stellarium folder for connection [Gmail]/Stellarium has To Wait = FALSE can run = TRUE
2013-12-02 16:35:20.000033 UTC - 1968171840[71372c00]: ReadNextLine [stream=71370460 nb=34 needmore=0]
2013-12-02 16:35:20.000064 UTC - 1968171840[71372c00]: 713de000:imap.gmail.com:S-INBOX:CreateNewLineFromSocket: 252 OK IDLE terminated (Success)
2013-12-02 16:35:20.000145 UTC - 1968171840[71372c00]: 713de000:imap.gmail.com:S-INBOX:ProcessCurrentURL: entering
2013-12-02 16:35:20.000197 UTC - 1968171840[71372c00]: 713de000:imap.gmail.com:S-INBOX:ProcessCurrentURL:imap://merikes%40gmail%2Ecom@imap.gmail.com:993/select%3E/INBOX:  = currentUrl

Also, right after crashing netstat had 5 established and 1 close_wait connection to la-in-f108.1e100.net:imaps and one imap2 established connection to a non-google host. And given the time it happened, I was accessing my google apps email account and marking email read/unread on my Android phone at the same time.
My crash of version 24.3.0: bp-fdd983bd-b3ce-4719-a617-77c122140220

Always-on desktop, no wake up from sleep involved. No protocol logs, and the event happened while I was away from screen, so no user-interaction. Thunderbird is polling IMAP and using IDLE.
Had another crash overnight: bp-ae8a8471-9bb2-48c8-bd0f-a6ccd2140221.

Having not had crashes of this signature before, I'll note that my Thunderbird configuration changed recently with the addition of another IMAP server.
I've had a few more crashes. For the most recent, bp-ae42adde-4537-49ee-b19e-dbf3e2140227, I also have a protocol log (IMAP:5,ImapAutoSync:5,timestamp) from start to crash of 187,260,369 bytes (22039034 gzipped), covering about 5 hours.

I'm reticent to attach the log to the bug (plus it may be too large), but can make it available to a developer.
you can probably send the log to magnus who's working on this if he needs it.
(In reply to :Irving Reid from comment #18)
> Comment on attachment 680458 [details] [diff] [review]
> proposed fix
> 
> David, I can't make sense of the threading in this bug. Can you take a look
> at it?

Irving, Bienvenu isn't going to touch this unless it's absolutely essential and we contact him outside of bugzilla.  Do you want to PM bienvenu, or do we have someone (rkent?) who might be able to sort it out?
Flags: needinfo?(mozilla) → needinfo?(irving)
magnus, did you get Phil's protocol log?

FWIW, Phil is still crashing recently ... bp-8ff9fbe2-0e67-4f06-aa43-07cd82140825 bp-1b36d76c-de93-45c7-bc42-62e5c2140828
Flags: needinfo?(mkmelin+mozilla)
Not that I recall, but I don't think it would help much.
Flags: needinfo?(mkmelin+mozilla)
I did not make a log available. I can, but the most recent one I have is from version 24.3.0; let me know if that's usable. I've enabled logging again to collect more recent data.  FYI, I've been logging with

NSPR_LOG_MODULES="IMAP:5,ImapAutoSync:5,timestamp"

if you need different settings, let me know. I'm also willing to run a debug build.
Wayne, at this point I'll leave it to Magnus or whoever else has time to investigate.
Flags: needinfo?(irving)
FWIW, Phil's crashes account for 25% of crash reports with an email address. :)
wirelessgregg has another 25%.
Whiteboard: [needs investigation of threading]
Comment on attachment 680458 [details] [diff] [review]
proposed fix

This will need a different reviewer
Attachment #680458 - Flags: review?(mozilla)
I had two crashes this morning against version 31.1.2: crash bp-74f8232d-ed82-4d5a-91e9-ef5c12140929 and crash bp-0b0868e4-a013-4bcb-a115-77f682140929. Both have logs that I can make available (5MB, 1.5MB).
Assignee: mkmelin+mozilla → nobody
Status: ASSIGNED → NEW
Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.
(In reply to Phil Pishioneri from comment #31)
> I had two crashes this morning against version 31.1.2: crash
> bp-74f8232d-ed82-4d5a-91e9-ef5c12140929 and crash
> bp-0b0868e4-a013-4bcb-a115-77f682140929. Both have logs that I can make
> available (5MB, 1.5MB).

PHil, are you still seeing crashes like two years ago?
From you in the last 2 months I see only 
bp-9290b418-1bd9-4346-aeb1-f345e2151201	2015-12-01 22:26:51 	PORT_ArenaAlloc_Util | nssTrust_GetCERTCertTrustForCert | stan_GetCERTCertificate   
bp-318c0a58-fe55-487a-859c-2003a2160128	2016-01-28 14:56:50 	shutdownhang | libsystem_kernel.dylib@0x16eb2
Flags: needinfo?(pgp)
I have not seen the crash in quite a long time. That time may have been when I switched to a different IMAP service: but whether that's why or not, I don't get them.
Flags: needinfo?(pgp)
Depends on: 825513
afaict no change over last 6  months, and in any v60 version, despite patches in bug 825513 and Bug 1507718 - crash in nsImapProtocol::GetMessageSize via nsIMAPBodyShell::Generate. m_hostSessionList is stale

not much hope for a testcase.
sampling 10 crashes from 60.4.0 - two have McAfee installed, one bitdefender.  Of the remaining crashes 
* two haven't had othe rrecent crashes (previous crashes 7 weeks ago and one year)
* hard to say if there is a common theme in the stacks:

- three emanate from nsImapMailFolder::InitiateAutoSync bp-3a96030a-f232-4246-a080-062ce0190105
 0 	xul.dll	nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*)	comm/mailnews/imap/src/nsImapProtocol.cpp:723
1 	xul.dll	nsImapProtocol::LoadImapUrl(nsIURI*, nsISupports*)	comm/mailnews/imap/src/nsImapProtocol.cpp:2151
2 	xul.dll	nsImapIncomingServer::GetImapConnectionAndLoadUrl(nsIImapUrl*, nsISupports*)	comm/mailnews/imap/src/nsImapIncomingServer.cpp:455
3 	xul.dll	nsImapService::GetImapConnectionAndLoadUrl(nsIImapUrl*, nsISupports*, nsIURI**)	comm/mailnews/imap/src/nsImapService.cpp:2227
4 	xul.dll	nsImapService::FolderCommand(nsIMsgFolder*, nsIUrlListener*, char const*, int, nsIMsgWindow*, nsIURI**)	comm/mailnews/imap/src/nsImapService.cpp:1511
5 	xul.dll	nsImapService::UpdateFolderStatus(nsIMsgFolder*, nsIUrlListener*, nsIURI**)	comm/mailnews/imap/src/nsImapService.cpp:1563
6 	xul.dll	nsImapMailFolder::UpdateStatus(nsIUrlListener*, nsIMsgWindow*)	comm/mailnews/imap/src/nsImapMailFolder.cpp:1420
7 	xul.dll	nsImapMailFolder::InitiateAutoSync(nsIUrlListener*)	comm/mailnews/imap/src/nsImapMailFolder.cpp:9575
8 	xul.dll	nsAutoSyncManager::TimerCallback(nsITimer*, void*)	comm/mailnews/imap/src/nsAutoSyncManager.cpp:332 

- two are like bp-63496f75-75ce-46c1-acde-c0a8d0190104
 0 	xul.dll	nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*)	comm/mailnews/imap/src/nsImapProtocol.cpp:723
1 	xul.dll	nsImapProtocol::LoadImapUrl(nsIURI*, nsISupports*)	comm/mailnews/imap/src/nsImapProtocol.cpp:2151
2 	xul.dll	nsImapIncomingServer::GetImapConnectionAndLoadUrl(nsIImapUrl*, nsISupports*)	comm/mailnews/imap/src/nsImapIncomingServer.cpp:455
3 	xul.dll	nsImapService::GetImapConnectionAndLoadUrl(nsIImapUrl*, nsISupports*, nsIURI**)	comm/mailnews/imap/src/nsImapService.cpp:2227
4 	xul.dll	nsImapService::FolderCommand(nsIMsgFolder*, nsIUrlListener*, char const*, int, nsIMsgWindow*, nsIURI**)	comm/mailnews/imap/src/nsImapService.cpp:1511
5 	xul.dll	nsImapService::UpdateFolderStatus(nsIMsgFolder*, nsIUrlListener*, nsIURI**)	comm/mailnews/imap/src/nsImapService.cpp:1563
6 	xul.dll	nsImapMailFolder::UpdateStatus(nsIUrlListener*, nsIMsgWindow*)	comm/mailnews/imap/src/nsImapMailFolder.cpp:1420
7 	xul.dll	nsImapIncomingServer::OnStopRunningUrl(nsIURI*, nsresult)	comm/mailnews/imap/src/nsImapIncomingServer.cpp:2427
8 	xul.dll	nsMsgMailNewsUrl::SetUrlState(bool, nsresult)	comm/mailnews/base/util/nsMsgMailNewsUrl.cpp:164
9 	xul.dll	nsImapMailFolder::SetUrlState(nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, nsresult)	comm/mailnews/imap/src/nsImapMailFolder.cpp:6846
10 	xul.dll	`anonymous namespace'::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, enum nsresult>::Run	comm/mailnews/imap/src/nsSyncRunnableHelpers.cpp:251
Crash Signature: [@ nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*)] [@ nsImapProtocol::SetupWithUrl] → [@ nsImapProtocol::SetupWithUrl]
No longer depends on: 825513
Keywords: testcase-wanted
See Also: → 628646
Summary: crash [@ nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*)][@ nsImapProtocol::SetupWithUrl] with null m_runningUrl - aURL isn't nsIImapUrl → crash [@ nsImapProtocol::SetupWithUrl ][@ nsImapProtocol::SetupWithUrl] with null m_runningUrl - aURL isn't nsIImapUrl

Ben, Perhaps related to the rats nest you are investigating.

Whiteboard: [needs investigation of threading] → [patchlove][needs investigation of threading]

Some, but not all, are during autosync
bp-5b7f81a1-d2f2-43eb-8225-551230191117

0 xul.dll nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*) comm/mailnews/imap/src/nsImapProtocol.cpp:785
1 xul.dll nsImapProtocol::LoadImapUrl(nsIURI*, nsISupports*) comm/mailnews/imap/src/nsImapProtocol.cpp:2100
2 xul.dll nsImapIncomingServer::GetImapConnectionAndLoadUrl(nsIImapUrl*, nsISupports*) comm/mailnews/imap/src/nsImapIncomingServer.cpp:418
3 xul.dll nsImapService::GetImapConnectionAndLoadUrl(nsIImapUrl*, nsISupports*, nsIURI**) comm/mailnews/imap/src/nsImapService.cpp:2083
4 xul.dll nsImapService::FolderCommand(nsIMsgFolder*, nsIUrlListener*, char const*, int, nsIMsgWindow*, nsIURI**) comm/mailnews/imap/src/nsImapService.cpp:1428
5 xul.dll nsImapService::UpdateFolderStatus(nsIMsgFolder*, nsIUrlListener*, nsIURI**) comm/mailnews/imap/src/nsImapService.cpp:1474
6 xul.dll nsImapMailFolder::UpdateStatus(nsIUrlListener*, nsIMsgWindow*) comm/mailnews/imap/src/nsImapMailFolder.cpp:1301
7 xul.dll nsImapMailFolder::InitiateAutoSync(nsIUrlListener*) comm/mailnews/imap/src/nsImapMailFolder.cpp:8758

Phil, do you still crash ?

Flags: needinfo?(phil.pishioneri)
Flags: needinfo?(phil.pishioneri)

I have one user who reported crashing with Bullguard AV. I checked five recent crashes and none have Bullguard.

I checked about 8 crashes and actually most don't have AV so I don't think there is a strong AV correlation.
bp-85ecd0cd-ecfc-4caa-87e2-c04250210306 AVG
bp-852d2aa4-f01d-45bf-bdb3-20be40210305 Trend micro

Depends on: 1175168
Depends on: 1742991

Looks like version 91 crash rate was roughly the same as version 78 (in early November 2021 just before crash-stats stopped accepting TB crash reports)

Crash report: https://crash-stats.mozilla.org/report/index/561aa5bc-6376-48e7-a868-28c700211109 91.3.0

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll nsImapProtocol::SetupWithUrl comm/mailnews/imap/src/nsImapProtocol.cpp:793
1 xul.dll nsImapProtocol::LoadImapUrl comm/mailnews/imap/src/nsImapProtocol.cpp:2226
2 xul.dll nsImapIncomingServer::GetImapConnectionAndLoadUrl comm/mailnews/imap/src/nsImapIncomingServer.cpp:424
3 xul.dll nsImapService::GetImapConnectionAndLoadUrl comm/mailnews/imap/src/nsImapService.cpp:2087
4 xul.dll nsImapService::FolderCommand comm/mailnews/imap/src/nsImapService.cpp:1429
5 xul.dll nsImapService::UpdateFolderStatus comm/mailnews/imap/src/nsImapService.cpp:1475
6 xul.dll nsImapMailFolder::UpdateStatus comm/mailnews/imap/src/nsImapMailFolder.cpp:1311
7 xul.dll nsImapMailFolder::InitiateAutoSync comm/mailnews/imap/src/nsImapMailFolder.cpp:8697
8 xul.dll static nsAutoSyncManager::TimerCallback comm/mailnews/imap/src/nsAutoSyncManager.cpp:299
9 xul.dll nsTimerImpl::Fire xpcom/threads/nsTimerImpl.cpp:618
Severity: critical → S4
You need to log in before you can comment on or make changes to this bug.