Bug 563099 (compartments-api)

Compartments and wrappers API

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 12 obsolete attachments)

(Assignee)

Description

9 years ago
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
(Assignee)

Comment 1

9 years ago
This will take longer than a week. I have lots of other bugs.
(Assignee)

Updated

9 years ago
Blocks: 563106
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?
(Assignee)

Comment 3

9 years ago
Gal is working on this stuff; see bug 568731 and bug 570040.
(Assignee)

Updated

9 years ago
Depends on: 568731, 570040

Comment 4

9 years ago
Created attachment 449493 [details] [diff] [review]
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.
Assignee: general → gal

Comment 5

9 years ago
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.
Attachment #449493 - Flags: feedback?(jorendorff)

Comment 6

9 years ago
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

9 years ago
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

9 years ago
Created attachment 449505 [details] [diff] [review]
patch
Attachment #449493 - Attachment is obsolete: true
Attachment #449493 - Flags: feedback?(jorendorff)

Comment 9

9 years ago
Created attachment 449511 [details] [diff] [review]
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.
Attachment #449505 - Attachment is obsolete: true

Updated

9 years ago
Attachment #449511 - Flags: review?(jorendorff)

Comment 10

9 years ago
Created attachment 449529 [details] [diff] [review]
patch

Switch to a function's compartment when calling it.
Attachment #449511 - Attachment is obsolete: true
Attachment #449511 - Flags: review?(jorendorff)

Updated

9 years ago
Attachment #449529 - Flags: review?(jorendorff)
(Assignee)

Comment 11

9 years ago
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.
(Assignee)

Updated

9 years ago
Attachment #449529 - Flags: review?(jorendorff)

Comment 12

9 years ago
Created attachment 450299 [details] [diff] [review]
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.
Attachment #449529 - Attachment is obsolete: true

Comment 13

9 years ago
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.
Attachment #450299 - Flags: review?(jorendorff)

Comment 14

9 years ago
Created attachment 450300 [details] [diff] [review]
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.
Attachment #450299 - Attachment is obsolete: true
Attachment #450300 - Flags: review?(jorendorff)
Attachment #450299 - Flags: review?(jorendorff)

Comment 15

9 years ago
Created attachment 450607 [details] [diff] [review]
patch
Attachment #450300 - Attachment is obsolete: true
Attachment #450300 - Flags: review?(jorendorff)

Comment 16

9 years ago
Created attachment 450608 [details] [diff] [review]
patch
Attachment #450607 - Attachment is obsolete: true

Updated

9 years ago
Attachment #450608 - Flags: review?(jorendorff)

Comment 17

9 years ago
construct hook issue filed as bug 571452

Comment 18

9 years ago
Note: wrappers are now parented to the global object in the wrapping compartment.
(Assignee)

Comment 19

9 years ago
>+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!
(Assignee)

Comment 20

9 years ago
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

9 years ago
Created attachment 450870 [details] [diff] [review]
patch
Attachment #450608 - Attachment is obsolete: true
Attachment #450608 - Flags: review?(jorendorff)

Updated

9 years ago
Attachment #450870 - Flags: review?(jorendorff)
(Assignee)

Comment 22

9 years ago
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
(Assignee)

Comment 24

9 years ago
(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

9 years ago
Created attachment 451755 [details] [diff] [review]
patch
Attachment #450870 - Attachment is obsolete: true
Attachment #450870 - Flags: review?(jorendorff)

Comment 26

9 years ago
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

9 years ago
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);
}
(Assignee)

Comment 28

9 years ago
Created attachment 452277 [details] [diff] [review]
unbitrotted
Attachment #451755 - Attachment is obsolete: true
(Assignee)

Comment 29

9 years ago
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.
(Assignee)

Comment 30

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

Comment 31

9 years ago
Chatted with Andreas about this. He likes the idea of pushing a fake stack frame every time we PIERCE.
(Assignee)

Comment 32

8 years ago
Created attachment 453453 [details] [diff] [review]
patch

This one I think actually works. Rerunning tests now.

API changed pretty significantly.
Assignee: gal → jorendorff
Attachment #452277 - Attachment is obsolete: true
Attachment #453453 - Flags: review?(gal)
(Assignee)

Comment 33

8 years ago
Comment on attachment 453453 [details] [diff] [review]
patch

Crashes like crazy.
Attachment #453453 - Flags: review?(gal)
(Assignee)

Comment 34

8 years ago
Created attachment 453488 [details] [diff] [review]
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.
Attachment #453453 - Attachment is obsolete: true
Attachment #453488 - Flags: review?(gal)

Comment 35

8 years ago
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.
Attachment #453488 - Flags: review?(gal) → review+
Depends on: 574262
This was checked in to TM:

http://hg.mozilla.org/tracemonkey/rev/3aaaa21012c8
Whiteboard: fixed-in-tracemonkey
Depends on: 574294
Depends on: 575208
Depends on: 575486

Updated

8 years ago
Alias: compartments-api

Updated

8 years ago
Blocks: 576421
Depends on: 576742

Comment 37

8 years ago
http://hg.mozilla.org/mozilla-central/rev/3aaaa21012c8
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 584648
Depends on: 585257

Updated

8 years ago
Depends on: 606158
Blocks: 563127

Updated

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