A compartment is a collection of GC-things that can have direct GC-references to each other. GC arenas, GC root sets, property trees, eval caches, and JIT caches must all be per-compartment to support compartmental GC (bug 558861). A wrapper is a special object that can hold a strong GC-reference to a single GC-object in another compartment. The API for this will be small, maybe as small as the existing JSExtendedClass::wrappedObject hook for now -- but ultimately we need a bit more than that for compartmental GC. I'll be designing this API over the next week or so in: https://wiki.mozilla.org/JavaScript:SpiderMonkey:GC_Futures

This will take longer than a week. I have lots of other bugs.

I saw your posting about the API on the GC_Futures page. Are you still working on the design or is it more or less coding work now? Have you seen any problems with your design so far?

Gal is working on this stuff; see bug 568731 and bug 570040.

The wrapper is wired up to evalcx, evalcx("") makes a new global object and returns a wrapper around it. evalcx("x=1", sandbox) unwraps sandbox and then runs x=1 in that compartment.

Comment on attachment 449493 [details] [diff] [review] patch Not tested much yet but the basics work. I explain the patch in a sec, dinner first.

Demo code. sandbox is a wrapper for the code here, but inside the evalcx sandbox its the global object (touched directly). var sandbox = evalcx(""); evalcx("x=1", sandbox); print(sandbox.x); print(Proxy.isTrapping(sandbox));

Some more sandbox fun: var sandbox = evalcx(""); sandbox.print = print; evalcx("f=function() { print(\"Hello World!\"); }", sandbox); delete print; sandbox.f(); host-5-104:src gal$ ./Darwin_DBG.OBJ/js compartments.js Hello World! Sandbox is a wrapper for the script. We give it our print method and then delete it. evalcx runs with sandbox as global object, and properly picks up print, so it clearly runs with the proper (unwrapped!) scope. Membrane fun.

This patch fixes bug 570382 and makes Function.prototype.toString (and toSource) work as expected on proxies and wrappers. Wrappers can intercept the call if they want to hide the source. Compartment wrappers are transparent and show the call trap's source.

Switch to a function's compartment when calling it.

