640473 - crash [@ nsDOMClassInfo::WrapNative] [@ WrapNative] after XPCOM shutdown

Status: RESOLVED WONTFIX

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

Description

[Josh Matthews [:jdm]]

This bug was filed from the Socorro interface and is 
report bp-a3caf345-07bf-4d6d-9fe8-759fd2110309 .
============================================================= 

XPCOM is in the midst of shutting down and ends up running an event that triggers some javascript and boom, we end up running

>4273 return sXPConnect->WrapNativeToJSVal(cx, scope, native, cache, aIID,
>4274                                      aAllowWrapping, vp, aHolder); 

where sXPConnect was released in nsDOMClassInfo::Shutdown.

0 	libxul.so 	nsDOMClassInfo::WrapNative 	dom/base/nsDOMClassInfo.cpp:4274
1 	libxul.so 	nsArraySH::GetProperty 	dom/base/nsDOMClassInfo.cpp:8020
2 	libxul.so 	nsNamedArraySH::GetProperty 	dom/base/nsDOMClassInfo.cpp:8206
3 	libxul.so 	XPC_WN_Helper_GetProperty 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1000
4 	libxul.so 	js_GetProperty 	js/src/jscntxtinlines.h:753
5 	libxul.so 	JSObject::getProperty 	js/src/jsobj.h:1228
6 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:4565
7 	libxul.so 	js::Invoke 	js/src/jsinterp.cpp:653
8 	libxul.so 	js_fun_apply 	js/src/jsfun.cpp:2206
9 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:4801
10 	libxul.so 	js::Invoke 	js/src/jsinterp.cpp:653
11 	libxul.so 	js_fun_apply 	js/src/jsfun.cpp:2206
12 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:4801
13 	libxul.so 	js::Invoke 	js/src/jsinterp.cpp:653
14 	libxul.so 	js::ExternalInvoke 	js/src/jsinterp.cpp:863
15 	libxul.so 	JS_CallFunctionValue 	js/src/jsapi.cpp:5173
16 	libxul.so 	nsXPCWrappedJSClass::CallMethod 	js/src/xpconnect/src/xpcwrappedjsclass.cpp:1674
17 	libxul.so 	nsXPCWrappedJS::CallMethod 	js/src/xpconnect/src/xpcwrappedjs.cpp:589
18 	libxul.so 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:134
19 	libxul.so 	libxul.so@0x93a8c4 	
20 	libxul.so 	mozilla::storage::::CompletionNotifier::Run 	storage/src/mozStorageAsyncStatementExecution.cpp:180
21 		@0x44bb155b 	
22 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:633
23 	libxul.so 	NS_ProcessPendingEvents_P 	nsThreadUtils.cpp:201
24 	libxul.so 	mozilla::ShutdownXPCOM 	xpcom/build/nsXPComInit.cpp:637
25 	libxul.so 	NS_ShutdownXPCOM_P 	xpcom/build/nsXPComInit.cpp:595
26 	libxul.so 	ScopedXPCOMStartup::~ScopedXPCOMStartup 	toolkit/xre/nsAppRunner.cpp:1116
27 	libxul.so 	XRE_main 	nsCOMPtr.h:800

Comment 1

[Josh Matthews [:jdm]]

Since this is storage-related, I'm bringing in sdwilsh for his thoughts.  We could either fix this by null-checking every use of sXPConnect, or we could see if there's some way to run the problematic CompletionNotifier in such a way that it do xpconnect stuff after we've shut down.

Comment 2

[Shawn Wilsher :sdwilsh]

I can't see us doing the latter approach, so I think the former approach is the only way to go.

Comment 3

[Josh Matthews [:jdm]]

Just to clarify, in comment 1 I meant to say "or we could see if there's some way to run the problematic CompletionNotifier in such a way that it _doesn't_ do xpconnect stuff after we've shut down."

Shawn, unless that changes your feelings, I'll see if I can whip up a null-check patch.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Avoid trying to use xpconnect after it doesn't exist.

Pushed to try: http://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]&rev=a885064f2e1f

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 536662 [details] [diff] [review]
Avoid trying to use xpconnect after it doesn't exist.

Try was all green.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 536662 [details] [diff] [review]
Avoid trying to use xpconnect after it doesn't exist.

Hmm, Peter, what do you think about this?

Comment 7

[Peter Van der Beken [:peterv]]

I think this is the wrong thing to do, apart form XPConnect a ton of other cached services have been released. Why do we run code that touches the DOM so late in shutdown? Even if we fix the crash this will leak all over the place since the CC has been shut down.
Could this run from "xpcom-will-shutdown" or be canceled from "xpcom-shutdown"?

