Closed Bug 852436 Opened 8 years ago Closed 8 years ago

crash in XPCWrappedNative::GetNewOrUsed @ JSAutoCompartment::JSAutoCompartment

Categories

(Core :: XPConnect, defect)

22 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox21 --- unaffected
firefox22 + verified

People

(Reporter: scoobidiver, Assigned: mrbkap)

References

Details

(4 keywords)

Crash Data

Attachments

(3 files)

With the below stack trace, it first showed up in 22.0a1/20130313. There was a hole in 22.0a1/20130316 and 17 because of bug 851806, bug 851807 and 851851. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7433bc4545c9&tochange=c1a5c44ae3d8

Signature 	JSAutoCompartment::JSAutoCompartment(JSContext*, JSObject*) More Reports Search
UUID	4369d6fe-215f-48fb-b95e-c4c182130318
Date Processed	2013-03-18 18:41:21
Uptime	45
Install Age	45 seconds since version was first installed.
Install Time	2013-03-18 18:40:24
Product	Firefox
Version	22.0a1
Build ID	20130318030947
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 37 stepping 5
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0xffffffffdadadada
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x68a0, AdapterSubsysID: 107a1462, AdapterDriverVersion: 8.751.0.0
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
Processor Notes 	sp-processor05.phx1.mozilla.com_13244:2008
EMCheckCompatibility	True
Adapter Vendor ID	0x1002
Adapter Device ID	0x68a0
Total Virtual Memory	4294836224
Available Virtual Memory	3756597248
System Memory Use Percentage	46
Available Page File	6154903552
Available Physical Memory	2255372288

