776579 - Use HandleValue and MutableHandleValue in object hooks where possible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Doing this will be less error prone, as hooks will be free to perform effectful operations after setting their output parameters.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch (fe623d60bea1)

Comment 2

[David Mandelin [:dmandelin]]

Waiting for Bill's review before sr'ing fully. At first glance, the jsapi.h and jsclass.h changes look reasonable. I like the fact that IIUC it really means strictly less work and less to understand for implementers of those callbacks. Also actually kind of like replacing 'jsval *', which is rather ambiguous about the semantics (is it an array?) with JSMutableHandleValue. I still wonder how many different kinds of things we'll have in play at the end, but this does seem like a step in the right direction, and not too risky. (I'll think about it more carefully when I see what Bill noticed that I didn't. ;-) )

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 645293 [details] [diff] [review]
patch (fe623d60bea1)

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

Thanks for doing this.

In the future, I think we need to consider how to deal with arrays of rooted things. It seems like that's where most of the calls to fromMarkedLocation come from. It would be great if we could eliminate those.

::: js/src/jsanalyze.cpp
@@ +20,5 @@
>  /////////////////////////////////////////////////////////////////////
>  
>  #ifdef DEBUG
>  void
> +PrintBytecode(JSContext *cx, JSScript *script_, jsbytecode *pc)

Luke wants vars like this to be called scriptArg now.

::: js/src/jsapi.cpp
@@ +6157,5 @@
> +    return true;
> +}
> +
> +JS_PUBLIC_API(JSBool)
> +JS_ParseJSONWithReviver(JSContext *cx, const jschar *chars, uint32_t len, jsval reviver_, jsval *vp)

reviverArg

::: js/src/jsarray.cpp
@@ +412,5 @@
>  static bool
>  GetElementsSlow(JSContext *cx, HandleObject aobj, uint32_t length, Value *vp)
>  {
>      for (uint32_t i = 0; i < length; i++) {
> +        if (!aobj->getElement(cx, i, MutableHandleValue::fromMarkedLocation(&vp[i])))

Could you maybe comment this function and GetElements, saying that vp needs to be rooted somehow?

@@ +1807,5 @@
>  
>      const Value* end = vector + count;
>      while (vector < end && start <= MAX_ARRAY_INDEX) {
>          if (!JS_CHECK_OPERATION_LIMIT(cx) ||
> +            !SetArrayElement(cx, obj, start++, HandleValue::fromMarkedLocation(vector++))) {

Same here.

::: js/src/jsdate.cpp
@@ +2605,5 @@
>          return true;
>      }
>  
>      /* Step 4. */
> +    MutableHandleValue toISO = MutableHandleValue::fromMarkedLocation(&vp[0]);

It seems like we could just make a local var holding vp[0] and then copy it back.

::: js/src/jsdbgapi.cpp
@@ +800,5 @@
>          lastException = cx->getPendingException();
>      cx->clearPendingException();
>  
>      Rooted<jsid> id(cx, shape->propid());
> +    if (!baseops::GetProperty(cx, obj, id, MutableHandleValue::fromMarkedLocation(&pd->value))) {

I'd rather keep uses of fromMarkedLocation to a minimum. In cases like this, where performance doesn't matter, can't we root it and then copy it back?

@@ +1727,5 @@
>  
>  #endif /* __linux__ */
>  
>  JS_PUBLIC_API(void)
> +JS_DumpBytecode(JSContext *cx, JSScript *script_)

scriptArg

@@ +1744,5 @@
>  #endif
>  }
>  
>  extern JS_PUBLIC_API(void)
> +JS_DumpPCCounts(JSContext *cx, JSScript *script_)

scriptArg

::: js/src/jsnum.cpp
@@ +1159,5 @@
>          return NULL;
>  
>      /* ES5 15.1.1.1, 15.1.1.2 */
>      if (!DefineNativeProperty(cx, global, cx->runtime->atomState.NaNAtom,
> +                              HandleValue::fromMarkedLocation(&cx->runtime->NaNValue),

Could you just root this one instead? Performance doesn't seem to be an issue here.

@@ +1165,3 @@
>                                JSPROP_PERMANENT | JSPROP_READONLY, 0, 0) ||
>          !DefineNativeProperty(cx, global, cx->runtime->atomState.InfinityAtom,
> +                              HandleValue::fromMarkedLocation(&cx->runtime->positiveInfinityValue),

Same here.

::: js/xpconnect/src/XPCQuickStubs.h
@@ +627,5 @@
>      JS::RootedId id(cx);
>      if (!JS_ValueToId(cx, v, id.address()))
>          return false;
>      JS_SET_RVAL(cx, vp, argval);
> +    return ApplyPropertyOp<Op>(cx, *popp, obj, id, JSMutableHandleValue::fromMarkedLocation(vp));

Can you use CallArgs here?

Comment 4

[David Mandelin [:dmandelin]]

Comment on attachment 645293 [details] [diff] [review]
patch (fe623d60bea1)

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

Looks nice. The changes at usage sites seem very simple and mechanical, which seems like a good sign.

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/090fd1585e34

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/090fd1585e34