570040 - TM: add an API for compartments

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Andreas Gal :gal]

Attachment: patch

Comment 5

[Jason Orendorff [:jorendorff]]

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

I really like the changes in jsapi.cpp, but that means we have to fix
the browser before this can land. Also please announce the API changes
on the newsgroup. It's hugely breaking but very easy to fix up; I don't
think users will mind that much.

However -- what about JS_NewObjectWithGivenProto(cx, cls, NULL, NULL)?
Do you intend to forbid that? If so, add an assert.

Also add an assertion in JS_NewObject, after NewObject() returns, like this:

-    return NewObject(cx, clasp, proto, parent);
+    JSObject *obj = NewObject(cx, clasp, proto, parent);
+    JS_ASSERT_IF(obj, obj->getParent());
+    return obj;

We depend on this always passing, right? (I'm guessing users might be
less pleased about that, but we should try.)

>+#define JSSLOT_GLOBAL_COMPARTMENT (JSProto_LIMIT * 3)

Don't use JSSLOT_ for that -- it means slot number, not reserved-slot
index. It might even make sense to rename GetGlobalObjectSlot because of
this.

In jscntxt.h:
>+    JS_FRIEND_API(AutoNewCompartment)(JSContext *cx);
>+    JS_FRIEND_API(~AutoNewCompartment)();
>[...etc.]

Is that really how you say that? Looks like it should work on all
platforms, but that's awfully weird.

jsgc.cpp, in GC:
>+    /*
>+     * Sweep compartments.
>+     */
>+    js::SweepCompartments(cx);

Heh, delete the comment please. :)

In NewCompartment:
>+    AutoLockGC lock(rt);
>+
>+    if (!rt->compartments.append(compartment)) {
>+        JS_ReportOutOfMemory(cx);
>+        return false;
>+    }

I don't know if you're allowed to JS_ReportOutOfMemory while holding
rt->gcLock. Brendan?

(If not, do we have this happening anywhere else?)

In SweepCompartments:
>+        *write = compartment;
>+        if (compartment->marked) {
>+            compartment->marked = false;
>+            ++write;

There's an idiom `*p++ = val` for writing.

    if (compartment->marked) {
        compartment->marked = false;
        *write++ = compartment;
    ...

In js_TraceObject:
>+    if (clasp->flags & JSCLASS_IS_GLOBAL)
>+        obj->getCompartment(cx)->marked = true;

This is good enough right now because if any object in a compartment is
alive, it will keep a global object alive. Cool.

Later, we can get rid of this and mark compartments in
FinalizeArenaList.

Leaving this r?me for a little while because I still haven't finished looking at the stuff in js.cpp.

Comment 6

[Jason Orendorff [:jorendorff]]

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

>+    if ((result = run(cx, argc, argv, envp)) != 0)
>+        return result;

This idiom doesn't appear anywhere else in the codebase. Splurge and get
a newline?

    result = run(cx, argc, argv, envp);
    if (result != 0)
        return result;

Comment 7

[Wesley W. Garland]

While you guys can fix the browser and spidermonkey in tandem, those of us in embedder-land can't always do this.  

Any chance we could get a JS_HAVE_NEWGLOBALOBJECT or similar #define to help us out?

Comment 8

[Andreas Gal :gal]

Wes, what do you want the define for? Source compatibility with older libraries? (not dismissing your concern, just trying to understand what you need)

Comment 9

[Andreas Gal :gal]

I don't want to forbid NewObjectWithGivenProto(NULL, NULL). Thats useful in other scenarios (i.e. making non-escaping objects that don't need a proto).

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to comment #9)
> I don't want to forbid NewObjectWithGivenProto(NULL, NULL). Thats useful in
> other scenarios (i.e. making non-escaping objects that don't need a proto).

But the implementation of JSObject::getCompartment in the patch will not work for such objects. I guess we can do without it for now since those objects don't escape. I'll have to weaken my asserts to cope with getCompartment returning NULL.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: my changes

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: updated patch

Comment 13

[Wesley W. Garland]

Andreas:

Yes, source compatibility with older spidermonkies is precisely the reason this define is needed.

It's not always possible or practical to distribute an embedding with a particular spidermonkey - keeping backwards compatibility for as long as possible when the cost approaches zero helps those of us who live on the bleeding edge: sometimes we need to bisect, sometimes we need to rollback, sometimes we need to deploy a new embedding into a production environment with a known-stable JSAPI, etc.

Comment 14

[Jason Orendorff [:jorendorff]]

Andreas, let's throw

#define JS_HAS_NEWGLOBALOBJECT 1

into jsversion.h, unconditionally. That should be good enough for what Wes wants to do, which is probably like:

    #ifndef JS_HAS_NEWGLOBALOBJECT
    #define JS_NewGlobalObject(cx, cls) JS_NewObject(cx, cls, NULL, NULL)
    #endif

Comment 15

[Brendan Eich [:brendan]]

Since you'll be defining JS_HAS_NEWGLOBALOBJECT down near JS_HAS_RESERVED_ECMA_KEYWORDS and so on, two more underscores between W/G and L/O wouldn't hurt. ;-)

/be

Comment 16

[Andreas Gal :gal]

I have a couple more changes related to this. Patch in a few.

Comment 17

[Andreas Gal :gal]

Attachment: patch

This patch includes jorendorff's changes and adds JS_MakeSystemObject and drops JS_NewSystemObject. We seem to be making global objects (BackstagePass) with non-null parent and a given proto in xpconnect. I hacked around this to support setting proto, but parent I ignore. That's just dead wrong. I think its cleared shortly thereafter anyway. Browser starts up and seems to work.

Comment 18

[Jason Orendorff [:jorendorff]]

>+JSCompartment *
>+JSObject::getCompartment(JSContext *cx) {
>+    JSObject *obj = getGlobal();
>+    jsval v = GetGlobalObjectReservedSlot(cx, obj,
                                            JSRESERVED_GLOBAL_COMPARTMENT);
>+    return (JSCompartment *) JSVAL_TO_PRIVATE(v);
>+}

This will assert or crash if we call this on a non-global object with no parent. Could you instead have it detect that case and return NULL?

> inline JSObject*
> xpc_NewSystemInheritingJSObject(JSContext *cx, JSClass *clasp, JSObject *proto,
>                                 JSObject *parent)
> {
>-    return JS_NewSystemObject(cx, clasp, proto, parent,
>-                              JS_IsSystemObject(cx, parent));
>+    JSObject *obj;
>+    if (clasp->flags & JSCLASS_IS_GLOBAL) {
>+        obj = JS_NewGlobalObject(cx, clasp);
>+        if (obj && proto)
>+            JS_SetPrototype(cx, obj, proto);
>+    } else {
>+        obj = JS_NewObject(cx, clasp, proto, parent);
>+    }
>+    if (obj && JS_IsSystemObject(cx, parent) && !JS_MakeSystemObject(cx, obj))
>+        obj = NULL;
>+    return obj;
> }

Someone else should look at this hunk. It's definitely a change in behavior; I'm not sure if it's OK or not.

Comment 19

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/51f83963d7f9

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/51f83963d7f9