Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: jorendorff, Assigned: mrbkap)

Tracking

(Depends on 1 bug)

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
Use the compartment API to divide up Gecko so that objects with different principals are always in different compartments. Use the wrapper API in XPConnect for our security wrappers. Fix what breaks (see the anti-mixing assertions mentioned in bug 563099).

In particular, wrappers and the structured clone algorithm will need to copy strings and doubles instead of passing them freely from one compartment to another.
If compartment < runtime then I don't see why immutable value types need to be copied instead of referenced. What am I missing?

/be
(In reply to comment #1)
> If compartment < runtime then I don't see why immutable value types need to be
> copied instead of referenced. What am I missing?

Er, compartment == GC heap, so that's a reason. But to explain my thinking better, isn't there a runtime-wide gcHeap still, from which proxies and other truly runtime-wide things may be allocated? If so, is there a way to cope with refs from inside the compartment to outside (whether to another compartment or to the runtime-wide heap)?

/be
(Reporter)

Comment 3

9 years ago
I was just thinking along the same lines:

Anything that caches JSStrings, like XPConnect's GetRTStringByIndex, would have to factor out their caches too. That sounds like unpleasant work to me; it would be much better to be able to share deeply-immutable gc-things across compartments. The trick is to come up with a GC strategy that supports this. More on that in a moment.

"proxies and other truly runtime-wide things" - Are proxies "truly runtime-wide"? I always imagine them as living in a compartment. The pictures in
  https://developer.mozilla.org/en/XPConnect_security_membranes
illustrate that view. Maybe I'm missing a better way to look at it. (I think my strong dislike for "Proxy"-style proxies stems about 50% from this view, so please break it if you can.)
(Reporter)

Comment 4

9 years ago
(In reply to comment #2)
> Er, compartment == GC heap, so that's a reason. But to explain my thinking
> better, isn't there a runtime-wide gcHeap still, from which proxies and other
> truly runtime-wide things may be allocated? If so, is there a way to cope with
> refs from inside the compartment to outside (whether to another compartment or
> to the runtime-wide heap)?

If we keep the locking around the GC heap, then yes, we could have a runtime-wide heap. Note that even so, the structured clone algorithm would still need to copy any strings and doubles that were allocated in a compartment's heap.

Alternatively, if we have a write barrier everywhere, we can allow strong references from one heap to deeply-immutable things in another heap. I like this, but I worry about back-channels through C++ defeating the write barrier. (The write barrier is necessary for the same reason that generational GCs need a write barrier for references from older generations to newer ones: references across such boundaries must be tracked. If heap A has a pointer to a string in heap B, and we do a local GC in heap B, we have to make sure the string gets marked, even if it's no longer directly reachable within heap B.)
(Reporter)

Comment 5

9 years ago
(In reply to comment #4)
> [...] but I worry about back-channels through C++ defeating the write barrier.

The argv array passed to a JSNative is an example. It would be a breaking API change to put a write barrier on that, yet it is a GC root.

Suppose a JSNative does JS_GetProperty(cx, someobj, "foo", &v) and the return value turns out to be a JSString from another heap. The native stashes the thing in argv[3]. Then someobj.foo gets set to some other value. Then the haep where the JSString lives does GC.

How can we guard against this? I don't think conservative stack-scanning helps.
Write barrier is new API, but we may want one anyway in due course. For now we could avoid inter-compartment refs by copying -- except: interned strings seem like something to exempt from this rule.

/be
The runtime-wide proxies I was thinking of would be for chrome looking at all content wrappers.

/be
(Reporter)

Comment 8

9 years ago
(In reply to comment #6)
> Write barrier is new API, but we may want one anyway in due course. For now we
> could avoid inter-compartment refs by copying -- except: interned strings seem
> like something to exempt from this rule.

That sounds just right to me.
(Reporter)

Updated

9 years ago
Depends on: 565730
(Reporter)

Updated

9 years ago
Depends on: 565735
(Reporter)

Updated

9 years ago
Depends on: 533592
(Reporter)

Updated

9 years ago
Depends on: 569000
(Reporter)

Updated

9 years ago
Blocks: 568886
Posted patch wip (obsolete) — Splinter Review
This doesn't compile yet: I haven't changed dom/base yet. But this is the direction I'm going. js/src and js/src/xpconnect both compile correctly. Hopefully, when this compiles and runs, it'll mean that we properly create compartments. I'm pretty sure that we're going to have to add a bunch of CrossCompartmentCall calls to a few places in XPConnect (and also nsJSContext).
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Posted patch compiling, lightly tested (obsolete) — Splinter Review
As a note: I wrote this patch on top of bug 575795 -- the browser starts and shuts down successfully. I stepped through a couple of navigations and shutdowns and compartments seem to be getting re-used correctly for global objects (and compartments seem to be correctly getting collected).
Attachment #456649 - Attachment is obsolete: true
Posted patch updated (obsolete) — Splinter Review
I just pushed this to the try server, so there might need to be a couple of followup patches, but this should be the meat of the changes.
Attachment #456967 - Attachment is obsolete: true
Attachment #457693 - Flags: review?(jst)
Attachment #457693 - Flags: review?(jorendorff)
Attachment #457693 - Flags: review?(jst) → review+
Attachment #457693 - Flags: review?(gal)
(Reporter)

Comment 12

9 years ago
Comment on attachment 457693 [details] [diff] [review]
updated

Mostly nits, just one real concern.

In jsgc.cpp, NewCompartment:
>+    JSCompartmentCallback callback = rt->compartmentCallback;
>+    if (callback && !callback(cx, compartment, JSCOMPARTMENT_NEW)) {
>+        AutoLockGC lock(rt);
>+        rt->compartments.popBack();
>+        return NULL;

I thought you had decided to drop this. Change of heart? It's fine with
me either way.

In jswrapper.h:
>-class AutoCompartment
>+class JS_FRIEND_API(AutoCompartment)

I don't think you need this change -- right? A thin wrapper around
this is already exposed in jsapi.h as JSAutoCrossCompartmentCall.
(Also it looks like you added JSAutoEnterCompartment instead of
using this; but see below about that.)

In nsXPConnect.cpp, xpc_CreateGlobalObject:
>+    XPCCompartmentMap& map = nsXPConnect::GetRuntimeInstance()->GetCompartmentMap();
>+    JSObject *tempGlobal;
>+    if(!map.Get(origin, compartment))
...
>+        tempGlobal = JS_NewCompartmentAndGlobalObject(cx, clasp, principals);
...
>+    else
...
>+        tempGlobal = JS_NewGlobalObject(cx, &xpcTempGlobalClass);

The second one should be clasp, right? What about the first one? clasp
is OK there too, right?

>+        JSAutoEnterCompartment autocompartment(cx, tempGlobal);
>+
>+        *global = tempGlobal;
>+        *compartment = tempGlobal->getCompartment(cx);
>+
>+        JS_SetCompartmentPrivate(cx, *compartment, ToNewCString(origin));
>+        map.Put(origin, *compartment);
>+    }

What is that JSAutoEnterCompartment buying us?

In nsXPConnect::InitClassesWithNewWrappedGlobal:
>+    JSAutoEnterCompartment autocompartment(ccx, compartment);
...
>         if(NS_FAILED(InitClasses(aJSContext, tempGlobal)))
>             return UnexpectedFailure(NS_ERROR_FAILURE);

Is JSAutoEnterCompartment enough to ensure that the objects created
under here are all parented correctly? I don't think it is. It seems
like some might end up being parented to the scope chain. (We need
assertions to detect that kind of tricky accidental compartment-mixing,
if we don't have them already...)

That is why I only exposed AutoCrossCompartmentCall up to now, and had
xpc_CreateSandboxObject using that.

Same comment on the other use of JSAutoEnterCompartment, in the
else-branch of xpc_CreateGlobalObject.

In xpconnect/src/xpcmaps.h:
>+typedef nsDataHashtableMT<nsCStringHashKey, JSCompartment *> XPCCompartmentMap;

Same typedef appears in xpcprivate.h. Are both necessary?

r=me with the comments addressed.
Attachment #457693 - Flags: review?(jorendorff) → review+
(In reply to comment #12)
> I thought you had decided to drop this. Change of heart? It's fine with
> me either way.

Oops, I meant to re-remove it (I thought that I was going to need it, but was wrong and didn't take it out). I still feel like it's weird to have an API that notifies only for destruction, so I'm going to leave it in.

> I don't think you need this change -- right? A thin wrapper around
> this is already exposed in jsapi.h as JSAutoCrossCompartmentCall.
> (Also it looks like you added JSAutoEnterCompartment instead of
> using this; but see below about that.)

Yeah.

> The second one should be clasp, right? What about the first one? clasp
> is OK there too, right?

Both of these should be clasp. I wasted a try server run because of this :-/.

> What is that JSAutoEnterCompartment buying us?

My impression was that in order to do any operations on any objects, the objects' compartments needed to be the current compartment.

> Is JSAutoEnterCompartment enough to ensure that the objects created
> under here are all parented correctly? I don't think it is. It seems
> like some might end up being parented to the scope chain. (We need
> assertions to detect that kind of tricky accidental compartment-mixing,
> if we don't have them already...)

Hmm, this is one of the few cases where we actually know a lot about the context in question:

  * We set aside the frame chain explicitly, so cx->fp is null.
  * We explicitly set the global object of the context to the temp global.

so we actually do know that it's enough. If you'd prefer, I could just create a cross-compartment call, though.

> That is why I only exposed AutoCrossCompartmentCall up to now, and had
> xpc_CreateSandboxObject using that.

Yeah xpc_CreateSandboxObject definitely needs to use an AutoCrossCompartmentCall.

> Same comment on the other use of JSAutoEnterCompartment, in the
> else-branch of xpc_CreateGlobalObject.

Yep.

> Same typedef appears in xpcprivate.h. Are both necessary?

Removed from xpcmaps.h.
(Reporter)

Comment 14

9 years ago
(In reply to comment #13)
> >+        JSAutoEnterCompartment autocompartment(cx, tempGlobal);
> >+
> >+        *global = tempGlobal;
> >+        *compartment = tempGlobal->getCompartment(cx);
> >+
> >+        JS_SetCompartmentPrivate(cx, *compartment, ToNewCString(origin));
> >+        map.Put(origin, *compartment);
> >+    }
> > What is that JSAutoEnterCompartment buying us?
> 
> My impression was that in order to do any operations on any objects, the
> objects' compartments needed to be the current compartment.

I think the only operation you're doing here on any object is
getCompartment(), which does not have that restriction. (It
wouldn't be very useful if it did!)

> > Is JSAutoEnterCompartment enough to ensure that the objects created
> > under here are all parented correctly? I don't think it is. It seems
> > like some might end up being parented to the scope chain. (We need
> > assertions to detect that kind of tricky accidental compartment-mixing,
> > if we don't have them already...)
> 
> Hmm, this is one of the few cases where we actually know a lot about the
> context in question:
> 
>   * We set aside the frame chain explicitly, so cx->fp is null.
>   * We explicitly set the global object of the context to the temp global.
> 
> so we actually do know that it's enough.

I don't understand this. I don't see how we know these things
at the line of code in question; if adding a few assertions in the
right places would make this clear, please add them!

But-- thinking about this makes it clear to me that
JS_Save/RestoreFrameChain(cx), and more generally any API that
affects JS_GetScopeChain(cx), should also update cx->compartment
consistently. If we do that, can we get rid of some or all of
those JSAutoEnterCompartment uses?
(Reporter)

Comment 15

9 years ago
This should also use JSAutoCrossCompartmentCall when firing events, at least. Maybe in a separate bug.

mrbkap, an updated patch would be much appreciated, at your convenience.
Posted patch UpdatedSplinter Review
This still doesn't pass all tests, but I'm down to a single type of test failure. Looking at that now.
Attachment #457693 - Attachment is obsolete: true
Attachment #457693 - Flags: review?(gal)
(In reply to comment #16)
> This still doesn't pass all tests, but I'm down to a single type of test
> failure. Looking at that now.

I lied, with the fixes to compartmentalize event handlers/<script>s, this appears to pass mochitests and jsreftests so should be good to go once it gets review.
Comment on attachment 458427 [details] [diff] [review]
Updated

IMO we should get this in... we can deal with AutoEnterCompartment/AutoCrossCompartmentCall as a followup.
Attachment #458427 - Flags: review?(gal)

Comment 19

9 years ago
Comment on attachment 458427 [details] [diff] [review]
Updated

I couldn't agree more. My patch is blocked on this, so is gregor's.
Attachment #458427 - Flags: review?(gal) → review+

Comment 21

9 years ago
http://hg.mozilla.org/mozilla-central/rev/3cdb70716688
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

8 years ago
Depends on: 658510
You need to log in before you can comment on or make changes to this bug.