1114667 - crash in js::VectorToIdArray(JSContext*, JS::AutoIdVector&, JSIdArray**)

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

This bug was filed from the Socorro interface and is 
report bp-2c216e25-0730-4d1a-a7d0-1a0382141216.
=============================================================
Crashing Thread
Frame 	Module 	Signature 	Source
0 	libxul.so 	js::VectorToIdArray(JSContext*, JS::AutoIdVector&, JSIdArray**) 	/builds/slave/b2g_m-cen_flm-kk_ntly-00000000/build/objdir-gecko/dist/include/mozilla/Atomics.h:464
1 	libxul.so 	JS_Enumerate(JSContext*, JS::Handle<JSObject*>) 	js/src/jsapi.cpp
2 	libxul.so 	nsGeolocationSettings::HandleGeolocationPerOriginSettingsChange(JS::Value const&) 	dom/geolocation/nsGeolocationSettings.cpp
3 	libxul.so 	GeolocationSettingsCallback::Handle(nsAString_internal const&, JS::Handle<JS::Value>) 	dom/geolocation/nsGeolocation.cpp
4 	libxul.so 	NS_InvokeByIndex 	xpcom/reflect/xptcall/md/unix/xptcinvoke_arm.cpp
5 	libxul.so 	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) 	js/xpconnect/src/XPCWrappedNative.cpp
6 	libxul.so 	XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp
7 	libxul.so 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/jscntxtinlines.h
8 	libxul.so 	Interpret 	js/src/vm/Interpreter.cpp
9 	libxul.so 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp
10 	libxul.so 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
11 	libxul.so 	js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
12 	libxul.so 	js::jit::DoCallFallback 	js/src/jit/BaselineIC.cpp
13 		@0xb2db8d0a

More crashes : https://crash-stats.mozilla.com/report/list?signature=js%3A%3AVectorToIdArray%28JSContext*%2C%20JS%3A%3AAutoIdVector%26%2C%20JSIdArray**%29#tab-reports

Looks like this is happening on Nightly Flame

Comment 1

[:garvan]

Due to changes in this bug?
https://bugzilla.mozilla.org/show_bug.cgi?id=1073419

Comment 2

[Dave Huseby [:huseby]]

Hrm...let me finish the JSAPI fixes and get the latest patch in Bug 1093688 in and see if that fixes this.

Comment 3

[Dave Huseby [:huseby]]

Actually, this crash is fixed by Bug 1107673 and Bug 1107681.  Once those land, this should be fixed.

Comment 4

[Dave Huseby [:huseby]]

Attachment: Bug_1114667.patch

Once Bug 1107681 lands, this patch should fix this bug.

Comment 5

[Dave Huseby [:huseby]]

Jeff, I would ask :bholley but he's "busy with media, don't ask JSAPI questions" :)  I'm trying to make sure that this code that handle geolocation settings changes is doing the right thing.

this crash is happening in a wrapped native call that happens on the main thread.  the JS::Value being passed in is a dictionary object.  My first question is around the rooting of the object.  I recently made a change for getting a proper rooting context in Bug 1107681:

> diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h
> index f05c7d2..fc629b7 100644
> --- a/dom/base/nsContentUtils.h
> +++ b/dom/base/nsContentUtils.h
> @@ -1571,16 +1571,18 @@ public:
> +  inline static JSContext *RootingCx() { return GetSafeJSContext(); }
> +  inline static JSContext *RootingCxForThread() { return GetDefaultJSContextForThread(); }

Is it good enough to just use the GetSafeJSContext() to root the object?

My second question is about constructing a JS::AutoIdArray by using JS_Enumerate.  Does JS_Enumerate trigger calling dictionary getters?  Do we need to find the global for the rooted object and create an AutoEntryScript in this case?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Dave Huseby [:huseby] from comment #5)
> Is it good enough to just use the GetSafeJSContext() to root the object?

On the main thread, yes.
 
> My second question is about constructing a JS::AutoIdArray by using
> JS_Enumerate.  Does JS_Enumerate trigger calling dictionary getters?  Do we
> need to find the global for the rooted object and create an AutoEntryScript
> in this case?

Yes.

Comment 7

[Dave Huseby [:huseby]]

Attachment: Bug_1114667.patch

Add proper rooting cx and auto entry script before creating a JS ID array using JS_Enumerate.

Try push here: http://mzl.la/1wR8Blp

Comment 8

[Dave Huseby [:huseby]]

