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

RESOLVED INACTIVE

Status

()

Core
DOM
--
critical
RESOLVED INACTIVE
7 years ago
4 days ago

People

(Reporter: jdm, Unassigned)

Tracking

({crash})

Trunk
All
Linux
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
Component: General → DOM
Product: Fennec → Core
QA Contact: general → general
(Reporter)

Comment 1

7 years ago
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.
I can't see us doing the latter approach, so I think the former approach is the only way to go.
(Reporter)

Updated

7 years ago
Summary: crash [@ nsDOMClassInfo::WrapNative] after XPCOM shutdown → crash [@ nsDOMClassInfo::WrapNative] [@ WrapNative] after XPCOM shutdown
(Reporter)

Comment 3

7 years ago
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.
(Reporter)

Comment 4

7 years ago
Created attachment 536662 [details] [diff] [review]
Avoid trying to use xpconnect after it doesn't exist.

Pushed to try: http://tbpl.mozilla.org/?tree=Try&pusher=josh@joshmatthews.net&rev=a885064f2e1f
(Reporter)

Updated

7 years ago
Assignee: nobody → josh
(Reporter)

Comment 5

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

Try was all green.
Attachment #536662 - Flags: review?(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?
Attachment #536662 - Flags: review?(jst) → review?(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"?
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.
(Reporter)

Comment 9

7 years ago
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?
(Reporter)

Comment 10

7 years ago
Created attachment 537946 [details] [diff] [review]
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?
(Reporter)

Updated

7 years ago
Attachment #536662 - Attachment is obsolete: true
Attachment #536662 - Flags: review?(peterv)
(Reporter)

Updated

7 years ago
Attachment #537946 - Flags: review?(mak77)
(Assignee)

Updated

7 years ago
Crash Signature: [@ nsDOMClassInfo::WrapNative] [@ WrapNative]
(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.
(Reporter)

Comment 12

7 years ago
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.
Crash Signature: [@ nsDOMClassInfo::WrapNative] [@ WrapNative] → [@ nsDOMClassInfo::WrapNative] [@ WrapNative]
(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?
(Reporter)

Comment 14

7 years ago
I simply assumed that there was a potential for any completion event callback to touch the DOM. What subset should include the check?
(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.
(Reporter)

Comment 16

7 years ago
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.
(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.
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.
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?
(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 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.
Attachment #537946 - Flags: review?(mak77)
FWIW, I got a Crash against this Signature on Shutdown using Nightly-Profiling Branch/Repo: bp-326775d9-373f-4069-bcfc-893392120611 (OS: Win7 x64).
I ran into this after clicking on Restart Now, after applying an update via Help -> About.
(Reporter)

Comment 25

4 years ago
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.
Assignee: josh → nobody

Comment 26

4 days ago
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Last Resolved: 4 days ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.