1142794 - Change every SetProperty signature for hopefully the last time

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Receivers are values, so we have to change all the SetProperty functions/methods at least one more time.

While we're in the neighborhood:

*   Change the order of arguments everywhere to be the same, to wit,
    (cx, obj, id, v, receiver). The last three are in the same order
    as in the standard.

*   Make the value argument a non-mutable HandleValue. This eliminates a
    RootedValue in some places and adds one in others; on the whole, no
    significant change, so we might as well do the sane thing.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Change 'receiver' argument to SetProperty functions and ProxyHandler::set methods to be a HandleValue

Also: Change signature of these functions and methods to all have the same arguments in the same order: (cx, obj, id, v, receiver). Also change v from MutableHandleValue to HandleValue.

There is no change in behavior.

In fact the new error message `JSMSG_SET_NON_OBJECT_RECEIVER` is
impossible to trigger from scripts for now, I think (after re-reading
the whole patch with this in mind). JS_ForwardSetPropertyTo is the only
way to get a non-object receiver into the engine, but no caller
currently does so.

We're installing new pipes here, and they should work, but for now it's
the same cold water flowing through as before. Actually hooking up the
hot water is left for another bug (one with tests, not to put too fine a
point on it).

Notes:

*   InvokeGetterOrSetter had to be split into two functions:
    InvokeGetter takes a MutableHandleValue out-param,
    InvokeSetter a HandleValue in-param.

*   Watchpoints can still tamper with values being assigned. So can
    JSSetterOps. I'm pleased we can support this craziness in a way that
    doesn't have to spread via the type system to encompass the entire
    codebase.

*   Change in GlobalObject::setIntrinsicValue is not really a change.
    Yes, it asserted before, but an exception thrown during self-hosting
    initialization is not going to go unnoticed either.

*   Since the receiver argument to js::SetProperty() is at the end now, it
    makes sense for it to be optional. Some callers look nicer.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8578508 [details] [diff] [review]
Change 'receiver' argument to SetProperty functions and ProxyHandler::set methods to be a HandleValue

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

::: js/src/jsapi.cpp
@@ +2874,5 @@
>  JS_PUBLIC_API(bool)
>  JS_SetElement(JSContext *cx, HandleObject obj, uint32_t index, HandleValue v)
>  {
>      RootedValue value(cx, v);
> +    return SetElement(cx, obj, index, value);

You can't eliminate |value| and just pass |v|?

::: js/src/jsobj.h
@@ +863,2 @@
>   *
>   * When obj != receiver, it's a reasonable guess that a proxy is involved, obj

s/When/If you're passing/ perhaps.

::: js/src/proxy/ScriptedIndirectProxyHandler.cpp
@@ +340,5 @@
>          return result.fail(JSMSG_GETTER_ONLY);
>  
> +    if (!receiver.isObject())
> +        return result.fail(JSMSG_SET_NON_OBJECT_RECEIVER);
> +    RootedObject receiverObj(cx, &receiver.toObject());

A cromulent hack.  Any reason not to move the Rooted below the !op exit, tho?

::: js/src/vm/NativeObject.cpp
@@ +2068,5 @@
>   *
>   * FIXME: This should be updated to follow ES6 draft rev 28, section 9.1.9,
>   * steps 4.d.i and 5.
>   *
>   * Note that receiver is not necessarily native.

I'd probably remove this now.

::: js/src/vm/ScopeObject.cpp
@@ -1602,5 @@
>          if (debugScope->isOptimizedOut())
>              return Throw(cx, id, JSMSG_DEBUG_CANT_SET_OPT_ENV);
>  
>          AccessResult access;
> -        if (!handleUnaliasedAccess(cx, debugScope, scope, id, SET, vp, &access))

Ugh, this prior possibility of |vp| being mutated in this method was confusingmaking.

@@ +1608,1 @@
>              return false;

It's extraordinarily frustrating to have all accesses -- sets and gets both -- go through a single method.  :-(  It's thus impossible to tell from the signature what'll happen, and what will be affected by the call.

We've been in this situation before -- I *remember* having one massive, messy function for this sort of thing before, and manually disentangling it into something far more readable.  It was a huge win for readability.  And now, we've regressed to exactly the same situation again.  :-(

Could you file a followup to break this method into handleUnaliasedGet and handleUnaliasedSet, have each of those methods delegate to get/set methods existing on CallObject and on ClonedBlockObject, and then pair up those classes' get/set methods at the source level to aid in comparisons of the two, with whatever prefatory code-sharing is possible in such system?

Once this happens, there will be no need to copy |v| here.

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> >      RootedValue value(cx, v);
> > +    return SetElement(cx, obj, index, value);
> 
> You can't eliminate |value| and just pass |v|?

Er, yes. Done.

> >   * When obj != receiver, it's a reasonable guess that a proxy is involved, obj
> 
> s/When/If you're passing/ perhaps.

Changed to "Callers pass obj != receiver e.g. when a proxy is involved, ..."

> ::: js/src/proxy/ScriptedIndirectProxyHandler.cpp
> > +    if (!receiver.isObject())
> > +        return result.fail(JSMSG_SET_NON_OBJECT_RECEIVER);
> > +    RootedObject receiverObj(cx, &receiver.toObject());
> 
> A cromulent hack.  Any reason not to move the Rooted below the !op exit, tho?

Nope. Moved.

> ::: js/src/vm/NativeObject.cpp
> >   * Note that receiver is not necessarily native.
> 
> I'd probably remove this now.

Removed.

> ::: js/src/vm/ScopeObject.cpp
> Could you file a followup to break this method into handleUnaliasedGet and
> handleUnaliasedSet [...]

Filed bug 1145430.

Comment 4

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=cf30f1352353

Comment 5

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7613fc978d36

Comment 6

[Phil Ringnalda (:philor)]

Backed out that whole push in https://hg.mozilla.org/integration/mozilla-inbound/rev/c3638d994edd for massive widespread bustage, https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7613fc978d36&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception - something must have changed out from under you.

Comment 7

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=823f77a7a8da

Comment 8

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=8fb7bc1aeee6

Comment 9

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=6cb998c1fb4d

Comment 10

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b47d5f7adce

Comment 11

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0466aa0693c5

Comment 12

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c02abfe5360

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c02abfe5360