It seems weird having the AutoEntryScript outside the call to JS_Enumerate.  Shouldn't there at least be an assert inside JS_Enumerate?  Or are there situations where that isn't necessary?

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8544391 [details] [diff] [review]
Bug_1114667.patch

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

Why is this code doing all of this with manual JSAPI? Shouldn't it be using a strongly-typed dictionary?

(In reply to Dave Huseby [:huseby] from comment #8)
> It seems weird having the AutoEntryScript outside the call to JS_Enumerate.

It's the caller's responsibility to set up JS state.

> Shouldn't there at least be an assert inside JS_Enumerate?  Or are there
> situations where that isn't necessary?

Yes, but we're not there yet.

::: dom/geolocation/nsGeolocationSettings.cpp
@@ +233,5 @@
>    // clear the hash table
>    mPerOriginSettings.Clear();
>  
> +  // root the object and get the global
> +  JSContext* cx = nsContentUtils::RootingCx();

As I said in a previous review, please don't call this cx - doing so is a terrible footgun that causes people to pass that JSContext to other JSAPI functions (as you do below), when it's in fact only valid for rooting.

@@ +247,1 @@
>    JS::AutoIdArray ids(cx, JS_Enumerate(cx, obj));

you should be using aes.cx() here.

Comment 10

[Dave Huseby [:huseby]]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #9)
> As I said in a previous review, please don't call this cx - doing so is a
> terrible footgun that causes people to pass that JSContext to other JSAPI
> functions (as you do below), when it's in fact only valid for rooting.

Can you tell me why this is a terrible footgun?  Why is the default context (for thread) only good for rooting?  What's the proper way to get a context that can be used for things other than rooting?

AFAIK, I need to root an object so that I can get its global which then identifies the proper compartment--because one compartment per global?--and then we can get a context from the global that can be used to do other things with the object and not have to worry about cross-compartment stuff?  So using the default context for rooting, allows us to get the global which gets us the correct context for the compartment?  Am I even close on my thinking?

Comment 11

[Dave Huseby [:huseby]]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #9)
> Why is this code doing all of this with manual JSAPI? Shouldn't it be using
> a strongly-typed dictionary?

Do we have any documentation and/or examples of a strongly-typed dictionary?  The code I'm patching handles callbacks when the settings db is changed.  The value being stored in the settings db can come from anywhere, including JS apps.  In this case it is coming from the Settings app.  How would I define/use/enforce a strongly typed dictionary in this case?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Dave Huseby [:huseby] from comment #10)
> (In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect
> things) from comment #9)
> > As I said in a previous review, please don't call this cx - doing so is a
> > terrible footgun that causes people to pass that JSContext to other JSAPI
> > functions (as you do below), when it's in fact only valid for rooting.
> 
> Can you tell me why this is a terrible footgun?  Why is the default context
> (for thread) only good for rooting?

Because those are the invariants of our setup. If you want to call into JSAPI, you need to have an AutoJSAPI on the stack, and you need to pass the cx obtained from that AutoJSAPI. Under the hood, this is important because the cx needs to be 'pushed' (i.e. activated) before spidermonkey can use it.

The fact that rooting requires a cx is dumb - it really should just require the runtime (and indeed, when you root with the cx, spidermonkey just grabs cx->runtime() and uses that). The only reason we had to make the RootingCx() API was because some rooted spidermonkey types require a cx (for no good reason) and neither of us had time to fix it.

> What's the proper way to get a context
> that can be used for things other than rooting?

Instantiate an AutoJSAPI (or subclass thereof) and use the cx from that.

> AFAIK, I need to root an object so that I can get its global

You need to root an object to declare it on the stack as a temporary. In your patch for this bug, you don't need the rooting at a..

> which then
> identifies the proper compartment--because one compartment per global?--and
> then we can get a context from the global that can be used to do other
> things with the object and not have to worry about cross-compartment stuff?

This is mostly right, but there are other reasons why AutoJSAPI and company need to know the global that you're starting out from. For AutoEntryScript in particular, this serves as the 'entry global', which is web-observable.

> So using the default context for rooting, allows us to get the global

Per above, not really.

(In reply to Dave Huseby [:huseby] from comment #11)
> (In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect
> things) from comment #9)
> > Why is this code doing all of this with manual JSAPI? Shouldn't it be using
> > a strongly-typed dictionary?
> 
> Do we have any documentation and/or examples of a strongly-typed dictionary?

Any dictionary type defined in a .webidl file that it used from CPP.

> The code I'm patching handles callbacks when the settings db is changed. 
> The value being stored in the settings db can come from anywhere, including
> JS apps.  In this case it is coming from the Settings app.  How would I
> define/use/enforce a strongly typed dictionary in this case?

Declare a rooted dictionary and then .Init() it?

Comment 13

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: logcat on Flame 2.2

I'm running into this crash while testing Privacy Panel.

STR:
1) Navigate to Settings > Privacy Panel > Location Accuracy
2) Toggle ON for 'Use location adjustment', tap on the dropdown list below, and check 'Custom Location'
3) Select a location when prompted
4) On the same Location Accuracy page, go to 'Add exceptions', and add custom locations to Camera and System apps.
5) Return to Privacy Panel and set a passphrase under 'Remote Privacy Protection'
6) Use another device and send a 'RPP locate [passphrase]' to DUT

