923836 - Set aside the first 3 reserved slots of global objects for application use

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

bz says Gecko needs three slots and that's good enough for me.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=82bc1df1ca88

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 813900 [details] [diff] [review]
v1

Review of attachment 813900 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +1601,5 @@
>  
>      /* Initialize any classes that have not been initialized yet. */
>      for (unsigned i = 0; standard_class_atoms[i].init; i++) {
>          const JSStdName &stdnm = standard_class_atoms[i];
> +        if (!obj->as<GlobalObject>().isStandardClassResolved(stdnm.clasp)) {

Is this cast safe?

::: js/src/jsobj.cpp
@@ +2190,1 @@
>              obj->setReservedSlot(slot, v);

Not setConstructorPropertySlot()? Maybe add a comment if there's a reason.

@@ +3063,5 @@
>  
>      // Now, see if the cached object matches |obj|.
>      //
>      // Note that standard class objects are cached in the range [0, JSProto_LIMIT),
>      // and the prototypes are cached in [JSProto_LIMIT, 2*JSProto_LIMIT).

Fix or remove the comment

::: js/src/jsproxy.cpp
@@ -3291,5 @@
>  
>  JS_FRIEND_API(JSObject *)
>  js_InitProxyClass(JSContext *cx, HandleObject obj)
>  {
> -    Rooted<GlobalObject*> global(cx);

Huh.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 813900 [details] [diff] [review]
v1

Review of attachment 813900 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/Class.h
@@ +562,2 @@
>  //
> +#define JSCLASS_GLOBAL_SLOT_COUNT      (3 + JSProto_LIMIT * 3 + 26)

I still think we can do better than smacking all this random junk into global object reserved slots, but this is a path forward, at least.

Why have these slots at the start of everything, rather than at the end of all slots?  (Which would have the virtue of not requiring adjusting all the arithmetic everywhere in this patch, although we probably still want the slot-numbering abstractions in any case.)

::: js/src/builtin/Intl.cpp
@@ +2051,5 @@
>                                     global, SingletonObject);
>      if (!Intl)
>          return false;
>  
> +    global->setConstructor(JSProto_Intl, ObjectValue(*Intl));

Bleh, this reads so totally wrong.  I guess we should solve this by making Intl a one-off slot or something (I can't remember what made that hard when this code was written, something must have, as I remember requesting it and then understanding why it wasn't done).  But obviously not for this bug.

::: js/src/builtin/TypedObject.cpp
@@ +1372,5 @@
>      if (!DefineConstructorAndPrototype(cx, global, JSProto_Data,
>                                         DataCtor, DataProto))
>          return false;
>  
> +    global->setConstructor(JSProto_Data, ObjectValue(*DataCtor));

All this file's changes will race against bug 914220 landing, so you're aware.

::: js/src/jsapi.cpp
@@ +1601,5 @@
>  
>      /* Initialize any classes that have not been initialized yet. */
>      for (unsigned i = 0; standard_class_atoms[i].init; i++) {
>          const JSStdName &stdnm = standard_class_atoms[i];
> +        if (!obj->as<GlobalObject>().isStandardClassResolved(stdnm.clasp)) {

I think it may be an API requirement that you only enumerate/init standard classes on global objects.  So yes, this is "safe".  We should add a clearer assert of global-ness at the start of the method, tho, so that people don't randomly assert inside too deep a stack and not understand what's implied.  Same goes for JS_ResolveStandardClass.

::: js/src/jsobj.cpp
@@ +2190,1 @@
>              obj->setReservedSlot(slot, v);

sCPS does seem more appropriate here, at a glance.

@@ +2206,5 @@
>  SetClassObject(JSObject *obj, JSProtoKey key, JSObject *cobj, JSObject *proto)
>  {
>      JS_ASSERT(!obj->getParent());
>      if (!obj->is<GlobalObject>())
>          return;

Uh.  Isn't the definition of a global, an object with null parent?  At least, that's how it was formulated in JSObject::getGlobal, once.  This deserves looking-into in some other bug.

Oh, it's detritus from when we used JS_InitClass for standard stuff.  I guess it doesn't really matter, then, and I should polish off that patch making us not use it at all in the engine.

@@ +2217,5 @@
>  ClearClassObject(JSObject *obj, JSProtoKey key)
>  {
>      JS_ASSERT(!obj->getParent());
>      if (!obj->is<GlobalObject>())
>          return;

Same.

::: js/src/jsobjinlines.h
@@ +915,5 @@
>  
>      /* Set these first in case AddTypePropertyId looks for this class. */
> +    global->setConstructor(key, ObjectValue(*ctor));
> +    global->setPrototype(key, ObjectValue(*proto));
> +    global->setConstructorPropertySlot(key, ObjectValue(*ctor));

I should have added these sorts of utility methods back a long time ago, really, to centralize the slot-number mathematics.  :-(  Sorry for not doing so!  Would have made life easier with this patch.

::: js/src/jsproxy.cpp
@@ -3291,5 @@
>  
>  JS_FRIEND_API(JSObject *)
>  js_InitProxyClass(JSContext *cx, HandleObject obj)
>  {
> -    Rooted<GlobalObject*> global(cx);

Huh**2.

::: js/src/vm/GlobalObject.cpp
@@ +365,2 @@
>          return nullptr;
> +    if (!self->addDataProperty(cx, cx->names().Function, constructorPropertySlot(JSProto_Function), 0))

Blargh this is ugh pfui.

::: js/src/vm/GlobalObject.h
@@ +33,5 @@
>  class Debugger;
>  
>  /*
>   * Global object slots are reserved as follows:
>   *

* [0, APPLICATION_SLOTS)
 *   Pre-reserved slots in all global objects that the embedding may use for any purpose it desires.  As with all reserved slots these start out as UndefinedValue(), are traced for GC purposes, and may be otherwise managed however the embedding desires.

@@ +50,5 @@
>   *   otherWindow.eval; eval(...)| as an indirect eval), a bit indicating
>   *   whether this object has been cleared (see JS_ClearScope), and a cache for
>   *   whether eval is allowed (per the global's Content Security Policy).
>   *
>   * The first two ranges are necessary to implement js::FindClassObject,

The first two JSProto_Limit-sized ranges are...

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Comment on attachment 813900 [details] [diff] [review]
> v1
> 
> Review of attachment 813900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/public/Class.h
> @@ +562,2 @@
> >  //
> > +#define JSCLASS_GLOBAL_SLOT_COUNT      (3 + JSProto_LIMIT * 3 + 26)
> 
> I still think we can do better than smacking all this random junk into
> global object reserved slots, but this is a path forward, at least.
> 
> Why have these slots at the start of everything, rather than at the end of
> all slots?  (Which would have the virtue of not requiring adjusting all the
> arithmetic everywhere in this patch, although we probably still want the
> slot-numbering abstractions in any case.)

To avoid a branch in the unwrapping code in dom/.

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> I still think we can do better than smacking all this random junk into
> global object reserved slots, but this is a path forward, at least.

Yes.

> ::: js/src/builtin/Intl.cpp
> > +    global->setConstructor(JSProto_Intl, ObjectValue(*Intl));
> 
> Bleh, this reads so totally wrong.  I guess we should solve this [...]
> [...] But obviously not for this bug.

Want a follow-up assigned to you?

> > +        if (!obj->as<GlobalObject>().isStandardClassResolved(stdnm.clasp)) {
> 
> I think it may be an API requirement that you only enumerate/init standard
> classes on global objects.  So yes, this is "safe".

I'm pretty sure it is a requirement; many of the js_InitWhateverClass functions do obj->as<GlobalObject>() first thing.

> We should add a clearer assert of global-ness at the start of the method

Done.

> >              obj->setReservedSlot(slot, v);
> 
> sCPS does seem more appropriate here, at a glance.

Done.

> @@ +2206,5 @@
> >  SetClassObject(JSObject *obj, JSProtoKey key, JSObject *cobj, JSObject *proto)
> >  {
> >      JS_ASSERT(!obj->getParent());
> >      if (!obj->is<GlobalObject>())
> >          return;
> 
> Uh.  Isn't the definition of a global, an object with null parent?  At
> least, that's how it was formulated in JSObject::getGlobal, once.  This
> deserves looking-into in some other bug.

Filed followup bug 926608.

I'm not too fond of any of our code around "standard classes". It's probably very silly.

> > +    if (!self->addDataProperty(cx, cx->names().Function, constructorPropertySlot(JSProto_Function), 0))
> 
> Blargh this is ugh pfui.

Is it bad? How worried should I be?

> * [0, APPLICATION_SLOTS)
>  *   Pre-reserved slots in all global objects [...]

Thanks.

> >   * The first two ranges are necessary to implement js::FindClassObject,
> 
> The first two JSProto_Limit-sized ranges are...

Fixed.

Comment 7

[Jason Orendorff [:jorendorff]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/0a16850fbd85

Comment 8

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> > > +    if (!self->addDataProperty(cx, cx->names().Function, constructorPropertySlot(JSProto_Function), 0))
> > 
> > Blargh this is ugh pfui.
> 
> Is it bad? How worried should I be?

Not.  This was a complaint about the code looking fugly, not about the approach being wrong or the consequences being bad.

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> Not.  This was a complaint about the code looking fugly, not about the
> approach being wrong or the consequences being bad.

You wrote this code! And it's beautiful. Compared to what we had before. :)

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0a16850fbd85