810618 - crash in mozilla::dom::EventHandlerNonNull::`.* deleting destructor'

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Scoobidiver (away)]

It first showed up in 19.0a1/20121110. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=90cea19e27e2&tochange=a47525b93528
It's likely a regression from bug 779048.

Signature 	mozilla::dom::EventHandlerNonNull::`scalar deleting destructor''(unsigned int) More Reports Search
UUID	af39b94c-92e8-4616-8273-2cab52121110
Date Processed	2012-11-10 19:24:25
Uptime	866
Last Crash	1.2 hours before submission
Install Age	1.4 hours since version was first installed.
Install Time	2012-11-10 17:58:25
Product	Firefox
Version	19.0a1
Build ID	20121110030714
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 15 stepping 11
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x10de, AdapterDeviceID: 0x05e2, AdapterSubsysID: 00000000, AdapterDriverVersion: 9.18.13.697
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
EMCheckCompatibility	False
Adapter Vendor ID	0x10de
Adapter Device ID	0x05e2
Total Virtual Memory	4294836224
Available Virtual Memory	3762077696
System Memory Use Percentage	30
Available Page File	10320437248
Available Physical Memory	4498960384

Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::dom::EventHandlerNonNull::`scalar deleting destructor' 	
1 	xul.dll 	mozilla::dom::CallbackFunction::Release 	dom/bindings/CallbackFunction.cpp:24
2 	xul.dll 	nsJSEventListener::`vector deleting destructor' 	
3 	xul.dll 	nsJSEventListener::Release 	dom/src/events/nsJSEventListener.cpp:142
4 	xul.dll 	nsTArray<nsListenerStruct,nsTArrayDefaultAllocator>::DestructRange 	obj-firefox/dist/include/nsTArray.h:1223
5 	xul.dll 	nsTArray<nsListenerStruct,nsTArrayDefaultAllocator>::RemoveElementsAt 	obj-firefox/dist/include/nsTArray.h:943
6 	xul.dll 	nsAutoTObserverArray<nsListenerStruct,2>::Clear 	obj-firefox/dist/include/nsTObserverArray.h:244
7 	xul.dll 	nsDOMEventTargetHelper::~nsDOMEventTargetHelper 	content/events/src/nsDOMEventTargetHelper.cpp:85
8 	xul.dll 	nsXMLHttpRequest::`vector deleting destructor' 	
9 	xul.dll 	nsXMLHttpRequest::Release 	content/base/src/nsXMLHttpRequest.cpp:656
10 	xul.dll 	DoDeferredRelease<nsISupports*> 	js/xpconnect/src/XPCJSRuntime.cpp:507
11 	xul.dll 	XPCJSRuntime::GCCallback 	js/xpconnect/src/XPCJSRuntime.cpp:713
12 	mozjs.dll 	Collect 	js/src/jsgc.cpp:4610
13 	mozjs.dll 	js::GC 	js/src/jsgc.cpp:4630
14 	mozjs.dll 	js::DestroyContext 	js/src/jscntxt.cpp:539
15 	mozjs.dll 	JS_DestroyContext 	js/src/jsapi.cpp:1237
16 	xul.dll 	mozJSComponentLoader::UnloadModules 	js/xpconnect/loader/mozJSComponentLoader.cpp:1047
17 	xul.dll 	mozJSComponentLoader::Observe 	js/xpconnect/loader/mozJSComponentLoader.cpp:1380
18 	xul.dll 	mozilla::ShutdownXPCOM 	xpcom/build/nsXPComInit.cpp:638
19 	xul.dll 	ScopedXPCOMStartup::~ScopedXPCOMStartup 	toolkit/xre/nsAppRunner.cpp:1123
20 	xul.dll 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3914
21 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:4083

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Adom%3A%3AEventHandlerNonNull%3A%3A%60scalar+deleting+destructor%27%27%28unsigned+int%29

Comment 1

[Boris Zbarsky [:bzbarsky]]

Definitely a regression from bug 779048, yes.

What I don't understand is why it's failing.  It's clearly a null deref, but of what?

There is no destructor declared in EventHandlerNonNull; there's just a virtual destructor in the superclass...

I wonder whether the scalar vs vector thing matters here somehow.  I would have expected these to all be scalar deleting destructors, so the fact that they're not is a bit surprising to me.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The one weird part I see is that we're doing NS_DROP_JS_OBJECTS in the CallbackFunction destructor, which does NS_CYCLE_COLLECTION_UPCAST() on the object, which is in the middle of being destroyed.  But it doesn't seem like that should matter... In particular, the subclass has no virtual methods that could get called accidentally here.

Really wish I had a way to reproduce.

Comment 3

[Scoobidiver (away)]

It's #3 top crasher in today's build.

Comment 4

[Scoobidiver (away)]

There's one comment from a user with many extensions:
"FF nigtly has been filing bug reports for the past 48 hours when I am actually closing it normally. It isn't so much a crash as a problem with the program not closing down properly after instructing it to do so."

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, so crash during shutdown.  That's what the stack in comment 0 shows too.

I wonder whether sXPConnect might be null in nsContentUtils::DropJSObjects or something.

Comment 6

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

(In reply to Boris Zbarsky (:bz) from comment #5)
> OK, so crash during shutdown.  That's what the stack in comment 0 shows too.
> 
> I wonder whether sXPConnect might be null in nsContentUtils::DropJSObjects
> or something.

Yes, that's exactly what is happening.

Comment 7

[Boris Zbarsky [:bzbarsky]]

And in particular, nsXMLHttpRequest dtor released layout statics, which was the last release, but its superclass is what drops the callbacks.  Going to make callbacks refcount layout statics.

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://crash-stats.mozilla.com/report/list?product=Firefox&platform=windows&platform=mac&platform=linux&query_search=signature&query_type=contains&query=DropJSObjects&reason_type=contains&date=11%2F12%2F2012%2022%3A19%3A08&range_value=1&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=nsContentUtils%3A%3ADropJSObjects%28void*%29 shows that there are non-Windows crashes that just don't have DropJSObjects inlined into the destructor, confirming that hypothesis.

Comment 9

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

bz is going to patch this.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Andrew, could this be because of the unbalanced hold/drop js calls? Didn't you file a bug about that.

Comment 11

[Andrew McCreight [:mccr8]]

From IRC chat, my understanding of this is that it isn't a problem in DropJSObjects, the problem is that the callback stuff bz added doesn't properly do LayoutStatics::AddRef, so sXPConnect goes away before it should. Or something like that.

Comment 12

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

The problem is that the last nsLayoutStatics::Release call causes (among other things) nsContentUtils::sXPConnect to be nulled out.  ~nsXMLHttpRequest calls nsLayoutStatics, but ~nsDOMEventTargetHelper (and thus clearing of the callbacks) hasn't run yet, so when we do get to clearing the callbacks we've lost.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure we don't allow layout statics to go away while we're still trying to use them.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 680841 [details] [diff] [review]
Make sure we don't allow layout statics to go away while we're still trying to use them.

So why isn't this about the hold/drop problem.
nsContentUtils::HoldJSObjects should keep 
nsLayoutStatics alive unless we call nsContentUtils::DropJSObjects
too often, right?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  You're right, it seems like it should.  If sJSGCThingRootCount is correct.  Which it clearly isn't....

Comment 16

[Andrew McCreight [:mccr8]]

The gist of it is that some classes are sloppy about HOLDing or DROPing objects multiple times, but the current code for the root count assumes it is right, so it can fall out of balance, which potentially causes the release to happen too early.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Can we somehow assert at runtime (fatally!) if there is an unbalanced DROP?

Comment 18

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

Well we can't tell until it's too late, no?

Comment 19

[Boris Zbarsky [:bzbarsky]]

We sort of can.  We can tell if someone DROPs twice in a row on the same object.  Even if they did two HOLD calls, I would argue it's a bug and they should just make sure they DROP only once (and HOLD only once).

Comment 20

[Andrew McCreight [:mccr8]]

Yes, what we would do is return an error if we HOLD an object already in the table, or DROP an object not in the table, and not adjust the count if there's an error. I'll put something together in bug 792861.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Landed http://hg.mozilla.org/integration/mozilla-inbound/rev/92cb4a2c1ed4 for now, while we sort out the hold/drop stuff.  Kyle, thank you for your help debugging this and for the fast review!

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/92cb4a2c1ed4

Comment 23

[Andrew McCreight [:mccr8]]

I don't see any crashes on the 11/13 build, so I guess this actually fixed the crash?

Comment 24

[Boris Zbarsky [:bzbarsky]]

That would be the thinking, yes.... ;)

Comment 25

[Ada [:adalucinet]]

A crash with mozilla::dom::EventHandlerNonNull::`vector deleting destructor''(unsigned int) signature has occurred in Firefox 19 beta 3.