It seems like all the code in class JSProxy that looks like if (!JSVAL_IS_PRIMITIVE(handler)) do one thing and return; do something altogether different; could be simplified just by having scripted proxies store the handler object in JSSLOT_PROXY_PRIVATE and store JSScriptedProxyHandler::singleton in JSSLOT_PROXY_HANDLER. That is, scripted proxies are being treated as a special case, but they're not. JSScriptedProxyHandler is really just another subclass of JSProxyHandler. --- Please rename ValueHasher to WrapperHasher or something; or else put comments on it explaining that it's really not designed to hash general jsvals. (ValueHasher::match returns false for equal strings; and ValueHasher::hash would hash JSVAL_TO_INT(0) and JSVAL_TO_INT(1) to the same hashcode.) In jsfun.cpp: > #if JS_HAS_TOSOURCE > static JSBool > fun_toSource(JSContext *cx, uintN argc, jsval *vp) > { >- return fun_toStringHelper(cx, JS_DONT_PRETTY_PRINT, argc, vp); >+ return fun_toString(cx, argc, vp); > } > #endif The change to how fun_toString works is good generally, but this implementation detail. The existing code does it right. The patched code throws away the information "this is toSource, not toString" and then immediately has to dig it out of vp[0] again and branch on it. Why not just rename the existing static fun_toStringHelper function to fun_toString_common, if that's the issue? In jsgc.cpp, in SweepCompartments: >+ JSCompartment *compartment = (*read); This can say *read++, and you can delete the line that says: >+ ++read; In jsobj.cpp: >+static JSBool >+obj_toString(JSContext *cx, uintN argc, jsval *vp) >+{ >+ JSObject *obj = JS_THIS_OBJECT(cx, vp); >+ if (!obj) >+ return false; >+ >+ JSString *str = js::obj_toStringHelper(cx, obj); >+ if (!str) >+ return false; >+ > *vp = STRING_TO_JSVAL(str); > return JS_TRUE; > } Might as well change `JS_TRUE` to `true` while you're at it. > JSCompartment * > JSObject::getCompartment(JSContext *cx) { >- JSObject *obj = getGlobal(); >+ JSObject *obj = this; >+ if (obj->isProxy()) >+ obj = obj->unwrap(); >+ obj = obj->getGlobal(); >+ if (!obj) >+ return NULL; > JS_ASSERT(obj->getClass()->flags & JSCLASS_IS_GLOBAL); > jsval v = obj->getReservedSlot(JSRESERVED_GLOBAL_COMPARTMENT); > return (JSCompartment *) JSVAL_TO_PRIVATE(v); > } This seems wrong to me. When I ask what compartment a proxy is in, I need the proxy's compartment, not the compartment of whatever it's wrapping. In jsproxy.cpp: >+struct JSCallConstructPair { >+ jsval call; >+ jsval construct; >+}; Let's use reserved slots for this, as discussed on IRC. >+JSString * >+JSProxyHandler::obj_toString(JSContext *cx, JSObject *proxy) >+{ >+ JS_ASSERT(proxy->isProxy()); >+ return JS_InternString(cx, proxy->isFunctionProxy() >+ ? "[object Function]" >+ : "[object Object]"); >+} We probably don't want to call JS_InternString within the engine. There are a bunch of runtime-wide atoms; we can use that mechanism for this, right? >+bool >+JSProxyHandler::construct(JSContext *cx, JSObject *proxy, JSObject *receiver, >+ uintN argc, jsval *argv, jsval *rval) >+{ >+ JS_ASSERT(OperationInProgress(cx, proxy)); >+ jsval fval = GetConstruct(proxy); >+ if (fval == JSVAL_VOID) { >+ /* >+ * proxy is the constructor, so get proxy.prototype as the proto >+ * of the new object. >+ */ >+ if (!JSProxy::get(cx, proxy, receiver, ATOM_TO_JSID(ATOM(classPrototype)), rval)) >+ return false; >+ JSObject *proto = !JSVAL_IS_PRIMITIVE(*rval) ? JSVAL_TO_OBJECT(*rval) : NULL; >+ >+ JSObject *newobj = NewObject(cx, &js_ObjectClass, proto, NULL); >+ *rval = OBJECT_TO_JSVAL(newobj); >+ >+ /* If the call returns an object, return that, otherwise the original newobj. */ >+ if (!js_InternalCall(cx, newobj, GetCall(proxy), argc, argv, rval)) >+ return false; >+ if (JSVAL_IS_PRIMITIVE(*rval)) >+ *rval = OBJECT_TO_JSVAL(newobj); >+ >+ return true; >+ } >+ return js_InternalCall(cx, receiver, GetConstruct(proxy), argc, argv, rval); s/GetConstruct(proxy)/fval/ on that last line, since we already computed it above. This is a little confusing for me. My reading of the straw-man spec is that JSProxy::construct and JSProxyHandler::construct should not take a 'receiver' argument at all. The JSClass::construct hook is not the same thing as the constructTrap for function proxies, so we're probably not implementing the straw-man correctly. JSClass::construct only virtualizes the function-calling step of constructing a new object (specifically, step 8 of the [[Construct]] internal method described in ES5 13.2.2). I think the constructTrap is supposed to virtualize the entire [[Construct]] method. This further raises the question of whether the straw-man intends that |new x| can be a primitive value if x happens to be a function proxy. I imagine not; it should be brought up in the committee. For now maybe we just file a follow-up bug, unless you know how we want this to behave -- I don't. (I can explain what our existing wrappers do if anyone cares--which I doubt; this is esoterica.) In jswrapper.cpp: >+class AutoExceptionTranslator { >+ JSContext *mCx; >+ JSCompartment *mCompartment; >+ bool mOk; >+ >+ public: >+ AutoExceptionTranslator(JSContext *cx, JSCompartment *compartment) : >+ mCx(cx), mCompartment(compartment), mOk(true) { >+ JS_ASSERT(!cx->throwing); >+ } >+ >+ AutoExceptionTranslator() { This needs to be the destructor! So it wants a tilde: ~AutoExceptionTranslator(). >+ mOk = !mCx->throwing || mCompartment->wrap(mCx, &mCx->exception); It's no use assigning to mOk, right? If we fail to wrap mCx->exception, we have to ReportOutOfMemory or something, and then clear mCx->throwing and (the key part) set mCx->exception to JSVAL_VOID. >+bool >+JSCompartment::wrap(JSContext *cx, jsval *vp) I'm wary of this style of API for wrapping, because the value in *vp might be left unchanged on error. Then if the caller ignores the error, we have not just a dropped error but a probable crash (at the next GC) and a likely security hole. The mocked-up wrapper code I posted in bug 568671 always puts "wet" and "dry" values in separate variables. It's a little ritualized (as with milk and meat in a kosher kitchen), but I think it's worth it. It won't make the code much longer and we can't be too safe. (Right now the only way wrapping can fail is OOM. But I think we will have cases where wrapping will do a security check, so failures will not be hard to arrange.) >+ /* Identity is no issue for doubles, so simply always copy them. */ >+ if (JSVAL_IS_DOUBLE(*vp)) { >+ AutoCompartment(cx, this); >+ return js_NewNumberInRootedValue(cx, *JSVAL_TO_DOUBLE(*vp), vp); >+ } This is an example of a place where we would leave *vp unchanged on error. Not likely in this case; I'm just saying. Instead of using AutoCompartment in here, we should just assert at the top that cx->compartment == this. Let's require the proxy code to know what it's doing and use AutoCompartment appropriately. Crossing the boundary requires an AutoCompartment anyway... >+bool >+JSCrossCompartmentWrapper::get(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id, jsval *vp) >+{ >+ JSCompartment *local = proxy->getCompartment(cx); >+ JSCompartment *other = wrappedObject(proxy)->getCompartment(cx); >+ AutoExceptionTranslator translator(cx, local); >+ return other->wrap(cx, &receiver) && >+ other->wrapId(cx, &id) && >+ JSWrapper::get(cx, proxy, receiver, id, vp) && >+ local->wrap(cx, vp) && >+ translator.ok(); >+} This kind of code needs an AutoCompartment around just these three lines: >+ return other->wrap(cx, &receiver) && >+ other->wrapId(cx, &id) && >+ JSWrapper::get(cx, proxy, receiver, id, vp) && The fact that we don't have an AutoCompartment around the JSWrapper::get call looks like a bug to me. As noted above, checking translator.ok() doesn't do any good; you can just skip that part throughout.

