881266 - shutdown crash in nsContentUtils::DropJSObjects or mozilla::cyclecollector::RemoveJSHolder

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P1)

Description

[Scoobidiver (away)]

It's currently #2 top crasher in today's build with about 45 crashes an hour. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=efbe547a7972&tochange=252b1ac4d537
It's likely a regression from bug 877584.

Stack traces are various:
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsContentUtils::DropJSObjects 	content/base/src/nsContentUtils.cpp:4336
1 	xul.dll 	nsDocument::~nsDocument 	content/base/src/nsDocument.cpp:1430
2 	xul.dll 	mozilla::dom::XMLDocument::`scalar deleting destructor' 	
3 	xul.dll 	nsNodeUtils::LastRelease 	content/base/src/nsNodeUtils.cpp:259
4 	xul.dll 	nsDocument::Release 	content/base/src/nsDocument.cpp:1583
5 	xul.dll 	DoDeferredRelease<nsISupports*> 	js/xpconnect/src/XPCJSRuntime.cpp:628
6 	xul.dll 	XPCJSRuntime::GCCallback 	js/xpconnect/src/XPCJSRuntime.cpp:828
7 	mozjs.dll 	Collect 	js/src/jsgc.cpp:4586
8 	mozjs.dll 	js::DestroyContext 	js/src/jscntxt.cpp:388
9 	xul.dll 	mozJSComponentLoader::UnloadModules 	js/xpconnect/loader/mozJSComponentLoader.cpp:1146
10 	xul.dll 	mozJSComponentLoader::Observe 	js/xpconnect/loader/mozJSComponentLoader.cpp:1460
11 	xul.dll 	mozilla::ShutdownXPCOM 	xpcom/build/nsXPComInit.cpp:672
...

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsContentUtils::DropJSObjects 	content/base/src/nsContentUtils.cpp:4336
1 	xul.dll 	nsXMLHttpRequest::~nsXMLHttpRequest 	content/base/src/nsXMLHttpRequest.cpp:333
2 	xul.dll 	nsXMLHttpRequest::`scalar deleting destructor' 	
3 	xul.dll 	nsCSSKeyframeStyleDeclaration::Release 	layout/style/nsCSSRules.cpp:2669
4 	xul.dll 	DoDeferredRelease<nsISupports*> 	js/xpconnect/src/XPCJSRuntime.cpp:628
5 	xul.dll 	XPCJSRuntime::GCCallback 	js/xpconnect/src/XPCJSRuntime.cpp:828
6 	mozjs.dll 	Collect 	js/src/jsgc.cpp:4586
7 	mozjs.dll 	js::DestroyContext 	js/src/jscntxt.cpp:388
8 	xul.dll 	mozJSComponentLoader::UnloadModules 	js/xpconnect/loader/mozJSComponentLoader.cpp:1146
9 	xul.dll 	mozJSComponentLoader::Observe 	js/xpconnect/loader/mozJSComponentLoader.cpp:1460
10 	xul.dll 	mozilla::ShutdownXPCOM 	xpcom/build/nsXPComInit.cpp:672
...

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsContentUtils::DropJSObjects 	content/base/src/nsContentUtils.cpp:4336
1 	xul.dll 	mozilla::dom::CallbackObject::~CallbackObject 	obj-firefox/dist/include/mozilla/dom/CallbackObject.h:58
2 	xul.dll 	mozilla::dom::RTCSessionDescriptionCallback::`scalar deleting destructor' 	
3 	xul.dll 	DialogValueHolder::Release 	dom/base/nsGlobalWindow.cpp:547
4 	xul.dll 	nsIJSEventListener::~nsIJSEventListener 	obj-firefox/dist/include/nsIJSEventListener.h:264
5 	xul.dll 	nsJSEventListener::`vector deleting destructor' 	
6 	xul.dll 	nsJSEventListener::Release 	dom/src/events/nsJSEventListener.cpp:141
7 	xul.dll 	nsTArray_Impl<nsListenerStruct,nsTArrayInfallibleAllocator>::RemoveElementsAt 	obj-firefox/dist/include/nsTArray.h:1110
8 	xul.dll 	nsEventListenerManager::RemoveAllListeners 	content/events/src/nsEventListenerManager.cpp:144
9 	xul.dll 	nsContentUtils::RemoveListenerManager 	content/base/src/nsContentUtils.cpp:3807
10 	xul.dll 	nsNodeUtils::LastRelease 	content/base/src/nsNodeUtils.cpp:235
11 	xul.dll 	mozilla::dom::FragmentOrElement::Release 	content/base/src/FragmentOrElement.cpp:1710
12 	xul.dll 	DoDeferredRelease<nsISupports*> 	js/xpconnect/src/XPCJSRuntime.cpp:628
13 	xul.dll 	XPCJSRuntime::GCCallback 	js/xpconnect/src/XPCJSRuntime.cpp:828
14 	mozjs.dll 	Collect 	js/src/jsgc.cpp:4586
15 	mozjs.dll 	js::DestroyContext 	js/src/jscntxt.cpp:388
16 	xul.dll 	mozJSComponentLoader::UnloadModules 	js/xpconnect/loader/mozJSComponentLoader.cpp:1146
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Acyclecollector%3A%3ARemoveJSHolder%28void*%29
https://crash-stats.mozilla.com/report/list?signature=nsContentUtils%3A%3ADropJSObjects%28void*%29