https://crash-stats.mozilla.com/report/list?signature=mozilla::dom::EventHandlerNonNull::`vector%20deleting%20destructor''(unsigned%20int)

Can someone please look at the crash and verify if it's the same bug?

Comment 26

[Boris Zbarsky [:bzbarsky]]

The link in comment 25 shows no crashes for me....

Comment 27

[Ada [:adalucinet]]

Sorry, here is the correct link:

https://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A19.0b3&query_search=signature&query_type=contains&query=mozilla%3A%3Adom%3A%3AEventHandlerNonNull%3A%3A%60vector%20deleting%20destructor%27%27%28unsigned%20int%29&reason_type=contains&date=01%2F29%2F2013%2014%3A22%3A02&range_value=1&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozilla%3A%3Adom%3A%3AEventHandlerNonNull%3A%3A%60vector%20deleting%20destructor%27%27%28unsigned%20int%29#reports

Comment 28

[Boris Zbarsky [:bzbarsky]]

Thanks.

That looks like someone jumped off into lala-land during createInstance....  not related to this bug, offhand.

Comment 29

[Ada [:adalucinet]]

Marking as verified in Firefox 19 based on comment 28.

Comment 30

[pal-moz]

again ?

(2013061003 NIGHTLY)
Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20130610 Firefox/24.0 ID:20130610031147 CSet: 252b1ac4d537

Comment 31

[Scoobidiver (away)]

(In reply to pal-moz from comment #30)
> again ?
You hit bug 881266, not this bug.