782646 - Make JSObject::doSomethingToThisObject methods static

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

There are a bunch of instance methods like getGeneric etc. on JSObject to do some operation which can change VM state and trigger GC.  Almost all the callers root the 'this' object when calling these methods, but since 'this' cannot be a handle in the callee the callee will need to reroot the object before doing anything.  This is ugly and it would be better if the methods were statics which took the former 'this' object as a Handle parameter.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch (6627eca98f7e)

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 652408 [details] [diff] [review]
patch (6627eca98f7e)

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

In future it would be nice if you could split out orthogonal changes like Get/SetLengthProperty into a different patch.  I was surprised at how annoying this was, considering that they are both trivial changes.  I think it was the constant need to switch attention between them.  In any case, it was quite distracting.

I'm also not entirely sure how I feel about the removal of the non-receiver versions of getProperty and getGeneric.  Was there a practical reason, or was it just to condense the object interface?  I think I prefer the smaller interface personally, but I'm wondering how Waldo is going to react when he gets back.

::: js/src/jsarray.h
@@ +71,5 @@
>  extern JSObject *
>  NewSlowEmptyArray(JSContext *cx);
>  
> +extern JSBool
> +GetLengthProperty(JSContext *cx, js::HandleObject obj, uint32_t *lengthp);

Remove js::

@@ +76,3 @@
>  
>  extern JSBool
> +SetLengthProperty(JSContext *cx, js::HandleObject obj, double length);

Remove js::

::: js/src/jsinterp.cpp
@@ +3169,5 @@
>          JS_ASSERT(obj->isArray());
>          JS_ASSERT(JSID_IS_INT(id));
>          JS_ASSERT(uint32_t(JSID_TO_INT(id)) < StackSpace::ARGS_LENGTH_MAX);
>          if (JSOp(regs.pc[JSOP_INITELEM_LENGTH]) == JSOP_ENDINIT &&
> +            !SetLengthProperty(cx, obj, (uint32_t) (JSID_TO_INT(id) + 1))) {

{ on new line.

::: js/src/jsinterpinlines.h
@@ +64,5 @@
>  
>      if (IsCacheableNonGlobalScope(obj))
>          return true;
>  
> +    JSObject *nobj = JSObject::thisObject(cx, obj);

RawObject

::: js/src/jsobjinlines.h
@@ +102,2 @@
>  {
>      js::Rooted<jsid> id(cx, js::NameToId(name));

RootedId, since you are here.

@@ +119,2 @@
>  {
>      js::Rooted<jsid> id(cx, SPECIALID_TO_JSID(sid));

RootedId

@@ +136,2 @@
>  {
>      js::Rooted<jsid> id(cx, js::NameToId(name));

RootedId

@@ +152,2 @@
>  {
>      js::Rooted<jsid> id(cx, SPECIALID_TO_JSID(sid));

RootedId

@@ +182,2 @@
>  {
>      js::Rooted<jsid> id(cx, js::NameToId(name));

RootedId

@@ +1052,5 @@
>                          JSPropertyOp getter /* = JS_PropertyStub */,
>                          JSStrictPropertyOp setter /* = JS_StrictPropertyStub */,
>                          unsigned attrs /* = JSPROP_ENUMERATE */)
>  {
>      js::Rooted<jsid> id(cx, js::NameToId(name));

RootedId... I'm tired of marking these, so I'm just going to assume that you did a find-and-replace when you read the first one of these comments.  If not, please do so now.

::: js/src/jsstr.cpp
@@ +374,5 @@
>              return false;
>          value.setString(str1);
> +        if (!JSObject::defineElement(cx, obj, i, value,
> +                                     JS_PropertyStub, JS_StrictPropertyStub,
> +                                     STRING_ELEMENT_ATTRS)) {

{ on new line.

@@ +398,5 @@
>          if (!str1)
>              return JS_FALSE;
>          RootedValue value(cx, StringValue(str1));
> +        if (!JSObject::defineElement(cx, obj, uint32_t(slot), value, NULL, NULL,
> +                                     STRING_ELEMENT_ATTRS)) {

{ on new line.

@@ +1750,5 @@
>          if (!arrayobj)
>              return false;
>      }
>  
> +    RootedObject obj(cx, arrayobj);

I think we should just root arrayobj.  The "fast path" that doesn't need rooting involves an allocation, so it's going to be slow enough that putting off rooting isn't likely to be useful.

::: js/src/vm/GlobalObject.cpp
@@ +591,5 @@
>  
> +    return JSObject::defineProperty(cx, ctor, cx->runtime->atomState.classPrototypeAtom,
> +                                    protoVal, JS_PropertyStub, JS_StrictPropertyStub,
> +                                    JSPROP_PERMANENT | JSPROP_READONLY) &&
> +        JSObject::defineProperty(cx, proto, cx->runtime->atomState.constructorAtom,

Fix alignment.

Comment 3

[Brian Hackett [Laid off!]]

Having the two versions of getProperty etc. was problematic for translating as things like:

obj->getProperty(cx, obj2, id, vp)

Would compile fine and silently do the wrong thing (obj2 becomes the object *and* receiver and obj is ignored).  The shorter signature could be added now but I don't really find it that compelling to do so.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf1a005f1e61

Comment 4

[Terrence Cole [:terrence]]

(In reply to Brian Hackett (:bhackett) from comment #3)
> Having the two versions of getProperty etc. was problematic for translating
> as things like:
> 
> obj->getProperty(cx, obj2, id, vp)
> 
> Would compile fine and silently do the wrong thing (obj2 becomes the object
> *and* receiver and obj is ignored).  The shorter signature could be added
> now but I don't really find it that compelling to do so.

Makes sense, I agree.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf1a005f1e61

Comment 6

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

Comment on attachment 652408 [details] [diff] [review]
patch (6627eca98f7e)

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

::: js/src/jsgc.cpp
@@ -2230,1 @@
>  }

Just to make sure, this was intentional?

Comment 7

[Brian Hackett [Laid off!]]

(In reply to :Ms2ger from comment #6)
> Just to make sure, this was intentional?

Yes.  AutoEnumStateRooter::obj was changed to a RootedObject, which does not require explicit tracing.