677294 - need a way to create new JS scopes and run scripts against them (same compartment)

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andreas Gal :gal]

Jetpack wants to load modules into their own scope, but doesn't need compartment boundaries in between. We should have a nice clean API for this.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

A bit of additional context: this bug is the result of a conversation Andreas and I had about addressing the issue in bug 672443 that the Add-on SDK creates many compartments and thus uses a bunch of memory.

The SDK uses sandboxes to achieve module scope isolation (in particular, giving each module its own set of global objects, so modules can't modify each others' globals).  But it doesn't need the additional isolation provided by compartments (namely, an inter-compartment boundary that restricts interactions between compartments with different origins).

So it isn't necessary for SDK module scopes to be separated by compartment boundaries, and we should load them all into a single compartment to save the memory cost of compartments.  As a side effect, we might also improve the performance of inter-module interaction, since compartment boundaries apparently have some non-negligible performance cost.

So this bug is about providing an API similar to Components.utils.Sandbox that enables the SDK's module loader to create JavaScript scopes without creating a compartment for each one.

Comment 2

[Jesse Ruderman]

Won't this conflict with our "compartment per global" plans?

Comment 3

[Gabor Krizsanits (INACTIVE)]

I dived into the sandbox creation code a little, and it looks like a new compartment is not always created. If there is a compartment for the URI (of the passed in principal) than that will be used. The exception case is the nsSystemPrincipal where as always a new compartment is created. I think I could simply merge these compartments, the same way as they are already merged for principals with the same URI. Would that solve the problem? I'll still have to double check if there are any other principal that has no URI like the nsSystemPrincipal.

Could someone help me with some test ideas, or are there already some tests around for jetpack, for security checks? Just to be sure that this solution does not lead to any security leak I'm not aware of.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> If there is a compartment for the URI (of the passed in principal) than that
> will be used.

Shouldn't be.  In particular, the ptr passed to xpc_CreateGlobalObject is unique for each sandbox, iirc, and xpc::PtrAndPrincipalHashKey::KeyEquals uses that in addition to the principal itself.

Comment 5

[Gabor Krizsanits (INACTIVE)]

> In particular, the ptr passed to xpc_CreateGlobalObject is
> unique for each sandbox, iirc, and xpc::PtrAndPrincipalHashKey::KeyEquals
> uses that in addition to the principal itself.

Right, silly me. I looked only at the generated hash (xpc::PtrAndPrincipalHashKey::mSavedHash) and not the KeyEquals function before I posted my previous comment. So I was wrong here. But the idea is the same. 

The jetpack team is struggling that they don't need for each sandboxes a new compartment, and it has a lot of overhead. So if I create a new sandbox constructor function ('addonbox') or add a special flag to the orignal ('mergeCompartments') and if that flag is set, then instead of that uniqe ptr I just pass an nsnull to the xpc::PtrAndPrincipalHashKey. Slightly modifying the hash function and the KeyEquals function for this special case, only for sandboxes with different principals would be a new compartment created.

So this would be I think pretty much what the jetpack team want, a new global object for each sandbox is created, but no extra compartments (with all the not needed extra overhead like JIT cache, extra wrappers).

What I have to try is a., will this work? b., is it really safe for addons to get rid of those compartments...

Comment 6

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

Why pass nsnull?  Can't we just pass a pointer to something that is unique per jetpack?

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
>
> So if I create a new sandbox
> constructor function ('addonbox') or add a special flag to the orignal
> ('mergeCompartments')

This reminds me of bug 673331, where the patch adds an extra arg to Cu.sandbox() to provide identifying info for the sandbox.

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> Why pass nsnull?  Can't we just pass a pointer to something that is unique
> per jetpack?

Makes sense. When addons will run in their own process it will not matter, but right now it's probably a good idea to separate addons into different compartments as you suggest. Except I didn't know how to identify a sandbox call, but for that, maybe bug 673331 mentioned by Nicholas will provide a solution for me.

Comment 9

[Gabor Krizsanits (INACTIVE)]

The problem with passing something that is unique per jetpack, is that I identifying which addon/jetpack a sandbox call does belong to is not easy. Even if we can identify by passing in a name string (see bug 673331) and then look up an object from some map, anyone could simply do the same from outside the addon by creating a sandbox and using the same name.
On the other hand I don't see any important benefit for such a separation. If two compartment have the same principal the wrappers work quite transparently anyway. Blake, could you help me here if it's important to have such a separation? I tried to read up on XPCSafeJSObjectWrapper, the only wrapper what you would get in this case, but I haven't find any useful info about it, nor do I find it's implementation.