DUT experiences an OS crash after step 6.

Repro rate:
3/4, it seems to be reliably occurring after a fresh flash. If DUT has crashed before using the same steps, it might not crash again.

Also attaching a logcat.

Tested on:
Device: Flame KK (full flash)
BuildID: 20150112010228
Gaia: f5e481d4caf9ffa561720a6fc9cf521a28bd8439
Gecko: bb8d6034f5f2
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (2.2 Master)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 14

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

FYI see comment 13 for steps to reproduce the crash.

Comment 15

[Josh Cheng [:josh]]

Hi Dave,
Can you check comment 12 from Bobby? Thanks!

Comment 16

[Dave Huseby [:huseby]]

I'm working on this now.  Patch in bound soon.

Comment 17

[Dave Huseby [:huseby]]

Attachment: Bug_1114667.patch

This patch fixes the rooting footgun but does not switch to using a strongly typed dictionary.  I'm just trying to fix the crash blocking v2.2.  I have another patch for Bug 1097229 that switches to using webidl definition for the GeolocationSetting dictionary and eliminates all of the hand-written JSAPI code.  Because we need to ship v2.2 soon, I'm holding off on landing the whole refactor.

Comment 18

[Dave Huseby [:huseby]]

The patch up for review is for v2.2.

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8560834 [details] [diff] [review]
Bug_1114667.patch

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

(In reply to Dave Huseby [:huseby] from comment #17)
> This patch fixes the rooting footgun but does not switch to using a strongly
> typed dictionary.  I'm just trying to fix the crash blocking v2.2. 

Makes sense.

> I have
> another patch for Bug 1097229 that switches to using webidl definition for
> the GeolocationSetting dictionary and eliminates all of the hand-written
> JSAPI code.

Thanks for doing that!

::: dom/geolocation/nsGeolocationSettings.cpp
@@ +236,5 @@
> +  // root the object and get the global
> +  JS::Rooted<JSObject*> obj(nsContentUtils::RootingCx(), &aVal.toObject());
> +  MOZ_ASSERT(obj);
> +  nsIGlobalObject* global = xpc::NativeGlobal(obj);
> +  MOZ_ASSERT(global);

Switch this to NS_ENSURE_TRUE_VOID(global && global->GetGlobalJSObject()). We need to do this, unfortunately, because AutoEntryScript initialization is not fallible.

Comment 20

[Dave Huseby [:huseby]]

Attachment: Bug_1114667.patch

:bholley r+ the previous patch with a small change.  this patch has the small change.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2edf7d18983

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2edf7d18983

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please request b2g34 and b2g37 approval for this when you get a chance.

Comment 24

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Crash occurred on build : 20150211010216 ; patch might not have landed in time for the build.  Shall we let it bake a little before the request?  Or do you feel it's safe to push?

Comment 25

[Dave Huseby [:huseby]]

Comment on attachment 8562470 [details] [diff] [review]
Bug_1114667.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1057675
User impact if declined: phone freezes and restarts when using geolocation.
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 26

[bhavana bajaj [:bajaj]]

Comment on attachment 8562470 [details] [diff] [review]
Bug_1114667.patch

[Triage Comment]

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1a45f5876ac8

Comment 29

[Ryan VanderMeulen [:RyanVM]]

nsGeolocationSettings.cpp doesn't exist on b2g34. Does something else need patching instead?

Comment 30

[Dave Huseby [:huseby]]

AFAIK we're not shipping the privacy panel in v2.1 or v2.1s.  None of my patches have ever been landed in those branches.  If I'm right, please clear out the "affected" flags or confirm I'm right and I'll do it.

Comment 31

[Josh Cheng [:josh]]

Hi Dave,
Thanks for the info, I just removed the flags on 2.1.