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

VERIFIED FIXED in Firefox 24

Status

()

P1
blocker
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: scoobidiver, Assigned: khuey)

Tracking

({crash, regression})

24 Branch
mozilla24
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 unaffected, firefox24+ verified)

Details

(crash signature)

(Reporter)

Description

5 years ago
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

Updated

5 years ago
Priority: -- → P1
Assignee: nobody → khuey
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.
(Reporter)

Updated

5 years ago
tracking-firefox24: --- → ?
Keywords: topcrash
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).
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.
Summary: crash in nsContentUtils::DropJSObjects or mozilla::cyclecollector::RemoveJSHolder → shutdown crash in nsContentUtils::DropJSObjects or mozilla::cyclecollector::RemoveJSHolder
Yeah how come we run GC in opt builds but not CC?
Random code forces GCs in shutdown, nothing does so for the CC.  The one in UnloadModules appears to precede the existence of the CC.
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.
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.
(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.
(In reply to Andrew McCreight [:mccr8] from comment #9)
> https://tbpl.mozilla.org/?tree=Try&rev=efc0024e21a3

Or you can try it! ;-)
It looks like even JS_DestroyContextNoGC will GC if it's destroying the last JSContext ...
JS_DestroyContextReallyNoGC? ;)

Updated

5 years ago
tracking-firefox24: ? → +
Blocks: 881364

Updated

5 years ago
Duplicate of this bug: 881364
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.
Well, this is a top crash, so you should probably just back it out.  Though this _is_ speeding up shutdown..
Blocks: 881560
(Reporter)

Comment 16

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Severity: critical → blocker
I've backed out bug 877584 at khuey's request.
Nightlies have been respun and will be available in a few hours.
(Reporter)

Comment 18

5 years ago
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.
Status: NEW → RESOLVED
Crash Signature: [@ mozilla::cyclecollector::RemoveJSHolder(void*) ] [@ nsContentUtils::DropJSObjects(void*)] → [@ mozilla::cyclecollector::RemoveJSHolder(void*) ] [@ nsContentUtils::DropJSObjects(void*)] [@ nsDocument::~nsDocument() ]
Last Resolved: 5 years ago
status-firefox24: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
I filed bug 883657 for avoiding this deep shutdown GC.

Comment 20

5 years ago
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.
Status: RESOLVED → VERIFIED
status-firefox24: fixed → verified
Keywords: topcrash
You need to log in before you can comment on or make changes to this bug.