1044443 - release off main thread crash in nsXPCWrappedJS::Release() via nsUpdateProcessor::~nsUpdateProcessor()

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Alice0775 White]

This bug was filed from the Socorro interface and is 
report bp-dba7aca3-756a-4b78-92be-25d062140726.
=============================================================

Crash after daily update from Help > About Nightly

Comment 1

[Josh Matthews [:jdm]]

The nsCOMPtr<nsIUpdate> in nsUpdateProcessor is the culprit here.

Comment 2

[Alice0775 White]

Aurora33.0a2 also crashed
bp-67a29718-895a-45a4-bd81-daf4a2140805

Comment 3

[baffclan]

Reproduce on:
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0

bp-e90a1577-4586-4723-b667-9dd062141014
bp-6358fbd4-f1d3-414b-b7a1-49b592141013

Comment 4

[Wayne Mery (:wsmwk)]

Thunderbird dev builds
bp-cc16ba46-4a7d-40d7-9284-77fc22141006
bp-a943a723-8a97-4385-9ab1-8288f2141020
bp-6a838c1d-3bcc-4936-ba84-6f8832141018
 0 	xul.dll	nsXPCWrappedJS::Release()	js/xpconnect/src/XPCWrappedJS.cpp
1 	xul.dll	nsUpdateProcessor::~nsUpdateProcessor()	toolkit/xre/nsUpdateDriver.cpp
2 	xul.dll	nsUpdateProcessor::Release()	toolkit/xre/nsUpdateDriver.cpp
3 	xul.dll	nsRunnableMethodImpl<tag_nsresult ( mozilla::net::BackgroundFileSaverStreamListener::*)(void), void, 1>::`scalar deleting destructor'(unsigned int)	
4 	xul.dll	AsyncLatencyLogger::Release()	netwerk/base/src/Dashboard.cpp
5 	xul.dll	nsThread::ProcessNextEvent(bool, bool*)	xpcom/threads/nsThread.cpp
6 	xul.dll	NS_ProcessNextEvent(nsIThread*, bool)	xpcom/glue/nsThreadUtils.cpp
7 	xul.dll	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)	ipc/glue/MessagePump.cpp

Comment 5

[Wayne Mery (:wsmwk)]

I was testing TB34beta update to TB36beta Help > About > Check for updates

me during startup bp-68c79503-d30b-4c7c-aadf-641032150216
0 	xul.dll	nsXPCWrappedJS::Release()	js/xpconnect/src/XPCWrappedJS.cpp
1 	xul.dll	nsXPTCStubBase::Release()	xpcom/reflect/xptcall/xptcall.cpp
2 	xul.dll	nsUpdateProcessor::~nsUpdateProcessor()	toolkit/xre/nsUpdateDriver.cpp
3 	xul.dll	nsUpdateProcessor::Release()	toolkit/xre/nsUpdateDriver.cpp
4 	xul.dll	nsRunnableMethodImpl<tag_nsresult ( mozilla::widget::AudioSession::*)(void), void, 1>::~nsRunnableMethodImpl<tag_nsresult ( mozilla::widget::AudioSession::*)(void), void, 1>()	
5 	xul.dll	nsRunnableMethodImpl<void ( CameraPermissionRequest::*)(void), void, 1>::`vector deleting destructor'(unsigned int)	
6 	xul.dll	nsTokenEventRunnable::Release()	netwerk/base/src/nsUDPSocket.cpp 

For nightly builds, approx 50% with this signature have this stack, which ranks it in the top 10.

Comment 6

[Robert Strong (they/them - no direct email)]

How were you able to get this to crash on startup?

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: patch that can be uplifted

Brian, I haven't been able to reproduce but it is obvious that the reference to the nsIUpdate doesn't need to be kept. This patch could be uplifted to branches if you think it will help and if you do I can put together a patch to clean it up further for nightly and aurora but that will require the uuid for nsIUpdateManager.

Comment 8

[Robert Strong (they/them - no direct email)]

Try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e4dac953e39

Comment 9

[Wayne Mery (:wsmwk)]

1. installed 34.0beta
2. started up to test profile (unknown which version of Thunderbird I last ran in it)
3. help | about | check for updates
4. pick update to 36.0 beta
5. version 36 installs and starts
6. bp-68c79503-d30b-4c7c-aadf-641032150216 

NOTE: the report cites version 34 but should it not be version 36?
Right after that I made a second attempt with the same steps, was unable to reproduce. 

Firefox crashes with this stack (not all startup)
bp-e90a1577-4586-4723-b667-9dd062141014
bp-6358fbd4-f1d3-414b-b7a1-49b592141013
bp-67a29718-895a-45a4-bd81-daf4a2140805


