650369 - Add a js::GlobalObject subtype of JSObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

Let's start using real object subtypes.  I had a patch for this with standard classes made eager (eagerness removed from this patch, for simplicity and because I ran into issues doing so), so this is a good, relatively small place to start.

Bug 566789 did this for Dates and RegExps, but it added a *Inner object which could directly access slots, as another abstraction against hard-coding slot numbers.  I haven't added that here because 1) I didn't originally, and 2) there are a lot more slots to access, so it seems more unwieldy here.  But maybe that's just my opinion.

Note that StudlyCaps.{cpp,h} is the new naming hotness per bug 648647 comment 8.  If I'm breaking a path, might as well make it as deep as possible!

Comment 1

[Nicholas Nethercote [inactive]]

Comment on attachment 526350 [details] [diff] [review]
Patch

I very much like the overall direction of this patch.  r=me with nits
addressed below.


You don't have 8 lines of context in your patch.  Maybe you need to set
"qdiff=-p -U 8" in your .hgrc file, or something like that?


> +GlobalObject *
> +GlobalObject::create(JSContext *cx, Class *clasp)
> +{
> +    JS_ASSERT(clasp->flags & JSCLASS_IS_GLOBAL);
> +
> +    JSObject *globalObj = NewNonFunction<WithProto::Given>(cx, clasp, NULL, NULL);
> +    if (!globalObj)
> +        return NULL;
> +
> +    GlobalObject *global = globalObj->asGlobal();
> +
> +    global->syncSpecialEquality();
> +
> +    /* Construct a regexp statics object for this global object. */
> +    JSObject *res = regexp_statics_construct(cx, global);
> +    if (!res)
> +        return NULL;
> +    global->setRegExpStatics(res);
> +    global->setGlobalFlags(0);
> +    return global;
> +}

In bug 566789 I used the convention that a JSObject usually had the name
"obj" and I only changed it to eg. "reobj" after calling asRegExp().  Here
you're using "globalObj" to name a JSObject and "global" for a GlobalObject.
I think "obj" and "globalObj" would be better.  But I see that "global" is
already used in some places... it's not crucial, but I'd like you to
consider whether changing the names here (and in similar places) would be an
improvement.


