Make JSObject::doSomethingToThisObject methods static

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Other Branch
mozilla17
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 652408 [details] [diff] [review]
patch (6627eca98f7e)
Assignee: general → bhackett1024
Attachment #652408 - Flags: review?(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.
Attachment #652408 - Flags: review?(terrence) → review+
(Assignee)

Comment 3

5 years ago
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
(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.
https://hg.mozilla.org/mozilla-central/rev/bf1a005f1e61
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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?
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 785452
No longer depends on: 785452
You need to log in before you can comment on or make changes to this bug.