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

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.)

(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.)

(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

(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.

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).

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).

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.

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.

(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.

(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?

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.

This still doesn't pass all tests, but I'm down to a single type of test failure. Looking at that now.

(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.

Comment on attachment 458427 [details] [diff] [review] Updated I couldn't agree more. My patch is blocked on this, so is gregor's.