It was originally designed that way that the script singleton is in the slot but I ran out of slots, but I guess I can simply use dslots now since I had to introduce a dynamic placeholder anyway.

Comment on attachment 450299 [details] [diff] [review] patch Can you take a look at the EnsureReservedSlots path? And the GC deallocating dslots for non-native objects? Looks like neither path really expects non-native objects, but I think it works anyway.

Avoid allocating dslots if we don't have a construct hook, which we almost never do. This is a little sketchy, but it seems to work.

construct hook issue filed as bug 571452

Note: wrappers are now parented to the global object in the wrapping compartment.

>+bool >+JSObject::isWrapper() const >+{ >+ return isProxy() && getProxyHandler() == &JSCrossCompartmentWrapper::singleton; >+} We should call this isCrossCompartmentWrapper(). (The alternative is to realign terminology so that "wrapper" implies cross-compartment and full membranes. I would love for that to be our reality, but it's not going to be initially.) In JSCompartment::wrap: >+ /* Unwrap incoming objects. */ >+ if (!JSVAL_IS_PRIMITIVE(*vp)) { >+ JSObject *obj = JSVAL_TO_OBJECT(*vp)->unwrap(); >+ /* If the wrapped object is already in this compartment, we are done. */ >+ if (obj->getCompartment(cx) == this) >+ return true; >+ *vp = OBJECT_TO_JSVAL(obj); >+ } This needs to set *vp before returning true! >+ /* If we already have a wrapper for this value, use it. */ >+ if (WrapperMap::Ptr p = crossCompartmentWrappers.lookup(*vp)) { >+ *vp = p->value; >+ return true; >+ } Is it worth storing wrappers for strings? >+ return crossCompartmentWrappers.put(*vp, *vp = STRING_TO_JSVAL(wrapped)); I think this is undefined behavior in C++: *vp is read and written without an intervening sequence point. It needs to be split up into multiple lines. >+ JS_ASSERT(JSVAL_IS_OBJECT(*vp)); >+ JSObject *obj = JSVAL_TO_OBJECT(*vp); The JS_ASSERT is unnecessary. JSVAL_TO_OBJECT asserts that. >+ /* >+ * We create wrappers with NULL proto and parent and then set them after >+ * adding the wrapper to the map to void infinite recursion. >+ */ >+ AutoObjectRooter proto(cx, obj->getProto()); >+ JSObject *wrapped = JSWrapper::New(cx, obj, NULL, NULL, &JSCrossCompartmentWrapper::singleton); Call this variable "wrapper" instead of "wrapped"? >+ if (!crossCompartmentWrappers.put(wrapped->getProxyPrivate(), *vp) || >+ !wrap(cx, proto.addr())) { Can you blow the stack here by wrapping an object with a long prototype chain (say, 10,000 objects)? >+bool >+JSCompartment::wrapException(JSContext *cx) { >+ JS_ASSERT(cx->compartment == this); >+ >+ if (cx->throwing) { >+ /* If wrap fails, it sets an exception, which we still report. */ >+ (void) wrap(cx, &cx->exception); >+ return false; >+ } It's a little weird to call into Wrapper::New here with an exception pending. Ordinarily while an exception is set we're doing nothing but unwinding and some very low-level cleanup; we clear cx->throwing before running finally blocks, for example. So, like this: AutoValueRooter exc(cx, cx->exception); cx->throwing = false; cx->exception = JSVAL_NULL; if (wrap(cx, exc.addr()) { cx->throwing = true; cx->exception = exc.value(); } return false; >+#define PIERCE(cx,pre,op,post) \ >+ JSCompartment *local = cx->compartment; \ >+ JSCompartment *other = wrappedObject(proxy)->getCompartment(cx); \ >+ { \ >+ AutoCompartment compartment(cx, other); \ >+ if (!(pre) || !(op)) return false; \ >+ } \ >+ return (post) && local->wrapException(cx); If op throws, the exception doesn't get wrapped. I think it has to be like this: ... ok = (pre) && (op); } return ok ? (post) : local->wrapException(cx); >+bool >+JSCrossCompartmentWrapper::hasOwn(JSContext *cx, JSObject *proxy, jsid id, bool *bp) >+{ >+ PIERCE(cx, >+ other->wrapId(cx, &id), >+ JSWrapper::has(cx, proxy, id, bp), JSWrapper::hasOwn!

In jsgc.cpp, FinalizeObject: >- if (obj->isProxy()) { >- jsval handler = obj->getProxyHandler(); >- if (JSVAL_IS_PRIMITIVE(handler)) >- ((JSProxyHandler *) JSVAL_TO_PRIVATE(handler))->finalize(cx, obj); >- } >+ if (obj->isProxy() && obj->getSlot(JSSLOT_PROXY_HANDLER) != JSVAL_VOID) >+ obj->getProxyHandler()->finalize(cx, obj); Can we put this special code into ProxyClass's finalizer instead of hacking another special case into the GC? A follow-up bug would be fine.

This line in JSCompartment::wrap is wrong: JSObject *global = cx->fp ? cx->fp->scopeChain->getGlobal() : cx->globalObject; Because we don't insert a dummy stack frame, this can set 'global' to an object in the wrong compartment. For example: var wglobal = evalcx(""); var wconcat = wglobal.Array.concat; wconcat([]); // wrapper for argument gets parented incorrectly At the time we wrap [], the top stack frame is the caller's stack frame. So the wrapper is incorrectly parented to the shell's global, not the sandbox. Note that this is NOT just because Array.concat is a fast native. Array is a slow native and it has the same problem: var wArray = wglobal.Array; wArray({}); // wrapper for argument gets parented incorrectly I think always pushing a dummy stack frame is the expedient thing here. Long-term we want to get rid of the parent chain.

(In reply to comment #22) > Long-term we want to get rid of the parent chain. There will always be a scope chain. Is this a case of parent in an object that is never on the scope chain? It looks like not. Wouldn't the same bug bite with an extrinsic scope chain? /be

(In reply to comment #23) > (In reply to comment #22) > > Long-term we want to get rid of the parent chain. > > There will always be a scope chain. Is this a case of parent in an object that > is never on the scope chain? Yes. The array created by wconcat([]) should never be on the scope chain.

host-6-186:src gal$ ./Darwin_DBG.OBJ/js compartments.js *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 1 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 Hello World! true *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 (function () {print("Hello World!");}) *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 *** Compartment mismatch 0x100514ef0 vs. 0x100513450 function () { print("Hello World!"); }

My testcase: var sandbox = evalcx(""); evalcx("x=1", sandbox); print(sandbox.x); sandbox.print = print; evalcx("f=function() { print(\"Hello World!\"); }", sandbox); sandbox.f(); evalcx("var q = ({});", sandbox); print(evalcx("q", sandbox) === evalcx("q", sandbox)); print(uneval(sandbox.f)); with (sandbox) { print(f); }

One reason we get so many mismatch warnings is that I'm warning when cx->compartment does not match the compartment of the top stack frame. The danger there is that JS_GetScopeChain or a debug API would leak an object right across the boundary. Maybe that is too strict; I'll try only warning when JS_GetScopeChain or JS_GetGlobalForScopeChain actually gets called.

Also proxy_Call doesn't call JSProxy::call(), so there really is a leak in this patch.

Chatted with Andreas about this. He likes the idea of pushing a fake stack frame every time we PIERCE.

This one I think actually works. Rerunning tests now. API changed pretty significantly.

Comment on attachment 453453 [details] [diff] [review] patch Crashes like crazy.

With the previous patch, the dummy stack frame pushed by JSAutoCompartment would cause JS_IsRunning to give the wrong answer (true when it should be false). I fixed this by having JS_IsRunning ignore dummy stack frames.

Comment on attachment 453488 [details] [diff] [review] v3 > JS_PUBLIC_API(JSObject *) >+JS_NewCompartmentAndGlobalObject(JSContext *cx, JSClass *clasp, JSCrossCompartmentCall **callp) >+{ >+ CHECK_REQUEST(cx); >+ JSCompartment *compartment = NewCompartment(cx); >+ if (!compartment) >+ return NULL; >+ >+ JSCompartment *saved = cx->compartment; >+ cx->compartment = compartment; >+ JSObject *obj = JS_NewGlobalObject(cx, clasp); >+ cx->compartment = saved; >+ >+ if (callp) { >+ *callp = JS_EnterCrossCompartmentCall(cx, obj); >+ if (!*callp) >+ return NULL; >+ } This doesn't buy as much. The caller can do it. This isn't any safer, but it forces you to use the unsafe enter/leave api instead of an auto helper. >+ PodZero(fp); >+ fp->fun = NULL; Redundant.

This was checked in to TM: http://hg.mozilla.org/tracemonkey/rev/3aaaa21012c8