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.

Just a preparatory cleanup patch, mostly not related to this exact bug.

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.

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.

Later patches here remove InternalInvoke -- this just marks them for termination, to get trickier cases out of my way in subsequent acking.

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.

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.

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

See attachment 8735021 [details] [diff] [review] for the new API, if its meaning wasn't intuitively obvious.

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.

Spreadin' the review lovin' and API awareness'n around.

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.

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.

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.

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.

This CallSelfHostedFunction stuff is a little goofy, but eh for now.

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 :)

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?

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 :)

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.

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>

Updated for bug 1256298.

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*

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.

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.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/de7e03fdff65 https://hg.mozilla.org/integration/mozilla-inbound/rev/61f736c678a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a930fcdf285 https://hg.mozilla.org/integration/mozilla-inbound/rev/52cf294fc0d5

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

https://hg.mozilla.org/mozilla-central/rev/de7e03fdff65 https://hg.mozilla.org/mozilla-central/rev/61f736c678a3 https://hg.mozilla.org/mozilla-central/rev/6a930fcdf285 https://hg.mozilla.org/mozilla-central/rev/52cf294fc0d5

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.

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.

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.

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

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.

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.

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

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!

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?

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0d5faf75aa5 https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d51ed412a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/b538123cb69f https://hg.mozilla.org/integration/mozilla-inbound/rev/b9cef6b587ac https://hg.mozilla.org/integration/mozilla-inbound/rev/7a299a590950 https://hg.mozilla.org/integration/mozilla-inbound/rev/a6fa04f4dee9 https://hg.mozilla.org/integration/mozilla-inbound/rev/9cf2b9587eb8 https://hg.mozilla.org/integration/mozilla-inbound/rev/8cf68d45fea8 https://hg.mozilla.org/integration/mozilla-inbound/rev/ceeb25518b0c https://hg.mozilla.org/integration/mozilla-inbound/rev/69079481ba3a https://hg.mozilla.org/integration/mozilla-inbound/rev/4012e45e502e https://hg.mozilla.org/integration/mozilla-inbound/rev/4099b331a79a https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ca1d46b22a https://hg.mozilla.org/integration/mozilla-inbound/rev/69d4569f7601

https://hg.mozilla.org/mozilla-central/rev/f0d5faf75aa5 https://hg.mozilla.org/mozilla-central/rev/b9d51ed412a7 https://hg.mozilla.org/mozilla-central/rev/b538123cb69f https://hg.mozilla.org/mozilla-central/rev/b9cef6b587ac https://hg.mozilla.org/mozilla-central/rev/7a299a590950 https://hg.mozilla.org/mozilla-central/rev/a6fa04f4dee9 https://hg.mozilla.org/mozilla-central/rev/9cf2b9587eb8 https://hg.mozilla.org/mozilla-central/rev/8cf68d45fea8 https://hg.mozilla.org/mozilla-central/rev/ceeb25518b0c https://hg.mozilla.org/mozilla-central/rev/69079481ba3a https://hg.mozilla.org/mozilla-central/rev/4012e45e502e https://hg.mozilla.org/mozilla-central/rev/4099b331a79a https://hg.mozilla.org/mozilla-central/rev/f5ca1d46b22a https://hg.mozilla.org/mozilla-central/rev/69d4569f7601