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 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.

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.

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?

(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.