Replace js::Invoke(cx, args) and js::Invoke(cx, thisv, fval, argc, argv, rval) with js::Call(cx, fval, thisv, args, rval)

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(18 attachments, 6 obsolete attachments)

3.60 KB, patch
till
: review+
Details | Diff | Splinter Review
2.55 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.05 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
8.94 KB, patch
efaust
: review+
Details | Diff | Splinter Review
2.19 KB, patch
mrrrgn
: review+
Details | Diff | Splinter Review
10.85 KB, patch
efaust
: review+
Details | Diff | Splinter Review
6.40 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.38 KB, patch
sfink
: review+
Details | Diff | Splinter Review
3.35 KB, patch
arai
: review+
Details | Diff | Splinter Review
17.12 KB, patch
till
: review+
Details | Diff | Splinter Review
11.99 KB, patch
shu
: review+
Details | Diff | Splinter Review
15.25 KB, patch
till
: review+
Details | Diff | Splinter Review
3.29 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.79 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
12.07 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
11.24 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
13.26 KB, patch
efaust
: review+
Details | Diff | Splinter Review
10.40 KB, patch
efaust
: review+
Details | Diff | Splinter Review
Assignee

Description

3 years ago
The current invocation APIs make it super-trivial to forget to setThis(), resulting in weird behaviors like |this === +0|.  It'd be better to have an API more akin to js::Construct and to the spec.  Namely, you create an InvokeArgs, you fill in the arguments, then you do js::Call(cx, fval, thisv, args, rval).  This forces the user to provide a thisv, so they can't mess up providing it.  It's also closer to the spec API, enhancing readability.

Long patch series making this change to follow.  Once these are all landed, there might be one or two other followup modifications that could be made, but I'd like to get this in with the dust settled before doing things like 1) make FixedInvokeArgs initing bounds-check at compile time; 2) make InvokeArgs/ConstructArgs not inherit from CallArgs but rather from some base of it, that CallArgs and those two share; 3) rename JS::CallArgs to JS::NativeArgs so that InvokeArgs can more sensibly be CallArgs; 4) modify FixedInvokeArgs so that its things like length() and argv() and base() are as close to compile-time computations as possible, amenable to inlining; 5) and maybe one or two other things, once those are done.

But one step at a time.  All these changes first, any of that long tail of ideas second.
Assignee

Comment 1

3 years ago
Just a preparatory cleanup patch, mostly not related to this exact bug.
Attachment #8735014 - Flags: review?(till)
Assignee

Comment 2

3 years ago
Attachment #8735015 - Flags: review?(jorendorff)
Assignee

Comment 3

3 years ago
I'm not wholly convinced these functions are useful, or that a recursion-check doesn't belong in, say, js::Call.  But let's forget that for now.
Attachment #8735016 - Flags: review?(efaustbmo)
Assignee

Comment 4