Comment 10

[Brian Warner [:warner :bwarner]]

If it helps, all the sandboxes used in a single addon will be created by the same loader (a specific piece of code inside the XPI, shipped with the SDK, written by us, not by addon authors). That loader can arrange to pass the same object into each sandbox-creating call, or it could ask for some "domain"-like object once when the addon is activated, then use that object to obtain new sandboxes for each module.

I think it might actually be ok to use a unique string for this, because (I'm assuming) the sandbox-creation code is only available through XPCOM, and thus restricted to modules that declare require("chrome"). The loader has access to the unique (at least unique within a single browser) name of the addon, and could use that as a key.

Comment 11

[Gabor Krizsanits (INACTIVE)]

I would prefer to pass in an identity object instead of the string. So I don't have to maintain a map just for this on the c++ side.

Comment 12

[Gabor Krizsanits (INACTIVE)]

Attachment: fix with using a sandboxName as addon indentifier

In the end Warner's comment (#10) convinced me, and since there is already a solution for identifying addons (bug 673331) I ended up using the same solution. So the sandboxes with the same name and same principal will share compartments.

Comment 13

[Gabor Krizsanits (INACTIVE)]

In xpc_CreateSandboxObject line 3184 the 'Identity *idPtr;' should be 
'Identity *idPtr = nsnull;'
Also there is a related bug I filled: bug 681648

Comment 14

[Gabor Krizsanits (INACTIVE)]

Attachment: some fixes / cleaning up to the previous patch

I had a talk on irc with bz, and I made some changes he proposed: using nsString instead of nsCString, and storing the name on the identity object, which makes the cleaning up part a lot nicer/simplier.

Since the sandboxName turned out not to identify the addon but it identifies the module file name the sandbox was created for, I use addonId instead of sandboxId. Each addons have a unique name Id, so it should be easy and natural to use this version from jetpack.

Comment 15

[Andreas Gal :gal]

Comment on attachment 556866 [details] [diff] [review]
some fixes / cleaning up to the previous patch

AddonId seems weird to me. This might be very useful in Chrome in general. Blake, what do you think?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 556866 [details] [diff] [review]
some fixes / cleaning up to the previous patch

Having a dependent string outlive the thing it depends on (as here) is bad.  You probably want to declare that dependent string in that innermost block, then assign it to a stack nsString that lives long enough to be passed to the sandbox.

Comment 17

[Gabor Krizsanits (INACTIVE)]

Attachment: fix for previous patch

(In reply to Boris Zbarsky (:bz) from comment #16)
> Having a dependent string outlive the thing it depends on (as here) is bad. 
> You probably want to declare that dependent string in that innermost block,
> then assign it to a stack nsString that lives long enough to be passed to
> the sandbox.

Like this? I thought that the dependent string in this case does not really depends on the jsval option, but on the string that is actually owned by the argv[1] object. But one extra memcopy here is not an issue, so I fixed it anyway just to be on the safe side.

Andreas: I'm open for any other name. Since this was an issue for the jetpack team, and there exactly the addonId will be passed here as the value, I chose that name. But if there is any other case where this feature can be used, I don't mind a name change at all.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I thought that the dependent string in this case does not really depends on the jsval
> option, but on the string that is actually owned by the argv[1] object.

It depends on the jsval option....  The new code seems fine except for the trailing space after the sandboxName declaration.

Comment 19

[Andreas Gal :gal]

Comment on attachment 557862 [details] [diff] [review]
fix for previous patch

AddonId doesn't make sense to me, we want to use this mechanism from general chrome code too. Blake, you also mentioned that you wanted an entirely different mechanism altogether. Lets touch base via vidyo today. We need clear guidance for Gabor.

Comment 20

[Gabor Krizsanits (INACTIVE)]

(In reply to Andreas Gal :gal from comment #19)

> AddonId doesn't make sense to me
It is a group of sandboxes, where compartment is shared among the ones with the same principal. So we have sandboxName, we can call this one sandboxGroup for example.

> Blake, you also mentioned that you wanted an entirely
> different mechanism altogether. 
Any info about this one? I would be really interested in it.

Comment 21

[Gabor Krizsanits (INACTIVE)]

Any update for this? Last time I talked with Blake, I was told that it is going against the long term direction we are heading, namely the compartment per global. Since jetpack should go mobile quite soon, I argued that we could still use it as a temporary solution, until the compartments will be optimized to the point where it won't matter. For jetpack modules actually we really don't need that compartment boundaries between some modules, since it comes with a cost of the extra wrappers. Plus, for a lots and lots of simple modules the 64k lower limit for compartments is a big waste right now.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 557862 [details] [diff] [review]
fix for previous patch

This looks good. My only thought is that it might be easier to have the caller pass in another sandbox (or object) from which we can get the compartment/identity object. However, if JetPack's architecture makes that hard, then I'm fine going with the runtime map.

Comment 23

[Gabor Krizsanits (INACTIVE)]

(In reply to Blake Kaplan (:mrbkap) from comment #22)
> Comment on attachment 557862 [details] [diff] [review] [diff] [details] [review]
> fix for previous patch
> 
> This looks good. My only thought is that it might be easier to have the
> caller pass in another sandbox (or object) from which we can get the
> compartment/identity object. However, if JetPack's architecture makes that
> hard, then I'm fine going with the runtime map.

Ok, I will check this with the jetpack team and send a final patch soon.

Comment 24

[Gabor Krizsanits (INACTIVE)]

Attachment: using another sandbox instance to fetch the compartment from

It is possible. Here is a version that is using another instance of sandbox to get the identity object from (to find the compartment with it).

What we don't have this way is the name of the addon the compartment/sandbox belongs to and could be used on the about:memory page. But that is another problem, and maybe we can use the sandbox name for that so I don't mind it for now.

Implementation: What I don't like about this version is that I had to include jsobj.h for the '->compartment()' call. And that I had to get the identity object from the compartment of the sandbox, and then use that identity object later (xpc_CreateGlobalObject) to find the compartment again... I would prefer to turn the JSCompartment** arg in xpc_CreateGlobalObject into an 'in/out' argument from an 'out' argument. So it would check if it points to an nsnull or to a real compartment, and if it points to something that is not nsnull it would use it as a compartment. I just don't know if I break some practice with using a ** as an 'in/out' argument instead of a pure 'out' argument. (the danger ofc, if someone is using it with an uninitialized pointer it would crash)

Anyway let me know if you like this version or I should change something on it.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #24)
> What we don't have this way is the name of the addon the compartment/sandbox
> belongs to and could be used on the about:memory page. But that is another
> problem, and maybe we can use the sandbox name for that so I don't mind it
> for now.

I'd rather use the sandbox name for that, seeing as how that was one of the main motivations.

> Implementation: What I don't like about this version is that I had to
> include jsobj.h for the '->compartment()' call. And that I had to get the

For a little less of that "icky" feeling, you can use jsgc.h js::GetObjectCompartment here.

> identity object from the compartment of the sandbox, and then use that
> identity object later (xpc_CreateGlobalObject) to find the compartment
> again... I would prefer to turn the JSCompartment** arg in
> xpc_CreateGlobalObject into an 'in/out' argument from an 'out' argument. So
> it would check if it points to an nsnull or to a real compartment, and if it
> points to something that is not nsnull it would use it as a compartment. I
> just don't know if I break some practice with using a ** as an 'in/out'
> argument instead of a pure 'out' argument. (the danger ofc, if someone is
> using it with an uninitialized pointer it would crash)

I'd be OK with this, actually. We have other places that use in/out parameters. Just make sure that there's a big comment over the function so that potential users know to be wary.

> Anyway let me know if you like this version or I should change something on
> it.

I have a couple of nitpicks to point out, but it looks ok overall.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 567716 [details] [diff] [review]
using another sandbox instance to fetch the compartment from

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

I'll r+ the next version with the in/out parameter, since I agree that'd be cleaner.

::: js/src/xpconnect/src/xpccomponents.cpp
@@ +3161,5 @@
>  NS_IMPL_ISUPPORTS0(Identity)
>  
>  nsresult
>  xpc_CreateSandboxObject(JSContext * cx, jsval * vp, nsISupports *prinOrSop, JSObject *proto,
> +                        bool wantXrays, const nsACString &sandboxName, nsISupports* identityPtr)

I know the surrounding code isn't consistent here, but this is the only parameter that uses |type* name| as opposed to |type *name|. I prefer the latter style.

@@ +3420,5 @@
>  
>              sandboxName.Adopt(tmp, strlen(tmp));
>          }
> +
> +        if (!JS_HasProperty(cx, optionsObject, "compartmentFrom", &found))

I'm not thrilled with the name, but I don't have any better suggestions. If anybody else on the CC list has one, I'd love to hear them!

@@ +3425,5 @@
> +            return NS_ERROR_INVALID_ARG;
> +
> +        if (found) {
> +            if (!JS_GetProperty(cx, optionsObject, "compartmentFrom", &option) ||
> +                !JSVAL_IS_OBJECT(option)) {

The second branch of the or statement needs to be JSVAL_IS_PRIMITIVE(option) to protect against null (which is an object, much to everybody's initial surprise).

@@ +3429,5 @@
> +                !JSVAL_IS_OBJECT(option)) {
> +                    return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval);
> +            }
> +
> +            void* privateValue = JS_GetCompartmentPrivate(

I'd probably re-wrap this as:

    void *privateValue =
        JS_GetCompartmentPrivate(cx, js::GetObjectCompartment(JSVAL_TO_Object(option)));

Comment 27

[Gabor Krizsanits (INACTIVE)]

Attachment: polished version

(In reply to Blake Kaplan (:mrbkap) from comment #25)
> I'd be OK with this, actually. We have other places that use in/out
> parameters. Just make sure that there's a big comment over the function so
> that potential users know to be wary.

Unfortunately it did not work out. The thing is that with that identity object we might get back the same compartment but not always. If the principal object is different the compartment is not shared. An in/out compartment there would either ignore the principal and do the merge anyway, which sounds bad, or would make the API harder to use (right now it's enough to pass in the same sandbox to all the sandboxes, and sandboxes with the same principals will share compartment automatically). So I use the original approach, getting the identity object and using that, to look up the compartment. The compartmentFrom is really a bad name, and even confusing. I changed it to sameGroupAs, which is still not the best one (anyone with a better idea is more than welcome), but at least it reflects more the concept behind it: For the group there will be only one compartment per principal created.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 568048 [details] [diff] [review]
polished version

This looks good to me.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Patch doesn't apply, even after fixing the filenames.

Comment 30

[Gabor Krizsanits (INACTIVE)]

Attachment: Final version, in a pullable state.

I'm not sure what is the process in this case. There is no real change in this version and the previous one, it's just an updated version (in proper pullable format) Shall I ask for another review, or shall I do another pull-request?

Comment 31

[Myk Melez [:myk] [@mykmelez]]

myk		mrbkap: do you need to re-review the updated patch in bug 677294?			2:47:34 PM
mrbkap		myk: no. I can stamp it if it would make people feel better.			2:48:34 PM
mrbkap		myk: but I trust gabor's judgement for whether it merits another review.			2:48:48 PM

Comment 32

[Gabor Krizsanits (INACTIVE)]

I discovered what was the problem with the earlier version probably. Something is wrong with the vm I'm working on and sometimes the virtual hd is getting corrupted in a random way. I've just discovered an issue like that in one of my other patches, so I will have to get rid of wm, and when I have my dev. env. up and running again without any wm involved, will have to update all my patches. This patch should be safe to be pulled since it's identical with the previous one (other than the file name and line number changes) but might contain some weird binary error (like invisible illegal character), so I don't suggest it to anyone to waste much time on it if it does not apply, in fact I think it's better to wait for a new version... Sorry for the inconveniences.

Comment 33

[Myk Melez [:myk] [@mykmelez]]

The patch applied cleanly, so I pushed it to inbound:

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

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

... and it contained \r's. Please avoid that.

Comment 35

[Marco Bonardo [:mak]]

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

Comment 36

[Nickolay_Ponomarev]

This needs a doc update and a test.

The patch seems to modify the Sandbox() <https://developer.mozilla.org/en/Components.utils.Sandbox> constructor to allow optional sameGroupAs property on the 2nd parameter). I'm not 100% sure as to what its value is supposed to be from the code...

Comment 37

[Gabor Krizsanits (INACTIVE)]

(In reply to Nickolay_Ponomarev from comment #36)
> This needs a doc update and a test.

This is an optional optimization feature. The parameter should be another sandbox, and the result is, that the new sandbox will be created in the same sandbox group. Sandboxes from the same group share compartment per principals (only one compartment for sandboxes with system principals in the same group)

This patch might be a temporary optimization, which might be changed (removed) in the future, when compartments will be optimal enough for creating one per global. Which means I'm not sure if this should be a documented feature or something only addons will use for now. 

For the tests my plan was, to integrate this into the jetpack project, and use all the jetpack test for testing it, but I will add some stand alone tests as well this week.

Comment 38

[Nickolay_Ponomarev]

> The parameter should be another sandbox
Thanks for the clarification!

> might be changed (removed) in the future
That's true for many parts of the platform :)

Note that my "this needs" comment was more of a reminder to go with the flags change and a preface for my question. I'm not in the position to tell you what to do.

Comment 39

[Gabor Krizsanits (INACTIVE)]

So as it turned out there is a problem with this patch. Which I did not detect earlier since my test was a bit unique (I had an external sandbox in my test in which the other sandboxes were created). The problem is that when someone passing in a sandbox like:

Cu.Sandbox(somePrincipal, {sameGroupAs: otherSandbox});

then this code:

+            void* privateValue =
+                JS_GetCompartmentPrivate(cx,GetObjectCompartment(JSVAL_TO_OBJECT(option)));

will ignore the context of otherSandbox and will work with the context where the Cu.Sandbox is called. 'option' here will be an ObjectProxyClass. So my question is (blake, bz, or anyone who could help me) how could I get the context of the otherSandbox here instead of the context of the proxy object?

Also, I just realized that this can be NULL:
+            identity = compartmentPrivate->key->GetPtr();
in the case when the sameGroupAs property is an object from a compartment that does not belong to a sandbox, in which case I think we should just throw.

Finally, I really want to write a simple test for this, to avoid anything like this in the future, does anyone know how can I detect (in js) if two sandbox with the same principal share compartment or not?

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

Oops, I really should have seen that. You can call js::UnwrapObject on the incoming object. It's also probably worth insisting that either it or its global object's JS class is sandbox_class.

Comment 41

[Gabor Krizsanits (INACTIVE)]

Attachment: fix for the previous version

This patch fixes the problem, but I still don't know any good test for this patch(except debugging manually), Blake: I could not avoid this time including the jsobj.h since the context we have here and the unwrapped object would fail the assertSameCompartment tests. The include of GlobalObject.h could have been avoided by casting the GlobalObject* to JSObject*, but I was suggested not to do a c type cast, so I just included that too.

Comment 42

[Gabor Krizsanits (INACTIVE)]

So I just checked this patch today, while wrote a test for it, and realized that it does not compile any longer: linker error because of the functions GlobalObject.h functions. So... would you mind not doing the sandbox_clas check? It would mean that any object from the other sandbox's compartment can be the input argument.

Comment 43

[Bobby Holley (:bholley)]

We should be able to put anything we need into jsfriendapi, no?

Comment 44

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 575856 [details] [diff] [review]
fix for the previous version

You shouldn't include vm/GlobalObject.h; you won't be able to after bug 692277.

Comment 45

[Bobby Holley (:bholley)]

Followup from IRC: JSAPI has different APIs that allow different levels of intimacy. We're trying to move XPConnect off of using spidermonkey internals directly, and instead using public APIs. jsfriendapi is a catch-all API we use when XPConnect needs something that spidermonkey doesn't otherwise want to expose.

Ms2ger is probably the best person to help you out with which functions you should be using. And you guys are in similar timezones too! ;-)

Comment 46

[Gabor Krizsanits (INACTIVE)]

Attachment: fixed linking issues

Thanks, I fixed the linking problem I had with using jsfriendapi.

Comment 47

[Myk Melez [:myk] [@mykmelez]]

(Reopening to keep on radar, as essential followup patch awaits review and landing.)

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 583147 [details] [diff] [review]
fixed linking issues

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

Sorry for the delay. With the bug being closed, I didn't see this in my review queue.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3241,2 @@
>              void* privateValue =
> +                JS_GetCompartmentPrivate(cx,GetObjectCompartment(unwrapped));

Mind adding a space after the , here?

Comment 49

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 583147 [details] [diff] [review]
fixed linking issues

>+                GetObjectJSClass(global) != &SandboxClass) {
>+                    return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval);

Indented too far.

>-            if (!compartmentPrivate || !compartmentPrivate->key)
>+            if (!compartmentPrivate || !compartmentPrivate->key) {
>                 return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval);
>+            }

XPConnect uses JS style, which doesn't brace single-line ifs. (3 times)

Comment 50

[Gabor Krizsanits (INACTIVE)]

Attachment: fixed patch

(In reply to Blake Kaplan (:mrbkap) from comment #48)
> 
> Sorry for the delay. With the bug being closed, I didn't see this in my
> review queue.

Sorry about that and thanks Myk.

(In reply to Ms2ger from comment #49)
> Indented too far.
> XPConnect uses JS style, which doesn't brace single-line ifs. (3 times)

The file and even that function itself is quite inconsistent about that but I'll keep that in mind for the future.

Could someone help me landing this patch? I don't have access to tinderbox nor to central...

Comment 51

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'll land it.

Comment 52

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/941801e7439c