> +bool
> +GlobalObject::initStandardClasses(JSContext *cx)
> +{
> +    /* Native objects get their reserved slots from birth. */
> +    JS_ASSERT(numSlots() >= JSSLOT_FREE(getClass()));
> +
> +    JSAtomState &state = cx->runtime->atomState;
> +
> +    if (!defineProperty(cx, ATOM_TO_JSID(state.typeAtoms[JSTYPE_VOID]), UndefinedValue(),

The old version of this code had this comment, which seems useful, because
it's not obvious to me that state.typeAtoms[JSTYPE_VOID] corresponds to the
'undefined' property:

    /* Define a top-level property 'undefined' with the undefined value. */


> +void
> +GlobalObject::clear(JSContext *cx)
> +{
> +    /* This can return false but that doesn't mean it failed. */
> +    unbrand(cx);
> +
> +    for (int key = JSProto_Null; key < JSProto_LIMIT * 3; key++)
> +        setSlot(key, UndefinedValue());
> +
> +    /* Clear regexp statics. */
> +    RegExpStatics::extractFrom(this)->clear();
> +
> +    /* Clear the CSP eval-is-allowed cache. */
> +    getExtraSlot(EVAL_ALLOWED).setUndefined();
> +
> +    /*
> +     * Mark global as cleared. If we try to execute any compile-and-go
> +     * scripts from here on, we will throw.
> +     */
> +    int32 flags = getExtraSlot(FLAGS).toInt32();
> +    flags |= FLAGS_CLEARED;
> +    getExtraSlot(FLAGS).setInt32(flags);
> +}

You changed this code to use different slot getter/setters.  The
new methods seem to be more restrictive, ie. require that the slot be
defined, is that right?  It's a bit hard to follow with all the code being
moved around.


> +bool
> +GlobalObject::isEvalAllowed(JSContext *cx)
> +{
> +    Value &v = getExtraSlot(EVAL_ALLOWED);
> +    if (v.isUndefined()) {
> +        JSSecurityCallbacks *callbacks = JS_GetSecurityCallbacks(cx);
> +
> +        /*
> +         * If there are callbacks, make sure that the CSP callback is installed
> +         * and that it permits eval(), then cache the result.
> +         */
> +        v.setBoolean((!callbacks || !callbacks->contentSecurityPolicyAllows) ||
> +                     callbacks->contentSecurityPolicyAllows(cx));

The original code had a call to js_SetReservedSlot() here to update the
cache.  Why was it removed?


> +/*
> + * Global object slots are reserved as follows:
> + *
> + * [0, JSProto_LIMIT)
> + *   Stores the original value of the constructor for the corresponding
> + *   JSProtoKey.
> + * [JSProto_LIMIT, 2 * JSProto_LIMIT)
> + *   Stores the prototype, if any, for the constructor for the corresponding
> + *   JSProtoKey offset from JSProto_LIMIT.
> + * [2 * JSProto_LIMIT, 3 * JSProto_LIMIT)
> + *   Stores the current value of the global property named for the JSProtoKey
> + *   for the corresponding JSProtoKey offset from 2 * JSProto_LIMIT.
> + *
> + * The first two ranges are necessary for to implement js::FindClassObject,
> + * js::FindClassPrototype, and spec language speaking in terms of "the original
> + * Array prototype object", or "as if by the expression new Array()" referring
> + * to the original Array constructor.  The third range stores the (writable and
> + * even deletable) property exposed to script.
> + */

Nice comment.  But I personally dislike the use of mixed [] and () to
indicate ranges of integers (it makes more sense for real numbers).  I find
using ".." more obvious, eg:

    0 * JSProto_LIMIT .. 1 * JSProto_LIMIT - 1
    1 * JSProto_LIMIT .. 2 * JSProto_LIMIT - 1
    2 * JSProto_LIMIT .. 3 * JSProto_LIMIT - 1

But maybe that's just me.


> +    const Value &getExtraSlot(uintN i) const {
> +        JS_ASSERT(i < INDIVIDUAL_SLOT_COUNT);
> +        return getSlot(STANDARD_CLASS_SLOTS + i);
> +    }
> +    Value &getExtraSlot(uintN i) {
> +        JS_ASSERT(i < INDIVIDUAL_SLOT_COUNT);
> +        return getSlotRef(STANDARD_CLASS_SLOTS + i);
> +    }
> +
> +    void setRegExpStatics(JSObject *statics) {
> +        Value &v = getExtraSlot(REGEXP_STATICS);
> +        JS_ASSERT(v.isUndefined());
> +        v.setObject(*statics);
> +    }
> +
> +    void setEvalAllowed(bool allowed) {
> +        Value &v = getExtraSlot(EVAL_ALLOWED);
> +        JS_ASSERT(v.isUndefined());
> +        v.setBoolean(allowed);
> +    }
> +
> +    void setGlobalFlags(int32 flags) {
> +        getSlotRef(STANDARD_CLASS_SLOTS + FLAGS).setInt32(flags);
> +    }

The trend I started with all this slot reform was to have getters and
setters for every built-in slot, and avoid getSlot/setSlot-style calls as
much as possible.  You've partly done that, but you're still using
getExtraSlot() in a number of places.  Please use slot-specific
getters/setters instead;  after you've done that you'll be able to remove
getExtraSlot() altogether, because the slot-specific getters/setters can use
getSlot/setSlot directly.  (You'll need to redefine THROWTYPEERROR et al by
adding JSProto_LIMIT*3 to their values.)


> +    void setConstructorAndPrototype(JSProtoKey key, JSFunction *ctor, JSObject *proto);
> +
> +    friend JSBool
> +    ::js_SetClassObject(JSContext *cx, JSObject *obj,
> +                        JSProtoKey key, JSObject *cobj, JSObject *proto);
> +
> +    Value originalFunction(JSProtoKey key) const {
> +        return getSlot(key);
> +    }
> +
> +    Value originalPrototype(JSProtoKey key) const {
> +        return getSlot(JSProto_LIMIT + key);
> +    }

AFAICT these four declarations are dead; please remove them.


> diff --git a/js/src/Makefile.in b/js/src/Makefile.in
> --- a/js/src/Makefile.in
> +++ b/js/src/Makefile.in
> @@ -146,6 +146,7 @@ CPPSRCS		= \
>  		jsgc.cpp \
>  		jsgcchunk.cpp \
>  		jsgcstats.cpp \
> +		GlobalObject.cpp \
>  		jshash.cpp \
>  		jsinterp.cpp \
>  		jsinvoke.cpp \

That's an inventive place to put the filename, but I see why you did it so
I'll let it pass :)


> diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
> --- a/js/src/jsinterp.cpp
> +++ b/js/src/jsinterp.cpp
> @@ -6470,7 +6469,7 @@ END_CASE(JSOP_XMLPI)
>  BEGIN_CASE(JSOP_GETFUNNS)
>  {
>      Value rval;
> -    if (!js_GetFunctionNamespace(cx, &rval))
> +    if (!cx->fp()->scopeChain().getGlobal()->getFunctionNamespace(cx, &rval))

The old code has a cx->hasfp() check here, is that not necessary now?


> diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
> --- a/js/src/jsobj.cpp
> +++ b/js/src/jsobj.cpp
> @@ -1311,9 +1296,7 @@ DirectEval(JSContext *cx, const CallArgs
>  bool
>  IsBuiltinEvalForScope(JSObject *scopeChain, const Value &v)
>  {
> -    JSObject *global = scopeChain->getGlobal();
> -    JS_ASSERT((global->getClass()->flags & JSCLASS_GLOBAL_FLAGS) == JSCLASS_GLOBAL_FLAGS);
> -    return global->getReservedSlot(JSRESERVED_GLOBAL_EVAL) == v;
> +    return scopeChain->getGlobal()->getOriginalEval() == v;

Why did you remove the assert?


> diff --git a/js/src/jsobj.h b/js/src/jsobj.h
> --- a/js/src/jsobj.h
> +++ b/js/src/jsobj.h
> @@ -745,12 +746,17 @@ struct JSObject : js::gc::Cell {
>          parent = newParent;
>  
> +    js::GlobalObject *asGlobal() {
> +        JS_ASSERT(isGlobal());
> +        return reinterpret_cast<js::GlobalObject *>(this);
> +    }

In bug 566789 I put the corresponding asDate() and asRegExp() functions in
jsdate.h and jsregexp.h.  What do you think about putting this in
GlobalObject.h?  I'll let you decide.


> diff --git a/js/src/jsxml.cpp b/js/src/jsxml.cpp
> --- a/js/src/jsxml.cpp
> +++ b/js/src/jsxml.cpp
> @@ -7247,14 +7247,15 @@ js_GetFunctionNamespace(JSContext *cx, V
>           */
>          obj->clearProto();
>  
> -        vp->setObject(*obj);
> -        if (!js_SetReservedSlot(cx, global, JSRESERVED_GLOBAL_FUNCTION_NS, *vp))
> -            return false;
> -    }
> -
> +        v.setObject(*obj);
> +    }
> +
> +    *vp = v;
>      return true;
>  }

I have no idea what js_GetFunctionNamespace() does but this change is
surprising.  Can you explain?  Was the old code wrong?


> diff --git a/js/src/xpconnect/wrappers/WrapperFactory.cpp b/js/src/xpconnect/wrappers/WrapperFactory.cpp
> --- a/js/src/xpconnect/wrappers/WrapperFactory.cpp
> +++ b/js/src/xpconnect/wrappers/WrapperFactory.cpp
> @@ -113,7 +113,8 @@ WrapperFactory::WaiveXray(JSContext *cx,
>              JSAutoEnterCompartment ac;
>              if (!ac.enter(cx, obj))
>                  return nsnull;
> -            wobj = JSWrapper::New(cx, obj, proto, obj->getGlobal(), &WaiveXrayWrapperWrapper);
> +            wobj = JSWrapper::New(cx, obj, proto, JS_GetGlobalForObject(cx, obj),
> +                                  &WaiveXrayWrapperWrapper);
>              if (!wobj)
>                  return nsnull;
 
(This is a matter that predates your patch.)  Having JS_GetGlobalForObject
and getGlobal() is a bit ugly -- they're identical except the former has a
same-compartment assertion.  Are they both needed?  The obj->getGlobal()
form is more pleasing to my eye.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Adjusted for comments

(In reply to comment #1)
> You don't have 8 lines of context in your patch.  Maybe you need to set
> "qdiff=-p -U 8" in your .hgrc file, or something like that?

I upload patches directly from .hg/patches to easily include patch metadata, and I think my settings there are -pU2 to minimize merge conflicts.


> it's not crucial, but I'd like you to consider whether changing the names
> here (and in similar places) would be an improvement.

*shrug* obj/globalObj works as well as anything else, changed the few places here where I had otherwise.


> > +void
> > +GlobalObject::clear(JSContext *cx)
> > +{
> > +    /* This can return false but that doesn't mean it failed. */
> > +    unbrand(cx);
> > +
> > +    for (int key = JSProto_Null; key < JSProto_LIMIT * 3; key++)
> > +        setSlot(key, UndefinedValue());
> > +
> > +    /* Clear regexp statics. */
> > +    RegExpStatics::extractFrom(this)->clear();
> > +
> > +    /* Clear the CSP eval-is-allowed cache. */
> > +    getExtraSlot(EVAL_ALLOWED).setUndefined();
> > +
> > +    /*
> > +     * Mark global as cleared. If we try to execute any compile-and-go
> > +     * scripts from here on, we will throw.
> > +     */
> > +    int32 flags = getExtraSlot(FLAGS).toInt32();
> > +    flags |= FLAGS_CLEARED;
> > +    getExtraSlot(FLAGS).setInt32(flags);
> > +}
> 
> You changed this code to use different slot getter/setters.  The
> new methods seem to be more restrictive, ie. require that the slot be
> defined, is that right?  It's a bit hard to follow with all the code being
> moved around.

I'm not sure what you mean by "require that the slot be defined".  The slots all must exist, yes.  That's what the ensureClassReservedSlots call did in the old code, ensured that the object had its reserved slots.  But it turns out that wasn't necessary, because if you look at the NewNonFunction method that's called when the global is created, it gets InitScopeForObject'd, and that ensures the class reserved slots exist.  So the call was unnecessary, so I removed it and replaced it with an assertion.  (It didn't seem worth commenting in the code to explain this, at least not beyond the assertion, because it's mostly only interesting retrospectively.)

If you mean about getExtraSlot versus getSlot/setSlot, it originally seemed nice to segregate the one-offs from the proto-based ranks, but not doing so isn't so bad, so I've changed it.


> > +bool
> > +GlobalObject::isEvalAllowed(JSContext *cx)
> > +{
> > +    Value &v = getExtraSlot(EVAL_ALLOWED);
> > +    if (v.isUndefined()) {
> > +        JSSecurityCallbacks *callbacks = JS_GetSecurityCallbacks(cx);
> > +
> > +        /*
> > +         * If there are callbacks, make sure that the CSP callback is installed
> > +         * and that it permits eval(), then cache the result.
> > +         */
> > +        v.setBoolean((!callbacks || !callbacks->contentSecurityPolicyAllows) ||
> > +                     callbacks->contentSecurityPolicyAllows(cx));
> 
> The original code had a call to js_SetReservedSlot() here to update the
> cache.  Why was it removed?

Previously |v| was a |Value|.  Now it's a |Value&|, so setting it fills the cache without need for the rather-heavyweight js_SetReservedSlot call (which does just what this v.setBoolean does, only much more verbosely and obscurely).


> Nice comment.  But I personally dislike the use of mixed [] and () to
> indicate ranges of integers (it makes more sense for real numbers).  I find
> using ".." more obvious, eg:
> 
>     0 * JSProto_LIMIT .. 1 * JSProto_LIMIT - 1
>     1 * JSProto_LIMIT .. 2 * JSProto_LIMIT - 1
>     2 * JSProto_LIMIT .. 3 * JSProto_LIMIT - 1
> 
> But maybe that's just me.

A straw poll in #jsapi suggests it's just you, so I'm leaving this as is.  :-)


> > diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp

> >  BEGIN_CASE(JSOP_GETFUNNS)
> >  {
> >      Value rval;
> > -    if (!js_GetFunctionNamespace(cx, &rval))
> > +    if (!cx->fp()->scopeChain().getGlobal()->getFunctionNamespace(cx, &rval))
> 
> The old code has a cx->hasfp() check here, is that not necessary now?

It was never really necessary, taking into account this being the only place that called js_GetFunctionNamespace.  When you're in the interpreter you always have a frame pointer.


> > diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp

> >  bool
> >  IsBuiltinEvalForScope(JSObject *scopeChain, const Value &v)
> >  {
> > -    JSObject *global = scopeChain->getGlobal();
> > -    JS_ASSERT((global->getClass()->flags & JSCLASS_GLOBAL_FLAGS) == JSCLASS_GLOBAL_FLAGS);
> > -    return global->getReservedSlot(JSRESERVED_GLOBAL_EVAL) == v;
> > +    return scopeChain->getGlobal()->getOriginalEval() == v;
> 
> Why did you remove the assert?

JSObject::getGlobal should, and does, assert that global is one.  Hurray for choke points!


> > diff --git a/js/src/jsobj.h b/js/src/jsobj.h

> > +    js::GlobalObject *asGlobal() {
> > +        JS_ASSERT(isGlobal());
> > +        return reinterpret_cast<js::GlobalObject *>(this);
> > +    }
> 
> In bug 566789 I put the corresponding asDate() and asRegExp() functions in
> jsdate.h and jsregexp.h.  What do you think about putting this in
> GlobalObject.h?  I'll let you decide.

Works for me.


> > diff --git a/js/src/jsxml.cpp b/js/src/jsxml.cpp

> > @@ -7247,14 +7247,15 @@ js_GetFunctionNamespace(JSContext *cx, V
> >           */
> >          obj->clearProto();
> >  
> > -        vp->setObject(*obj);
> > -        if (!js_SetReservedSlot(cx, global, JSRESERVED_GLOBAL_FUNCTION_NS, *vp))
> > -            return false;
> > -    }
> > -
> > +        v.setObject(*obj);
> > +    }
> > +
> > +    *vp = v;
> >      return true;
> >  }
> 
> I have no idea what js_GetFunctionNamespace() does but this change is
> surprising.  Can you explain?  Was the old code wrong?

This is the same |Value| versus |Value&| thing, right down to the js_GetReservedSlot obfuscation.


> > diff --git a/js/src/xpconnect/wrappers/WrapperFactory.cpp b/js/src/xpconnect/wrappers/WrapperFactory.cpp

> (This is a matter that predates your patch.)  Having JS_GetGlobalForObject
> and getGlobal() is a bit ugly -- they're identical except the former has a
> same-compartment assertion.  Are they both needed?  The obj->getGlobal()
> form is more pleasing to my eye.

I agree, but the public JSAPI is exported C JS_* gunk (plus a few C++ auto-frills, but never the internal layouts of opaque symbols like JSObject).  It was an abuse of internal headers to use getGlobal(), especially when a public API existed.  (Although I didn't make these changes to reduce incest but rather to avoid needing to further expose the internal GlobalObject* as inheriting from JSObject*.)  So on the API-cleanness-of-use score card this is a win, although it's indeed somewhat of a loss for readability.

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> > 
> I'm not sure what you mean by "require that the slot be defined".

I think I was referring to the |JS_ASSERT(i < INDIVIDUAL_SLOT_COUNT);| in getExtraSlot... JS_SetReservedSlot doesn't do that, as it's a more general function.  Anyway, I'm happy with your change, land hoy!

Comment 4

[Jeff Walden [:Waldo]]

JS_SetReservedSlot does have it, but indirectly.  It asserts that the slot number is reserved, which corresponded to |i + 3*JSProto_LIMIT < RESERVED_SLOTS| if you disentangled things slightly.  But anyway.

http://hg.mozilla.org/tracemonkey/rev/c8ba5bd72b0c

Comment 5

[Jeff Walden [:Waldo]]

Backed out, some stuff was calling js_InitObjectClass that shouldn't have been (and was then recurring into j_InitFunctionAndObjectClasses, which then called js_InitObjectClass), resulting in the original eval function being stored twice, triggering an assertion that that slot was still defined to its original undefined value.  Also there may be further issues, because it looks like try is saying it causes a compartment mismatch assertion.  More debugging needed.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Extra interdiff of changes before final push

The assertions that setOriginalEval and setThrowTypeError are only called once are hopeless with the current bootstrapping code.  I was able to fix some of the errors by swapping out some js_InitObjectClass for js_InitFunctionAndObjectClasses in jsapi.cpp, but only some, and I got completely lost in labyrinthine code paths trying to trace through the setThrowTypeError failure that mochitests hit.  Commenting out just that assertion still left me hitting it in other places, and it turned into a big mess I couldn't understand.

So for now I'm commenting out both assertions, and we can enable them later when all this code is made much more sane and simple.

The remaining problem to be fixed is that getGlobal() doesn't assert same-compartment while JS_GetGlobalForObject does.  I carefully added some compartment-entering code (made syntactically non-trivial because the getGlobal appeared in a condition in an if-else if-else if-else sequence) to fix it, with some explicit bracing to ensure the compartment wasn't entered for subsequent parts of the condition sequence and consequent blocks.

I'll push the original patch with these modifications once the tree reopens.

Comment 7

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/069715a705c2 (with a mislabeled commit, alas, but I put a forwarding address in the referenced bug)

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/069715a705c2