3 years ago
I'm making this change because otherwise, DirectEval needs to be able to access args.rval(), and I want to kill off direct access to that in invocation code -- but I can't right now because of its being on JS::CallArgs and unavoidably exposed there.  Also, DirectEval wants to see args.calleev(), which my changes at one point, incorrectly, neglected to set (because an args.setThis() that affected both Invoke() and DirectEval() paths was moved into js::Call() paths, leaving DirectEval un-setThis()'d anywhere).  By making this v/vp-based instead of args-based, the status of args.calleev() becomes no longer relevant.
Attachment #8735018 - Flags: review?(efaustbmo)
Assignee

Comment 5

3 years ago
Later patches here remove InternalInvoke -- this just marks them for termination, to get trickier cases out of my way in subsequent acking.
Attachment #8735019 - Flags: review?(efaustbmo)
Assignee

Comment 7

3 years ago
Briefly, let's have dueling js::Invoke and js::Call, so that conversions from one to the other don't have to be all-at-once during reviewing/landing.
Attachment #8735021 - Flags: review?(efaustbmo)
Comment on attachment 8735014 [details] [diff] [review]
Intl.cpp:GetInternals should directly return its result, not go via outparam

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

Very yes.
Attachment #8735014 - Flags: review?(till) → review+
Assignee

Comment 9

3 years ago
See attachment 8735021 [details] [diff] [review] for the js::Call API.  (Hopefully it behaves the way you'd expect it to behave.)  Note also the recent introduction of FixedInvokeArgs<N> to substitute for InvokeArgs::init(constant number).
Attachment #8735022 - Flags: review?(jimb)
Assignee

Comment 10

3 years ago
See attachment 8735021 [details] [diff] [review] for the new API, if its meaning wasn't intuitively obvious.
Attachment #8735023 - Flags: review?(jdemooij)
Assignee

Comment 11

3 years ago
I promise I'll let your patch to this method land before mine.  :-)  But feel free to hold off reviewing this until then, if you have particular worries that that patch would significantly harm this one in terms of applicability.  I'm guess, without a whole lot of justification, that it doesn't.
Attachment #8735024 - Flags: review?(jdemooij)
Assignee

Comment 12

3 years ago
Spreadin' the review lovin' and API awareness'n around.
Attachment #8735025 - Flags: review?(sphink)
Assignee

Comment 13

3 years ago
If we checked |saveLoc| first, we could have two different callback approaches, one for FixedInvokeArgs<N> and one for FixedInvokeArgs<N + 1>.  It doesn't seem worth the effort to me.  I could do it in a followup if truly desired.
Attachment #8735026 - Flags: review?(arai.unmht)
Assignee

Comment 15

3 years ago
Note that js::Call fully consumes callee/thisv before setting rval on success.  So it's perfectly permissible to pass the same rooted Value as callee/this and as rval.  I do this many places in these patches, but I might have missed a couple places I *could* have done it.  I'm happy to fix such places if a complaint is made.  But an extra root/unroot is not the end of the world if it happens for now.
Attachment #8735028 - Flags: review?(till)
Assignee

Comment 16

3 years ago
Attachment #8735029 - Flags: review?(efaustbmo)
Comment on attachment 8735028 [details] [diff] [review]
Update miscellaneous code to use js::Call instead of js::Invoke

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

This is a funny mix of more and less verbose. One thing that would make it slightly nicer is a js::Call overload that takes a HandleObject for `this`. r=me without that, obviously.
Attachment #8735028 - Flags: review?(till) → review+
Assignee

Comment 18

3 years ago
I'm not happy about that InternalInvokeOrConstruct still there.  Nor about the invoke() interface duplicating the second argument to ForOfIterator's constructor.  But as you noted, this stuff probably should die from self-hosting anyway, so probably we should get away without cleaning it up that much.
Attachment #8735030 - Flags: review?(shu)
Assignee

Comment 19

3 years ago
This CallSelfHostedFunction stuff is a little goofy, but eh for now.
Attachment #8735032 - Flags: review?(till)
Comment on attachment 8735026 [details] [diff] [review]
Update Reflect.parse callback code to work with InvokeArgs and js::Call

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

nice :)
Attachment #8735026 - Flags: review?(arai.unmht) → review+
Comment on attachment 8735032 [details] [diff] [review]
Adjust Promise code to use Call instead of Invoke

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

These calls gained an unfortunate amount of additional boilerplate, but oh well.

::: js/src/jsapi.cpp
@@ +4866,5 @@
> +        FixedInvokeArgs<1> args(cx);
> +
> +        args[0].setObject(*arr);
> +
> +        RootedValue thisvOrRval(cx, UndefinedValue());

Can't you use UndefinedHandleValue as thisv here and get rid of the unfortunate name in one more place?
Attachment #8735032 - Flags: review?(till) → review+
Comment on attachment 8735020 [details] [diff] [review]
Remove jsarray.cpp's now-unused SortComparatorFunction

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

MRW I review a patch that removes dead code. https://i.imgur.com/6ADLV4r.gif :)
Attachment #8735020 - Flags: review?(winter2718) → review+
Assignee

Updated

3 years ago
Attachment #8735024 - Flags: review?(jdemooij)
Assignee

Comment 23

3 years ago
Comment on attachment 8735018 [details] [diff] [review]
Change js::DirectEval to take v/vp rather than a CallArgs to operate on directly

Bug 1256298 is going to slightly bitrot this, and the approach that bug takes suggests maybe a different approach to take in this area.  So canceling for now.
Attachment #8735018 - Flags: review?(efaustbmo)
Comment on attachment 8735023 [details] [diff] [review]
Update various miscellaneous function-calling code to js::Call

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

::: js/src/jit/VMFunctions.cpp
@@ +822,5 @@
>      args[1].set(val);
>      args[2].setString(kind);
>  
> +    rval.setUndefined(); // function ignores |this|
> +    return Call(cx, selfHostedFun, rval, args, rval);

Pass UndefinedHandleValue for thisv, instead of using rval?

::: js/src/vm/ForOfIterator.cpp
@@ +135,4 @@
>          return false;
>  
>      InvokeArgs args(cx_);
>      if (!args.init(0))

FixedInvokeArgs<0> while you're here?

@@ +168,5 @@
>      if (!GlobalObject::getSelfHostedFunction(cx_, cx_->global(), name, name, 1, &val))
>          return false;
>  
>      InvokeArgs args(cx_);
>      if (!args.init(1))

FixedInvokeArgs<1>
Attachment #8735023 - Flags: review?(jdemooij) → review+
Assignee

Updated

3 years ago
Attachment #8735018 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8735024 - Attachment is obsolete: true
Attachment #8735015 - Flags: review?(jorendorff) → review+
Comment on attachment 8735016 [details] [diff] [review]
Rename Invoke[GS]etter to Call[GS]etter, more in line with the spec's calling nomenclature

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

*steal*
Attachment #8735016 - Flags: review?(efaustbmo) → review+
Comment on attachment 8735027 [details] [diff] [review]
Update ScriptedDirectProxyHandler code to use js::Call and FixedInvokeArgs

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

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +238,5 @@
>      bool booleanTrapResult;
>      {
> +        FixedInvokeArgs<1> args(cx);
> +
> +        args[0].setObject(*target);

Why all these spaces? At least the one above is seems unnecessary to me.

@@ +241,5 @@
> +
> +        args[0].setObject(*target);
> +
> +        RootedValue thisv(cx, ObjectValue(*handler));
> +        if (!Call(cx, trap, thisv, args, &thisv))

This is not very nice. Can't we have a Call overwrite with HandleObject for thisv? I would rather keep trapResult as a name.
Attachment #8735027 - Flags: review?(evilpies) → review-
Attachment #8735189 - Flags: review?(jdemooij) → review+
Comment on attachment 8735022 [details] [diff] [review]
Update Debugger code to use js::Call rather than Invoke

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

I think this patch shows an issue with the goal here: we're removing a convenience function that wasn't hurting anyone, and replacing it with... boilerplate that has to be added at every call site.

Let's find a better way through here. We can bash out a better design in an etherpad, I think.

::: js/src/vm/Debugger.cpp
@@ +692,5 @@
> +
> +                args[0].set(completion);
> +
> +                RootedValue rval(cx, ObjectValue(*frameobj));
> +                bool hookOk = js::Call(cx, handler, rval, args, &rval);

For example, here the code shouldn't be any more complicated than

    RootedValue rval(cx);
    bool hookOk = Call(cx, handler, frameobj, Arguments{completion}, &rval);

I'm not worried about the compiled code; it's certainly OK to have the convenience function doing exactly the same work you're doing here. It's about readability -- the debugger is hairy enough without piling on boilerplate.
Attachment #8735022 - Flags: review?(jimb) → review-
(In reply to Tom Schuster [:evilpie] from comment #28)
> This is not very nice. Can't we have a Call overwrite with HandleObject for
> thisv?

I was thinking the same in the Debugger.cpp review.

(In general, JSAPI could benefit from some kind of glue that allowed APIs to accept either HandleObject or HandleValue - probably beyond the scope of this bug, though.)
Assignee

Updated

3 years ago
Keywords: leave-open
Assignee

Comment 32

3 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #30)
> (In reply to Tom Schuster [:evilpie] from comment #28)
> > This is not very nice. Can't we have a Call overwrite with HandleObject for
> > thisv?
> 
> I was thinking the same in the Debugger.cpp review.

Turns out HandleObject is out here.  Or at least it's out as far as Debugger's concerned.  A bunch of the Debugger cases have on hand Debugger::object as the |this| to pass.  This isn't a HandleObject, it's a HeapPtr<NativeObject*> that would have to additionally be shoved into a RootedObject to get a handle to it.  So I made it JSObject*, sadfaces.  That seems to work at least as far as Debugger's concerned.  Still propagating such adjustments down the patch-stack.
Comment on attachment 8735025 [details] [diff] [review]
Update function-calling JSAPI methods to use js::Call

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

r=me after IRC discussion. I don't care about the boilerplate, because what Invoke was doing was Bad Magic.
Attachment #8735025 - Flags: review?(sphink) → review+
Comment on attachment 8735030 [details] [diff] [review]
Adjust FixedInvokeGuard to not depend on CallArgs::set{Callee,This}, and remove js::Invoke

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

::: js/src/builtin/MapObject.cpp
@@ +554,5 @@
>                  args2[0].set(key);
>                  args2[1].set(val);
>  
> +                // |pairVal| is "dead" now and can be used as a spare root.
> +                if (!fig.invoke(cx, adderVal, mapVal, &pairVal))

I feel like this a refactor hazard. Can't be that expensive to make a RootedValue dummy(cx).

@@ +1197,4 @@
>                  args2[0].set(keyVal);
>  
> +                // |keyVal| is "dead" now and can be used as a spare root.
> +                if (!fig.invoke(cx, adderVal, setVal, &keyVal))

Use dummy.

::: js/src/builtin/WeakMapObject.cpp
@@ +371,5 @@
>                  args2[0].set(keyVal);
>                  args2[1].set(val);
>  
> +                // |pairVal| is "dead" now and can be used as a spare root.
> +                if (!fig.invoke(cx, adderVal, mapVal, &pairVal))

Use dummy.

::: js/src/builtin/WeakSetObject.cpp
@@ +142,4 @@
>                  args2[0].set(keyVal);
>  
> +                // |keyVal| is "dead" now and can be used as a spare root.
> +                if (!fig.invoke(cx, adderVal, setVal, &keyVal))

Use dummy.
Attachment #8735030 - Flags: review?(shu) → review+
Comment on attachment 8735030 [details] [diff] [review]
Adjust FixedInvokeGuard to not depend on CallArgs::set{Callee,This}, and remove js::Invoke

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

Perhaps also rename FastInvokeGuard to FastCallGuard and .invoke to .call.
Assignee

Updated

3 years ago
Attachment #8735022 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8735027 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8735029 - Attachment is obsolete: true
Attachment #8735029 - Flags: review?(efaustbmo)
Comment on attachment 8738234 [details] [diff] [review]
Update ScriptedDirectProxyHandler code to use js::Call and FixedInvokeArgs

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

I would have been ok with using FixedInvokeArgs for one argument as well, but this is fine.

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +237,5 @@
>      // Step 7.
>      bool booleanTrapResult;
>      {
> +        RootedValue arg(cx, ObjectValue(*target));
> +        RootedValue v(cx);

trapResult

@@ +293,5 @@
>      // Step 7.
> +    bool booleanTrapResult;
> +    {
> +        RootedValue arg(cx, ObjectValue(*target));
> +        RootedValue v(cx);

trapResult

@@ +748,5 @@
>          if (!IdToStringOrSymbol(cx, id, &value))
>              return false;
>  
> +        RootedValue targetVal(cx, ObjectValue(*target));
> +        RootedValue v(cx);

trapResult
Attachment #8738234 - Flags: review?(evilpies) → review+
Attachment #8738231 - Flags: review?(jorendorff) → review+
Comment on attachment 8738232 [details] [diff] [review]
Update Debugger code to use js::Call rather than Invoke

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

Beautiful.
Attachment #8738232 - Flags: review?(jorendorff) → review+
Assignee

Updated

3 years ago
Attachment #8735188 - Attachment is obsolete: true
Attachment #8735188 - Flags: review?(efaustbmo)
Comment on attachment 8735019 [details] [diff] [review]
Eliminate Invoke(JSContext*, const CallArgs&, MaybeConstruct = NO_CONSTRUCT) by 1) renaming it to a more-internal name, 2) adding an Invoke overload for existing InvokeArgs providers only, and 3) adding an InternalInvoke function to temporarily mark non-I

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

"Should not use internal cases" seems to be totally arbitrary, but sure, if it'll clean up by the end, it'll clean up.
Attachment #8735019 - Flags: review?(efaustbmo) → review+
Comment on attachment 8735021 [details] [diff] [review]
Add CallFromStack for certain internal users, js::Call for general use, and mark js::Invoke as deprecated

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

OK
Attachment #8735021 - Flags: review?(efaustbmo) → review+
Comment on attachment 8738235 [details] [diff] [review]
Update various builtins to use js::Call, not js::Invoke

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

This is much cleaner. Thanks!
Attachment #8738235 - Flags: review?(efaustbmo) → review+
Comment on attachment 8739275 [details] [diff] [review]
Change js::DirectEval to take v/vp rather than a CallArgs to operate on directly

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

Works for me.

::: js/src/builtin/Eval.cpp
@@ +413,5 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>      Rooted<GlobalObject*> global(cx, &args.callee().global());
>      RootedObject globalLexical(cx, &global->lexicalScope());
> +    return EvalKernel(cx, args.get(0), INDIRECT_EVAL, NullFramePtr(), globalLexical, nullptr,

Can we add a comment here that's like "EvalKernel will return the undefined passed by args.get(0) if args.length < 1" or something to that effect?

Perhaps this is actually better done inside EvalKernel, since there are many callers.

::: js/src/vm/Interpreter.cpp
@@ +2728,5 @@
>      } else {
>          if (!Invoke(cx, args))
>              goto error;
>      }
> +    REGS.sp -= 1 + argc;

So this is actually += -(argc + 2) + 1? It's callee, this, and args, but we leave a value pushed. Can we comment to that effect?
Attachment #8739275 - Flags: review?(efaustbmo) → review+
Assignee

Updated

3 years ago
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.