563099 - (compartments-api) Compartments and wrappers API

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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

Comment 1

[Jason Orendorff [:jorendorff]]

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

Comment 2

[Gregor Wagner [:gwagner]]

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?

Comment 3

[Jason Orendorff [:jorendorff]]

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

Comment 4

[Andreas Gal :gal]

Attachment: patch

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 5

[Andreas Gal :gal]

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.

Comment 6

[Andreas Gal :gal]

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

Comment 7

[Andreas Gal :gal]

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.

Comment 8

[Andreas Gal :gal]

Attachment: patch

Comment 9

[Andreas Gal :gal]

Attachment: patch

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.

Comment 10

[Andreas Gal :gal]

Attachment: patch

Switch to a function's compartment when calling it.

Comment 11

[Jason Orendorff [:jorendorff]]

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.

Comment 12

[Andreas Gal :gal]

Attachment: patch

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 13

[Andreas Gal :gal]

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.

Comment 14

[Andreas Gal :gal]

Attachment: patch

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.

Comment 15

[Andreas Gal :gal]

Attachment: patch

Comment 16

[Andreas Gal :gal]

Attachment: patch

Comment 17

[Andreas Gal :gal]

construct hook issue filed as bug 571452

Comment 18

[Andreas Gal :gal]

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

Comment 19

[Jason Orendorff [:jorendorff]]

>+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!

Comment 20

[Jason Orendorff [:jorendorff]]

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.

Comment 21

[Andreas Gal :gal]

Attachment: patch

Comment 22

[Jason Orendorff [:jorendorff]]

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.

Comment 23

[Brendan Eich [:brendan]]

(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

Comment 24

[Jason Orendorff [:jorendorff]]

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

Comment 25

[Andreas Gal :gal]

Attachment: patch

Comment 26

[Andreas Gal :gal]

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!");
}

Comment 27

[Andreas Gal :gal]

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);
}

Comment 28

[Jason Orendorff [:jorendorff]]

Attachment: unbitrotted

Comment 29

[Jason Orendorff [:jorendorff]]

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.

Comment 30

[Jason Orendorff [:jorendorff]]

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

Comment 31

[Jason Orendorff [:jorendorff]]

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

Comment 32

[Jason Orendorff [:jorendorff]]

Attachment: patch

This one I think actually works. Rerunning tests now.

API changed pretty significantly.

Comment 33

[Jason Orendorff [:jorendorff]]

Comment on attachment 453453 [details] [diff] [review]
patch

Crashes like crazy.

Comment 34

[Jason Orendorff [:jorendorff]]

Attachment: v3

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 35

[Andreas Gal :gal]

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.

Comment 36

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This was checked in to TM:

http://hg.mozilla.org/tracemonkey/rev/3aaaa21012c8

Comment 37

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3aaaa21012c8