Comment 1

[Andrew McCreight [:mccr8]]

Hmm.  I guess we need to null-check those functions.  Maybe when we're shutting down in opt builds we don't do shutdown CCs, so we end up with things being alive after CC shutdown.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well the problem with just null-checking is that if we don't remove dead things from the XPConnect hashtable if we trace XPConnect roots again we're going to crash (and that's going to be a security bug).

Comment 3

[Andrew McCreight [:mccr8]]

Good point.

Also, forcing a GC in mozJSComponentLoader::UnloadModules() seems goofy, given that this is really late in shutdown.  I'll file a bug on that, maybe we can change that.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah how come we run GC in opt builds but not CC?

Comment 5

[Andrew McCreight [:mccr8]]

Random code forces GCs in shutdown, nothing does so for the CC.  The one in UnloadModules appears to precede the existence of the CC.

Comment 6

[Andrew McCreight [:mccr8]]

In fact, assuming that
a) these crashes are mostly in mozJSComponentLoader::UnloadModules()
b) mozJSComponentLoader::UnloadModules() is being called deep in shutdown after the CC is shutdown
...I would argue that the right fix is to change JS_DestroyContext() to JS_DestroyContextNoGC() in mozJSComponentLoader::UnloadModules().  If UnloadModules() really needs the GC for whatever reason, it should be run before the CC shuts down.

Comment 7

[Andrew McCreight [:mccr8]]

This observer call actually happens immediately after the CC shuts down:
  http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPComInit.cpp#546
Anyways, maybe it should be moved before the CC shuts down, maybe not, but I think it really shouldn't force a GC itself.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> ...I would argue that the right fix is to change JS_DestroyContext() to
> JS_DestroyContextNoGC() in mozJSComponentLoader::UnloadModules().  If
> UnloadModules() really needs the GC for whatever reason, it should be run
> before the CC shuts down.

I'll try it but I expect it will leak.

Comment 9

[Andrew McCreight [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=efc0024e21a3

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> https://tbpl.mozilla.org/?tree=Try&rev=efc0024e21a3

Or you can try it! ;-)

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It looks like even JS_DestroyContextNoGC will GC if it's destroying the last JSContext ...

Comment 12

[Andrew McCreight [:mccr8]]

JS_DestroyContextReallyNoGC? ;)

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So, if we're willing to live with this for a couple days I think we can add the null check without the problem from comment 2 after some more of my patches land.

Alternatively we can back bug 877584 out and reland it later once those are done.

Comment 15

[Andrew McCreight [:mccr8]]

Well, this is a top crash, so you should probably just back it out.  Though this _is_ speeding up shutdown..

Comment 16

[Scoobidiver (away)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #14) 
> Alternatively we can back bug 877584 out and reland it later once those are
> done.
Yes because there are currently 2100 crashes.

Comment 17

[Ed Morley [:emorley]]

I've backed out bug 877584 at khuey's request.
Nightlies have been respun and will be available in a few hours.

Comment 18

[Scoobidiver (away)]

More reports also at (signature morphing):
https://crash-stats.mozilla.com/report/list?signature=nsDocument%3A%3A~nsDocument%28%29

Closing as fixed per comment 17.

Comment 19

[Andrew McCreight [:mccr8]]

I filed bug 883657 for avoiding this deep shutdown GC.

Comment 20

[Ioana (away)]

According to Socorro, this is definitely no longer a top crash.
[@ mozilla::cyclecollector::RemoveJSHolder(void*) ] - 3 crashes in Firefox 24 beta, no Firefox 25 and 26 crashes
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Acyclecollector%3A%3ARemoveJSHolder%28void%2A%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&date=2013-08-27+07%3A00%3A00&range_value=4

[@ nsContentUtils::DropJSObjects(void*)] - only Firefox 23 crashes
https://crash-stats.mozilla.com/report/list?signature=nsContentUtils%3A%3ADropJSObjects%28void%2A%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&date=2013-08-27+07%3A00%3A00&range_value=4

[@ nsDocument::~nsDocument() ] - 6 Firefox 24 crashes, 1 Firefox 26 crash
https://crash-stats.mozilla.com/report/list?signature=nsDocument%3A%3A~nsDocument%28%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&date=2013-08-27+07%3A00%3A00&range_value=4

Please let me know if you need me to file a new bug for the remaining crashes.