Frame 	Module 	Signature 	Source
0 	mozjs.dll 	JSAutoCompartment::JSAutoCompartment 	js/src/jsapi.cpp:1485
1 	xul.dll 	XPCWrappedNative::GetNewOrUsed 	js/xpconnect/src/XPCWrappedNative.cpp:530
2 	xul.dll 	XPCConvert::NativeInterface2JSObject 	js/xpconnect/src/XPCConvert.cpp:925
3 	xul.dll 	nsGlobalChromeWindow::QueryInterface 	dom/base/nsGlobalWindow.cpp:11090
4 	mozglue.dll 	je_free 	memory/mozjemalloc/jemalloc.c:6589
5 	xul.dll 	nsJSArgArray::Release 	dom/base/nsJSEnvironment.cpp:3871
6 	xul.dll 	nsCOMPtr_base::~nsCOMPtr_base 	obj-firefox/dist/include/nsAutoPtr.h:880
7 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
8 	xul.dll 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1417
9 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:384
10 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2397
11 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:333
12 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:398
13 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:431
14 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5760
15 	xul.dll 	mozilla::dom::EventHandlerNonNull::Call 	obj-firefox/dom/bindings/EventHandlerBinding.cpp:51
16 	xul.dll 	mozilla::dom::EventHandlerNonNull::Call<nsISupports*> 	obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:59
17 	xul.dll 	nsJSEventListener::HandleEvent 	dom/src/events/nsJSEventListener.cpp:250
18 	xul.dll 	nsEventListenerManager::HandleEventInternal 	content/events/src/nsEventListenerManager.cpp:994
19 	xul.dll 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:328
20 	xul.dll 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:634
21 	mozglue.dll 	je_malloc 	memory/mozjemalloc/jemalloc.c:6294
22 	xul.dll 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:575
23 	xul.dll 	nsEventStateManager::UpdateAncestorState 	content/events/src/nsEventStateManager.cpp:4761
24 	xul.dll 	testSortCallback 	content/xul/templates/src/nsXULSortService.cpp:206
25 	xul.dll 	nsEventStateManager::PostHandleEvent 	content/events/src/nsEventStateManager.cpp:3291
26 	xul.dll 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:6872
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=JSAutoCompartment%3A%3AJSAutoCompartment%28JSContext*%2C+JSObject*%29
This looks bad. Can we try to get STR? Looks like at least one reporter mentions bbc.co.uk.
Keywords: steps-wanted
(In reply to Bobby Holley (:bholley) from comment #1)
> Looks like at least one reporter mentions bbc.co.uk.
It's about a crash in 19.0.2 with a different stack trace.
See instead https://crash-stats.mozilla.com/report/list?version=Firefox:22.0a1&signature=JSAutoCompartment%3A%3AJSAutoCompartment%28JSContext*%2C+JSObject*%29
Crash Signature: [@ JSAutoCompartment::JSAutoCompartment(JSContext*, JSObject*)] → [@ JSAutoCompartment::JSAutoCompartment(JSContext*, JSObject*)] [@ JSAutoCompartment::JSAutoCompartment ]
OS: Windows 7 → All
It might be a dupe of bug 850741.
It's #11 top browser crasher in 22.0a1.
Keywords: topcrash
It seems correlated to Self-Destructing Cookies (https://addons.mozilla.org/firefox/addon/self-destructing-cookies):
  JSAutoCompartment::JSAutoCompartment|SIGSEGV (13 crashes)
     85% (11/13) vs.  18% (11/62) jid0-9XfBwUWnvPx4wWsfBWMCm4Jj69E@jetpack

  JSAutoCompartment::JSAutoCompartment(JSContext*, JSObject*)|EXCEPTION_ACCESS_VIOLATION_READ (55 crashes)
     38% (21/55) vs.   1% (33/2785) jid0-9XfBwUWnvPx4wWsfBWMCm4Jj69E@jetpack
Both crashes I've had with this signature today have been opening an alert service, could be caused by bug 782211
I had 2 crashes with this, both while the only thing going on was downloading update from Help -> About.
Actually, reading comment 6 it is possible what actually caused the crash was Sync trying to open a master password dialog alert.
(In reply to Scoobidiver from comment #3)
> It might be a dupe of bug 850741.

As we have assigned that one to Bill, we should at least have him CCed here. I'd like to get this one assigned to someone as well, even if just for investigating, who can take it?

This now is #5 topcrash on Nightly over the last 7 days, and when looking at crashes from the last 3 days' builds, it's actually #1 by a margin: https://crash-stats.mozilla.com/topcrasher/by_build_date/Firefox/22.0a1/3/browser
Blocks: 854025
(In reply to Scoobidiver from comment #5)
> It seems correlated to Self-Destructing Cookies
> (https://addons.mozilla.org/firefox/addon/self-destructing-cookies):
>   JSAutoCompartment::JSAutoCompartment|SIGSEGV (13 crashes)
>      85% (11/13) vs.  18% (11/62) jid0-9XfBwUWnvPx4wWsfBWMCm4Jj69E@jetpack
> 
>   JSAutoCompartment::JSAutoCompartment(JSContext*,
> JSObject*)|EXCEPTION_ACCESS_VIOLATION_READ (55 crashes)
>      38% (21/55) vs.   1% (33/2785) jid0-9XfBwUWnvPx4wWsfBWMCm4Jj69E@jetpack

I agree. I started having this problem after I installed self-destructing cookies. I will disable it and see if the crash goes away.
(In reply to webmaster from comment #10)
> (In reply to Scoobidiver from comment #5)
> > It seems correlated to Self-Destructing Cookies
> > (https://addons.mozilla.org/firefox/addon/self-destructing-cookies):
> >   JSAutoCompartment::JSAutoCompartment|SIGSEGV (13 crashes)
> >      85% (11/13) vs.  18% (11/62) jid0-9XfBwUWnvPx4wWsfBWMCm4Jj69E@jetpack
> > 
> >   JSAutoCompartment::JSAutoCompartment(JSContext*,
> > JSObject*)|EXCEPTION_ACCESS_VIOLATION_READ (55 crashes)
> >      38% (21/55) vs.   1% (33/2785) jid0-9XfBwUWnvPx4wWsfBWMCm4Jj69E@jetpack
> 
> I agree. I started having this problem after I installed self-destructing
> cookies. I will disable it and see if the crash goes away.

I've gone from one crash every ~15 mins to none for the past two hours after disabling it. I will continue monitoring.
I'm definitely hitting this frequently after one of the new desktop notifications is shown/interacted with. But not 100% of the time. I've seen it both from Facebook (via SocialAPI integration), and from the Copy ShortURL addon (which shows a notification after being triggered). The latter is especially interesting, because I've been using that addon for a long time.
Assignee: nobody → wmccloskey
It sounds like there's some hope of getting an STR here.
Keywords: qawanted
So I have been able to reproduce with self-destruct cookies add-on.  

STR:
0) install the self-destruct cookies add-on from amo. enable the add-ons bar
1) visit a random site. I've used yahoo.com, cnn.com and golfchannel.com
2) click the add-on icon in botton right corner and select to remove cookies when a tab is close
3) open another tab with random site.
close the tab in step 2.
4) wait some 10-15 seconds. 

tested results:
crash as reported.

note: after one one of these crashes, the add-on icon was missing from the add-ons bar. disable and re-enable the add-on fixed that.

one of my crashes: https://crash-stats.mozilla.com/report/index/bp-0a0ebf62-d938-4249-a25c-e06702130325
sync calling master password dialog does not cause this crash for me.
Thanks, this is perfect. Those steps work for me.
Bobby and I looked into this crash for a little while today. I added some printfs and now I have a decent idea of what's going on, but I don't yet know how all the pieces fit together.

We're asserting with the stack trace that's attached. The basic problem is that we're ending up with an outer window in nsWindowSH::PreCreate, which is only supposed to get inner windows. I'm not sure which caller in this stack is responsible for ensuring that no outer windows end up here. Bobby thinks maybe it's nsContentUtils::WrapNative. I'm CCing bz in case he knows more. Also, we're not sure if this outer/inner issue is actually contributing to the crash at all.

I instrumented the code to figure out the order of events. For the window in question, here's what happens:

  1. We set mJSObject to an nsOuterWindowProxy wrapper object during SetNewDocument.
  2. Shortly after that, nsGlobalWindow::FinalClose is called on the window.
  3. Later on, nsOuterWindowProxy::finalize is called on the JS object in step 1. Crucially, we do not clear the mJSObject field of the window.
  4. Then we assert with the stack that's attached. This happens before the destructor is called on the window, so it's still alive somehow.

As far as I can tell, failing to clear mJSObject is pretty busted. However, even if we did clear it, we'd still crash with a NULL pointer deref after the PreCreate hook returns a NULL parent. So that seems broken too. Also, there's the matter of this inner/outer assertion.
Keywords: reproducible
Duplicate of this bug: 854876
Actually, it looks like SetParentToWindow correctly handles the NULL case by returning an error. So setting mJSObject to NULL in nsOuterWindowProxy::finalize would at least stop us from crashing, I think.
(In reply to Bill McCloskey (:billm) from comment #19)
> Actually, it looks like SetParentToWindow correctly handles the NULL case by
> returning an error. So setting mJSObject to NULL in
> nsOuterWindowProxy::finalize would at least stop us from crashing, I think.

Yeah, we should definitely do that, unless there's something I'm missing.

The other question is what to do about outer windows passed to WrapNative. Comment 17 isn't quite right in that my belief is that it's up the caller to avoid passing an outer nsGlobalWindow to WrapNative.

I think mrbkap knows the situation here better than anyone. Blake, can you weigh on on the above two points?
Flags: needinfo?(mrbkap)
After going from crashes around every 10 minutes for weeks when I had the addon to no crashes for four days after I disabled it, I can confidently say that this crash is caused by the Self-Destructing Cookies (https://addons.mozilla.org/en-us/firefox/addon/self-destructing-cookies/) addon. Please notify me if anyone has experienced the crash and does not have the addon installed.
Attached patch Proposed fixSplinter Review
After talking to bz, we determined that this patch is safe. I'll file a new bug to finish cleaning up some of the nsWrapperCache::WrapObject failure cases.

The reason for the crash is that outer windows (and not inner windows) implement nsWrapperCache and are "DOM objects" (via IsDOMObject). They are meant to fall into that case and expect that returning null will mean that they fail to wrap (even though they don't set an exception). This patch treats all null returns from WrapObject as failure, preventing us from trying to incorrectly use the scriptable helper hooks on the outer window.
Assignee: wmccloskey → mrbkap
Status: NEW → ASSIGNED
Attachment #730480 - Flags: review?(bzbarsky)
Flags: needinfo?(mrbkap)
Blocks: 851392
Comment on attachment 730480 [details] [diff] [review]
Proposed fix

r=me, but a pox upon this no-brace style.  ;)
Attachment #730480 - Flags: review?(bzbarsky) → review+
Is the mJSObject thing an issue? Even if we're guaranteed never to access it after it's finalized, it would be nice to poison it or something. Also, could we move the IsInnerWindow assertion higher up in the callstack?
(In reply to Bill McCloskey (:billm) from comment #24)
> Is the mJSObject thing an issue? Even if we're guaranteed never to access it
> after it's finalized, it would be nice to poison it or something. Also,
> could we move the IsInnerWindow assertion higher up in the callstack?

I can definitely clear mJSObject. Moving the IsInnerWindow assertion higher is more challenging, though. The first place we call into window-specific code in this case is in nsWindowSH::PreCreate, everything above it on the stack deals with either nsISupports or nsWrapperCache. I'll attach a patch tomorrow that clears mJSObject.
Comment on attachment 730820 [details] [diff] [review]
Clear the outer window's mJSObject

This is orange on try.
Attachment #730820 - Flags: review?(bzbarsky)
I filed bug 855891 on figuring out what to do with the rest of the WrapObject callers.
https://hg.mozilla.org/mozilla-central/rev/a6762dd7a646
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment on attachment 730480 [details] [diff] [review]
Proposed fix

Review of attachment 730480 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCConvert.cpp
@@ +845,3 @@
>              }
>  
>              if (flat) {

This check looks awfully dead now.
(In reply to :Ms2ger from comment #32)
> This check looks awfully dead now.

It isn't in the case that there was a cached DOM object already in the cache.
Could we keep this open until the other patch lands, or else move that to a different bug? I want to make sure we don't lose track of it.
Is this supposed to be fixed in the latest Nightly? I got https://crash-stats.mozilla.com/report/index/bp-ba40a80a-8b5e-454f-89ab-20cb02130401 just a few minutes ago with the 2013-3-31 Nightly and clicking a video from the videos listed after the video has ended.
The buildid (20130330030828) in that crash report indicates you were actually running the 3/30 nightly.  Therefore, It did NOT contain the fix here.
Duplicate of this bug: 854025
(In reply to Bill McCloskey (:billm) from comment #34)
> Could we keep this open until the other patch lands, or else move that to a
> different bug? I want to make sure we don't lose track of it.

I filed bug 856818 on the other patch.
Duplicate of this bug: 850741
Duplicate of this bug: 851392
Verified that Firefox 22 RC1 does not crash when using the steps from Comment 14 (verified on Mac OS X 10.7, Windows 7 and Ubuntu 12.10)

Checked the Socorro reports and there are around 700 crashes related with the signature:[@ JSAutoCompartment::JSAutoCompartment(JSContext*, JSObject*)]

https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=JSAutoCompartment%3A%3AJSAutoCompartment%28JSContext%2A%2C%20JSObject%2A%29

Is this expected in any way?
(In reply to Simona B [QA] from comment #41)
> Is this expected in any way?

It's only expected in that JSAutoCompartment is a relatively likely place for us to crash if something goes wrong so we tend to collect possibly-unrelated crashes there. The remaining crashes aren't related directly to the bug that was fixed here.
Thanks Blake!
(In reply to Blake Kaplan (:mrbkap) from comment #42)
> (In reply to Simona B [QA] from comment #41)
> > Is this expected in any way?
> 
> It's only expected in that JSAutoCompartment is a relatively likely place
> for us to crash if something goes wrong so we tend to collect
> possibly-unrelated crashes there. The remaining crashes aren't related
> directly to the bug that was fixed here.

Does that mean we should add this frame to the "prefix list" in Socorro and have the next frame added to it to form a signature (which hopefully will be more specific to a certain problem then)?
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #44)
> Does that mean we should add this frame to the "prefix list" in Socorro and
> have the next frame added to it to form a signature (which hopefully will be
> more specific to a certain problem then)?

Adding the JSAutoCompartment constructor to the prefix list would be a good idea, yes.
Depends on: 885443
You need to log in before you can comment on or make changes to this bug.