1048330 - crash in JSAutoCompartment::JSAutoCompartment(JSContext*, JSObject*) | mozilla::dom::WrapNativeParent<nsISupports>

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

This bug was filed from the Socorro interface and is 
report bp-13273c72-cc38-46d0-bd26-646292140801.
=============================================================


This crash signature showed up as explosive on 2014-08-01 for Firefox 32, with a jump from a few crashes per day to 960 crashes on the 2014072812 build. 


Crashing thread:
0 	mozjs.dll 	JSAutoCompartment::JSAutoCompartment(JSContext*, JSObject*) 	js/src/jsapi.cpp
1 	xul.dll 	mozilla::dom::WrapNativeParent<nsISupports> 	obj-firefox/dist/include/mozilla/dom/BindingUtils.h
2 	xul.dll 	mozilla::dom::XULElementBinding::Wrap(JSContext*, nsXULElement*, nsWrapperCache*) 	obj-firefox/dom/bindings/XULElementBinding.cpp
3 	xul.dll 	nsXULElement::WrapNode(JSContext*) 	content/xul/content/src/nsXULElement.cpp
4 	xul.dll 	mozilla::dom::Element::WrapObject(JSContext*) 	content/base/src/Element.cpp
5 	xul.dll 	XPCConvert::NativeInterface2JSObject(JS::MutableHandle<JS::Value>, nsIXPConnectJSObjectHolder**, xpcObjectHelper&, nsID const*, XPCNativeInterface**, bool, tag_nsresult*) 	js/xpconnect/src/XPCConvert.cpp
6 	xul.dll 	nsXPConnect::WrapNativeToJSVal(JSContext*, JSObject*, nsISupports*, nsWrapperCache*, nsID const*, bool, JS::MutableHandle<JS::Value>) 	js/xpconnect/src/nsXPConnect.cpp
7 	xul.dll 	nsContentUtils::WrapNative(JSContext*, nsISupports*, nsWrapperCache*, nsID const*, JS::MutableHandle<JS::Value>, bool) 	content/base/src/nsContentUtils.cpp
8 	xul.dll 	nsXBLProtoImpl::InitTargetObjects(nsXBLPrototypeBinding*, nsIContent*, JS::MutableHandle<JSObject*>, bool*) 	dom/xbl/nsXBLProtoImpl.cpp
9 	xul.dll 	nsXBLProtoImpl::InstallImplementation(nsXBLPrototypeBinding*, nsXBLBinding*) 	dom/xbl/nsXBLProtoImpl.cpp
10 	xul.dll 	nsXBLBinding::InstallImplementation() 	dom/xbl/nsXBLBinding.cpp

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like a null-deref due to this line:

  JSAutoCompartment ac(cx, xblScope);

in WrapNativeParent.  Bobby, should xpc::GetXBLScope be managing to return null here???

Comment 2

[Bobby Holley (:bholley)]

(In reply to On vacation Aug 5-18.  Please do not request review. from comment #1)
> in WrapNativeParent.  Bobby, should xpc::GetXBLScope be managing to return
> null here???

Per bug 858642 it can. So the fix here is to add a null check. I'll attach a patch.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Null-check the XBL scope in more places. v1

Comment 4

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

Comment on attachment 8467411 [details] [diff] [review]
Null-check the XBL scope in more places. v1

Not sure why that NS_ERROR_OUT_OF_MEMORY is right.
I'd just use NS_ENSURE_STATE(xblScope)

We need this on branches, right?

Comment 5

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dae22380205

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8467411 [details] [diff] [review]
Null-check the XBL scope in more places. v1

This is basically a couple of missing null checks. Flagging for uplifting.

Approval Request Comment
[Feature/regressing bug #]: unknown
[User impact if declined]: crashes
[Describe test coverage new/current, TBPL]: No automated test. Just pushed to m-i.
[Risks and why]: Very low risk - null checks. 
[String/UUID change made/needed]: None

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8467411 [details] [diff] [review]
Null-check the XBL scope in more places. v1

Can't hurt to get this on esr either.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1dae22380205

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e24a524025af
https://hg.mozilla.org/releases/mozilla-beta/rev/f5df74fab22f
https://hg.mozilla.org/releases/mozilla-esr31/rev/80038bea118e

Comment 10

[Bogdan Maris [:bogdan_maris], Desktop QA]

Verified that no crashes are recorded in Socorro using that signature on Firefox 32 beta 5 and later:
https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A32.0b&range_value=1&range_unit=weeks&date=08%2F20%2F2014+09%3A00%3A00&query_search=signature&query_type=contains&query=JSAutoCompartment%3A%3AJSAutoCompartment%28JSContext*%2C+JSObject*%29+%7C+mozilla%3A%3Adom%3A%3AWrapNativeParent%3CnsISupports%3E&reason=&release_channels=&build_id=20140807212602%2C+20140811180644%2C+20140814150857%2C+20140818191513&process_type=any&hang_type=any

Aurora 4 crashes in the last 2 weeks, BUT on builds before the fix:
https://crash-stats.mozilla.com/report/list?signature=JSAutoCompartment%3A%3AJSAutoCompartment%28JSContext%2A%2C+JSObject%2A%29+%7C+mozilla%3A%3Adom%3A%3AWrapNativeParent%3CnsISupports%3E&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&version=Firefox%3A33.0a2&hang_type=any&date=2014-08-20+09%3A00%3A00&range_value=2#tab-reports

Nightly 1 crash in the last 2 weeks, BUT on build before the fix:
https://crash-stats.mozilla.com/report/list?signature=JSAutoCompartment%3A%3AJSAutoCompartment%28JSContext%2A%2C+JSObject%2A%29+%7C+mozilla%3A%3Adom%3A%3AWrapNativeParent%3CnsISupports%3E&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&version=Firefox%3A34.0a1&hang_type=any&date=2014-08-20+09%3A00%3A00&range_value=2#tab-reports

Based on the results from above, I`m marking this as Verified fixed.