Not to confuse, but there are other crashes in this league, but with different immediate cuase
this is a crash of mine on Feb 8, different stack. 
At the time I was not at the computer.
bp-a3da1435-0f95-46de-862c-5dcd62150208
0	xul.dll	nsXPCWrappedJS::Release()	js/xpconnect/src/XPCWrappedJS.cpp
1	xul.dll	nsXPTCStubBase::Release()	xpcom/reflect/xptcall/xptcall.cpp
2	xul.dll	ReleaseObjects	xpcom/glue/nsCOMArray.cpp
3	xul.dll	nsCOMArray_base::Clear()	xpcom/glue/nsCOMArray.cpp
4	xul.dll	nsMsgProgress::~nsMsgProgress()	c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mailnews/base/src/nsMsgProgress.cpp:34
5	xul.dll	nsMsgProgress::`scalar deleting destructor'(unsigned int)	
6	xul.dll	nsMsgProgress::Release()	c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mailnews/base/src/nsMsgProgress.cpp:21
7	xul.dll	nsMsgComposeAndSend::~nsMsgComposeAndSend()	c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mailnews/compose/src/nsMsgSend.cpp:323
8	xul.dll	nsMsgComposeAndSend::`scalar deleting destructor'(unsigned int)	
9	xul.dll	nsMsgComposeAndSend::Release()	c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mailnews/compose/src/nsMsgSend.cpp:255
10	xul.dll	CopyListener::~CopyListener()	c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mailnews/compose/src/nsMsgCopy.cpp:49
11	xul.dll	CopyListener::`scalar deleting destructor'(unsigned int)	
12	xul.dll	mozilla::net::FileOpenHelper::Release()	netwerk/cache2/CacheFile.cpp
13	xul.dll	nsImapMailCopyState::~nsImapMailCopyState()	c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mailnews/imap/src/nsImapMailFolder.cpp:8215
14	xul.dll	nsImapMailCopyState::`scalar deleting destructor'(unsigned int)	
15	xul.dll	mozilla::net::WebSocketRequest::Release()	netwerk/base/Dashboard.cpp
16	xul.dll	nsImapUrl::~nsImapUrl()	c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mailnews/imap/src/nsImapUrl.cpp:79
17	xul.dll	nsImapUrl::`scalar deleting destructor'(unsigned int)	
18	xul.dll	nsMsgMailNewsUrl::Release()	c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mailnews/base/util/nsMsgMailNewsUrl.cpp:57
19	xul.dll	nsSmtpUrl::Release()	c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mailnews/compose/src/nsSmtpUrl.cpp:599
20	xul.dll	nsMsgProtocol::~nsMsgProtocol()	c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mailnews/base/util/nsMsgProtocol.cpp:83
21	xul.dll	nsImapProtocol::~nsImapProtocol()	c:/builds/moz2_slave/tb-c-cen-w32-ntly-000000000000/build/mailnews/imap/src/nsImapProtocol.cpp:581
22	xul.dll	nsImapProtocol::`scalar deleting destructor'(unsigned int)

Comment 10

[Robert Strong (they/them - no direct email)]

For my own clarity, does this crash happen after clicking the button in the about window?

Comment 11

[Robert Strong (they/them - no direct email)]

Also, could you provide the text of the button in the about window or a screenshot?

Comment 12

[Wayne Mery (:wsmwk)]

Attachment: TB34beta-check.png

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #10)
> For my own clarity, does this crash happen after clicking the button in the
> about window?

crash was after clicking *restart*

Comment 13

[Robert Strong (they/them - no direct email)]

I'm quite certain it isn't during startup which is supported by the report being for 34 instead of 36.

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: patch that can be uplifted

Comment 15

[Brian R. Bondy [:bbondy]]

Comment on attachment 8566094 [details] [diff] [review]
patch that can be uplifted

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

Worth a try I think.

Comment 16

[Robert Strong (they/them - no direct email)]

Comment on attachment 8566094 [details] [diff] [review]
patch that can be uplifted

Brian, sorry for the additional request.... spohl isn't around

Comment 17

[Robert Strong (they/them - no direct email)]

Try push with the updated patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2bc72413560

Comment 18

[Robert Strong (they/them - no direct email)]

pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/b596d5ffe50f

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b596d5ffe50f

Comment 20

[Robert Strong (they/them - no direct email)]

Comment on attachment 8566094 [details] [diff] [review]
patch that can be uplifted

Crash reports for this signature have not been present on Nightly since this landed.

Approval Request Comment
[Feature/regressing bug #]: Bug 307181 and exposed by shutdownhang crashes
[User impact if declined]: Continued to crash during shutdown when staging an update
[Describe test coverage new/current, TreeHerder]: the code is exercised and tested by both xpcshell and mochitest-chrome tests.
[Risks and why]: Minimal due to the patch being a simple change of not keeping a reference to the object. Also, I have found no indication that the crash has moved to a different signature.
[String/UUID change made/needed]: None

Comment 21

[Robert Strong (they/them - no direct email)]

Comment on attachment 8566094 [details] [diff] [review]
patch that can be uplifted

This will make it into aurora with the merge so removing that request.

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8566094 [details] [diff] [review]
patch that can be uplifted

Taking this crash fix related to updates in 37. Beta+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8ac02f8d22e5

Comment 24

[Robert Strong (they/them - no direct email)]

Comment on attachment 8566094 [details] [diff] [review]
patch that can be uplifted

Approval Request Comment
[Feature/regressing bug #]: Bug 307181 and exposed by shutdownhang crashes
[User impact if declined]: Continued to crash during shutdown when staging an update
[Describe test coverage new/current, TreeHerder]: the code is exercised and tested by both xpcshell and mochitest-chrome tests.
[Risks and why]: Minimal due to the patch being a simple change of not keeping a reference to the object. Also, I have found no indication that the crash has moved to a different signature.
[String/UUID change made/needed]: None

Comment 25

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-release
https://hg.mozilla.org/releases/mozilla-release/rev/3cd0f44138da

Comment 26

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Verified in Socorro for the past 4 weeks, and this looks good so far:
39.0a1 - 1 crash for TB (not via nsUpdateProcessor)
38.0a2 - 0 crashes
38.0a1 (builds after Feb 19) - 0 crashes
37.0b2 (and older) - 2 crashes (not via nsUpdateProcessor)
36.0.1 - need more time to gather data

[1] - https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=nsXPCWrappedJS%3A%3ARelease%28%29#tab-sigsummary