Comment 8

[Peter Van der Beken [:peterv]]

Also, what kind of array is that scriptable helper for? We could keep nsLayoutStatics as long as the array is alive, that's what we do for other DOM objects.

Comment 9

[Josh Matthews [:jdm]]

https://crash-stats.mozilla.com/report/index/1c09504a-4585-449d-a681-6760b2110604 seems to be a pretty representative sample of the crashes.

As far as I can tell, we're running storage::CompletionNotifier runnables that have been queued up, and then they go and stick their grubby hands in the DOM. I suppose it would be conceivable to make each runnable observe xpcom-shutdown and avoid triggering the callback. sdwilsh, does this sound like a reasonable solution?

Comment 10

[Josh Matthews [:jdm]]

Attachment: Avoid running storage callbacks when xpcom is shutting down.

This patch delegates the responsibility of not running to the storage code. Is this a preferable solution, Peter?

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> As far as I can tell, we're running storage::CompletionNotifier runnables that
> have been queued up, and then they go and stick their grubby hands in the DOM.
> I suppose it would be conceivable to make each runnable observe xpcom-shutdown
> and avoid triggering the callback. sdwilsh, does this sound like a reasonable
> solution?
There can be a lot of those, and I really don't like that approach.

Comment 12

[Josh Matthews [:jdm]]

Please note that the patch does not actually implement that idea. Instead there's a global flag+observer combo that all completion runnables check before attempting to run their callback.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> Please note that the patch does not actually implement that idea. Instead
> there's a global flag+observer combo that all completion runnables check before
> attempting to run their callback.
I still don't like this because it makes all completion events not notify.  Not all completion events are touching the DOM, but they all pay the price and don't run because some do?

Comment 14

[Josh Matthews [:jdm]]

I simply assumed that there was a potential for any completion event callback to touch the DOM. What subset should include the check?

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> I simply assumed that there was a potential for any completion event callback
> to touch the DOM. What subset should include the check?
Not sure I understand what you are asking.  Storage can't really know what its consumers are going to do with data.

Comment 16

[Josh Matthews [:jdm]]

Ok, so your complaint is that all runnables get dropped after shutdown even if it's unnecessary, and Peter doesn't like the null checks. Is there any other way you see of avoiding this problem? I feel like we're going to have to choose one of them.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> Ok, so your complaint is that all runnables get dropped after shutdown even if
> it's unnecessary, and Peter doesn't like the null checks. Is there any other
> way you see of avoiding this problem? I feel like we're going to have to choose
> one of them.
Not really.  This is only a problem with the DOM, and storage is a lower level API than that which is why I'm very much against doing this there.

Comment 18

[Peter Van der Beken [:peterv]]

It doesn't seem like a good idea to run these callbacks that late in shutdown. It's trivial to make it leak (and probably crash too) without even touching the DOM.

Comment 19

[Marco Bonardo [:mak]]

I guess some of these are Storage implementers that do work too late, I know that some close the db connection or executes shutdown tasks at xpcom-shutdown (some don't even close it), while they should rather close the db at profile-before-change. This still doesn't ensure work has finished by xpcom-shutdown though, but makes these issues pretty rare.

Shawn, could we maybe spin the async-thread events loop at xpcom-shutdown so that we are sure all pending notifications have been served?

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> Shawn, could we maybe spin the async-thread events loop at xpcom-shutdown so
> that we are sure all pending notifications have been served?
Connections should be getting closed by xpcom-shutdown-threads, at which point the threads should be merged with the main thread (which accomplishes what you ask for).

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 537946 [details] [diff] [review]
Avoid running storage callbacks when xpcom is shutting down.

I don't think I can review this in its current state, especially with module owner not agreeing on the approach.

Comment 22

[(mostly gone) XtC4UaLL [:xtc4uall]]

FWIW, I got a Crash against this Signature on Shutdown using Nightly-Profiling Branch/Repo: bp-326775d9-373f-4069-bcfc-893392120611 (OS: Win7 x64).

Comment 23

[Takanori MATSUURA]

Crash Report: bp-280c350a-86f0-4992-a1e8-883eb2130315

Comment 24

[Bill Gianopoulos [:WG9s]]

I ran into this after clicking on Restart Now, after applying an update via Help -> About.

Comment 25

[Josh Matthews [:jdm]]

Not working on this. The way forward is to ensure that all storage consumers close any open connections when xpcom-shutdown-threads occurs like comment 20 says.

Comment 26

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.