1070083 - Compartment mismatch in MMI code

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

STR:

1. Flash the device with a debug build.
2. Boot the device and launch the Settings App
3. Go into "Device Information" and then click on "More Information"

That triggers a compartment mismatch.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, this code is just busted:

587 MobileConnectionRequestParent::NotifySendCancelMmiSuccess(JS::Handle<JS::Value> aResult)
588 {
589   AutoSafeJSContext cx;
590   RootedDictionary<MozMMIResult> result(cx);
591
592   if (!result.Init(cx, aResult)) {


What made people think that aResult was in the default compartment of the safe JS context, exactly?

This method should be taking a JSContext from the caller and using that.

Also, while we're here, it sure would be nice to get rid of all that manual jsapi for the additionalInformation bit: just use a sequence of unions, then check that they're all the same type.

Comment 3

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

(In reply to Boris Zbarsky [:bz] from comment #2)
> MobileConnectionRequestParent::NotifySendCancelMmiSuccess(JS::Handle<JS::
> Value> aResult)
> 588 {
> 589   AutoSafeJSContext cx;
> 590   RootedDictionary<MozMMIResult> result(cx);
> 591
> 592   if (!result.Init(cx, aResult)) {
> 
> 
> What made people think that aResult was in the default compartment of the
> safe JS context, exactly?
Compartments are black magic, and hard for reviewers to catch compartment issues.
We really need some static checking there.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The lack of debug testing on b2g means we're never going to find these issues either.

Comment 5

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

And we need to change JSAPI usage so that compartment handling is explicit.
No plain JS::Values as params. Always JSContext too.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> and hard for reviewers to catch compartment issues

Here's a simple rule of thumb in today's world.  If you're using AutoSafeJSContext, you're probably doing it wrong.....

> No plain JS::Values as params.

I was just thinking the same: can we make having a jsval param imply an implict_jscontext in xpidl?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We could, but it's not going to mean anything useful after bholley nukes JSContexts shortly is it?

Comment 8

[Boris Zbarsky [:bzbarsky]]

That's a good point...

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

[Blocking Requested - why for this release]:
this appears to be a regression from bug 843452.

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Bug 843452 wasn't in v2.1 branch so this wouldn't block 2.1, but blocks 2.2 maybe.

Comment 11

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

Edgar, could you take a look at this?

Comment 12

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

bug 1070222 is about generic improvements for compartment handling.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Hsin-Yi Tsai (OOO Sep. 22) [:hsinyi] from comment #10)
> Bug 843452 wasn't in v2.1 branch so this wouldn't block 2.1, but blocks 2.2
> maybe.

That 2.1S[4-6] means 2.2 is not confusing at all :P

Comment 14

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

Or Edgar, if you don't have time, feel free to assign this to me.
(I really should have caught this when reviewing.)

Comment 15

[Edgar Chen [:edgar] (PTO until 02/09)]

(In reply to Olli Pettay [:smaug] from comment #11)
> Edgar, could you take a look at this?

Sure.

Comment 16

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Patch, v1

Comment 17

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8493639 [details] [diff] [review]
Patch, v1

I found some other codes also have the compartment issue, I fix them in this patch as well. Could you help to review this patch for me? Thank you.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Wait.  Why the changes from AutoJSAPI to AutoSafeJSContext?

AutoSafeJSContext is deprecated and shouldn't be used in new code.  Not least because it does not enter a useful compartment!

And please do file a followup of getting rid of the array JSAPI bits in there....

Comment 19

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

Comment on attachment 8493639 [details] [diff] [review]
Patch, v1

Yeah, AutoJSAPI is the thing to use these days.
http://mxr.mozilla.org/mozilla-central/source/dom/base/ScriptSettings.h?rev=cafe6d84215b&mark=371-371#368

Comment 20

[Edgar Chen [:edgar] (PTO until 02/09)]

(In reply to Boris Zbarsky [:bz] from comment #18)
> 
> And please do file a followup of getting rid of the array JSAPI bits in
> there....

We are working on getting rid of "jsval" in mobileConnection interface for some reason (please see bug 1047196), those array JSAPI bits will be removed there as well. :)

Comment 21

[Andrew McCreight [:mccr8]]

This sounds like a bad crash, but I assume this isn't exposed to web content, which will make exploiting it difficult, so I'm marking it moderate.

Comment 22

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 1: Use the right compartment, v2

Address comment #18.
- Don't use AutoSafeJSContext.

Comment 23

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 2: Remove AutoSafeJSContext, v1

Comment 24

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8494440 [details] [diff] [review]
Part 2: Remove AutoSafeJSContext, v1

In most of cases we don't need to enter some compartment, so use AutoJSContext.

Comment 25

[Bobby Holley (:bholley)]

Comment on attachment 8494440 [details] [diff] [review]
Part 2: Remove AutoSafeJSContext, v1

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

::: dom/mobileconnection/ipc/MobileConnectionParent.cpp
@@ +433,5 @@
> +  // Junk Scope.
> +  // Note that we are going to get rid of the "jsval" used in MobileConnection's
> +  // interface in bug 1047196, then we don't need these things.
> +  AutoJSContext cx;
> +  JSAutoCompartment ac(cx, xpc::PrivilegedJunkScope());

This should be:

AutoJSAPI jsapi;
if (NS_WARN_IF(!jsapi.Init(xpc::PrivilegedJunkScope())) {
  return false;
}
JSContext* cx = jsapi.cx();


Note that using xpc::PrivilegedJunkScope requires explicit case-by-case approval from the XPConnect module owner (me), along with a comment indicating that I have approved the usage. Given the ongoing work in bug 1047196, I'm ok with this as a temporary workaround. But please do the following:

* Annotate these cases with a comment indicating that bholley has approved it.
* File a followup bug dependent on bug 1047196 to make sure that all junk scope usage has been removed from dom/mobileconnection.

Comment 26

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

(In reply to Bobby Holley (:bholley) from comment #25)

> Note that using xpc::PrivilegedJunkScope requires explicit case-by-case
> approval from the XPConnect module owner (me)

This obviously hints that our setup is rather broken. I guess we should have a talk during the work week how to make
JSAPI usage sane.

Comment 27

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

Comment on attachment 8494436 [details] [diff] [review]
Part 1: Use the right compartment, v2

I think bholley should then review these.

Comment 28

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

Comment on attachment 8494440 [details] [diff] [review]
Part 2: Remove AutoSafeJSContext, v1

... since I can't keep up with the latest xpconnect setup.

Comment 29

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

Filed bug 1072315 as a proposal to make junkscope usage a bit more understandable.

Comment 30

[Edgar Chen [:edgar] (PTO until 02/09)]

(In reply to Bobby Holley (:bholley) from comment #25)
>
> * File a followup bug dependent on bug 1047196 to make sure that all junk
> scope usage has been removed from dom/mobileconnection.

Filed bug 1072710, thank you.

Comment 31

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 1: Use the right compartment, v3

Rebase and address comment #25.

Comment 32

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 2: Remove AutoSafeJSContext, v2

Rebase and address comment #25.

Comment 33

[Edgar Chen [:edgar] (PTO until 02/09)]

Comment on attachment 8495024 [details] [diff] [review]
Part 1: Use the right compartment, v3

Hi bholley, may I have your review? Thank you.

Comment 34

[Bobby Holley (:bholley)]

Comment on attachment 8495029 [details] [diff] [review]
Part 2: Remove AutoSafeJSContext, v2

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

Looks great. Thanks for doing that. r=bholley

Comment 35

[Edgar Chen [:edgar] (PTO until 02/09)]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=39be4790fd0f

Comment 36

[Edgar Chen [:edgar] (PTO until 02/09)]

https://hg.mozilla.org/integration/b2g-inbound/rev/90fc951b321c
https://hg.mozilla.org/integration/b2g-inbound/rev/10346199c21b

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90fc951b321c
https://hg.mozilla.org/mozilla-central/rev/10346199c21b

Comment 38

[bhavana bajaj [:bajaj]]

Clearing the blocking nom for 2.2? as this is already fixed/verified on that branch per the status flag