Last Comment Bug 782646 - Make JSObject::doSomethingToThisObject methods static
: Make JSObject::doSomethingToThisObject methods static
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on:
Blocks: GenerationalGC
  Show dependency treegraph
 
Reported: 2012-08-14 07:23 PDT by Brian Hackett (:bhackett)
Modified: 2013-06-25 13:50 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (6627eca98f7e) (261.85 KB, patch)
2012-08-16 05:40 PDT, Brian Hackett (:bhackett)
terrence: review+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2012-08-14 07:23:39 PDT
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 (:bhackett) 2012-08-16 05:40:22 PDT
Created attachment 652408 [details] [diff] [review]
patch (6627eca98f7e)
Comment 2 Terrence Cole [:terrence] 2012-08-16 12:14:35 PDT
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 (:bhackett) 2012-08-21 12:20:20 PDT
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] 2012-08-21 13:53:41 PDT
(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] 2012-08-21 19:08:46 PDT
https://hg.mozilla.org/mozilla-central/rev/bf1a005f1e61
Comment 6 :Ms2ger 2012-08-22 01:32:28 PDT
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 (:bhackett) 2012-08-22 05:09:50 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.