Closed Bug 852602 Opened 7 years ago Closed 7 years ago

Pass CallArgs to Proxy call() and construct() hooks

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(2 files)

Attached patch Patch v1Splinter Review
No description provided.
Attachment #726777 - Flags: review?(jwalden+bmo)
Comment on attachment 726777 [details] [diff] [review]
Patch v1

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

This doubtless broke horribly in the Great Handle-ification Of All Things Proxy And Wrapper.  The bustage is probably extensive but not too difficult to work through.  As always, if you judge that a re-review is needed for an updated version, feel free to request it.

::: js/public/CallArgs.h
@@ +241,5 @@
>          return argv_[i];
>      }
>  
>      /* Returns a mutable handle for the i-th zero-indexed argument. */
> +    MutableHandleValue handleAt(unsigned i) const {

Hmm, this was a MutableHandleValue?  Maybe, I guess.  I think it'd be better for arguments not to be implicit mutable roots, and have people use Rooted rather than rely on hairy rooting that only can work if enough arguments were provided, but I guess that's for another bug.

::: js/src/jswrapper.cpp
@@ +496,3 @@
>              if (!cx->compartment->wrap(cx, &arg))
>                  return false;
> +            args[n] = arg;

If we're going to say the args are mutable, and actually use that, right now, just pass &arg[n] and get rid of the RootedValue and reassignment.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1220,2 @@
>              return false;
> +        args.setThis(thisv);

Ugh.  setThis(), per comments, is dodgy and mostly shouldn't be used.  So this will have to be a temporary hackaround, that'll get fixed up later to something more desirable, that likely doesn't exist just yet.

@@ +1220,3 @@
>              return false;
> +        args.setThis(thisv);
> +        for (size_t n = 0; n < args.length(); ++n) {

Dunno why s/i/n/, but whatever.
Attachment #726777 - Flags: review?(jwalden+bmo) → review+
Attached patch Patch v2Splinter Review
It wasn't just evilpie who bitrotted me, but bholley did too, in a more annoying way.
Attachment #728642 - Flags: review?(jwalden+bmo)
Comment on attachment 728642 [details] [diff] [review]
Patch v2

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

Possibly I should look at the computeThis change again.

::: js/public/CallArgs.h
@@ +141,5 @@
>          // MOZ_ASSERT(!argv_[-1].isMagic(JS_IS_CONSTRUCTING));
>          return HandleValue::fromMarkedLocation(&argv_[-1]);
>      }
>  
> +    Value computeThis(JSContext *cx) const {

jorendorff and I discussed computeThis behavior wrt CallArgs recently, and we concluded that we shouldn't expose this in CallArgs -- callers should handle the null/undefined case explicitly.  (This really only matters to methods that might be called on the global object, without qualification.  That's basically only methods on Window, which bz says can be code-genned accordingly, and some of the oddballs on Object.prototype, like __{define,lookup}{G,S}etter__.  And moderately-generic cases like this one.)

Let's go with this for now, to keep moving forward.  But I'd like a followup to remove this method, and replace the one use of it here with a JS::ToObject method (that acts like the spec -- note this is different from JS_ValueToObject; yay non-spec names and non-spec algorithms).

::: js/src/jsproxy.cpp
@@ +2116,3 @@
>      };
>      RootedValue thisValue(cx, ObjectValue(*handler));
> +    return Invoke(cx, thisValue, trap, 3, argv, args.rval().address());

Make that ArrayLength(argv) while you're here.

@@ +2153,3 @@
>      };
>      RootedValue thisValue(cx, ObjectValue(*handler));
> +    return Invoke(cx, thisValue, trap, 2, constructArgv, args.rval().address());

ArrayLength(constructArgv)

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1113,5 @@
>      // Run the resolve hook of the wrapped native.
>      XPCWrappedNative *wn = getWN(wrapper);
>      if (NATIVE_HAS_FLAG(wn, WantCall)) {
> +        XPCCallContext ccx(JS_CALLER, cx, wrapper, nullptr, JSID_VOID, args.length(), args.array(),
> +                           args.rval().address());

Hmm.  XPCCallContext may want to take const CallArgs& too.  Followup-land.

@@ +1118,5 @@
>          if (!ccx.IsValid())
>              return false;
>          bool ok = true;
> +        nsresult rv = wn->GetScriptableInfo()->GetCallback()->Call(
> +            wn, cx, wrapper, args.length(), args.array(), args.rval().address(), &ok);

And this Call hook as well.
Attachment #728642 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Comment on attachment 728642 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 728642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Possibly I should look at the computeThis change again.
> 
> ::: js/public/CallArgs.h
> @@ +141,5 @@
> >          // MOZ_ASSERT(!argv_[-1].isMagic(JS_IS_CONSTRUCTING));
> >          return HandleValue::fromMarkedLocation(&argv_[-1]);
> >      }
> >  
> > +    Value computeThis(JSContext *cx) const {
> 
> jorendorff and I discussed computeThis behavior wrt CallArgs recently, and
> we concluded that we shouldn't expose this in CallArgs -- callers should
> handle the null/undefined case explicitly.  (This really only matters to
> methods that might be called on the global object, without qualification. 
> That's basically only methods on Window, which bz says can be code-genned
> accordingly, and some of the oddballs on Object.prototype, like
> __{define,lookup}{G,S}etter__.  And moderately-generic cases like this one.)
> 
> Let's go with this for now, to keep moving forward.  But I'd like a followup
> to remove this method, and replace the one use of it here with a
> JS::ToObject method (that acts like the spec -- note this is different from
> JS_ValueToObject; yay non-spec names and non-spec algorithms).

Will do.

> ::: js/xpconnect/wrappers/XrayWrapper.cpp
> @@ +1113,5 @@
> >      // Run the resolve hook of the wrapped native.
> >      XPCWrappedNative *wn = getWN(wrapper);
> >      if (NATIVE_HAS_FLAG(wn, WantCall)) {
> > +        XPCCallContext ccx(JS_CALLER, cx, wrapper, nullptr, JSID_VOID, args.length(), args.array(),
> > +                           args.rval().address());
> 
> Hmm.  XPCCallContext may want to take const CallArgs& too.  Followup-land.

Got a patch for that.

> @@ +1118,5 @@
> >          if (!ccx.IsValid())
> >              return false;
> >          bool ok = true;
> > +        nsresult rv = wn->GetScriptableInfo()->GetCallback()->Call(
> > +            wn, cx, wrapper, args.length(), args.array(), args.rval().address(), &ok);
> 
> And this Call hook as well.

But I hope we can remove all these hooks in a couple of months, so I'm going to let it die as ugly as it is.
https://hg.mozilla.org/mozilla-central/rev/04987f63066f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Filed bug 859445 about comment 3.
You need to log in before you can comment on or make changes to this bug.