969478 - Implement arguments/caller on functions through accessors on Function.prototype

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

An incremental step toward killing off fun_resolve, and making these properties not propertyops.

I'm aware ES6 specifies further semantics here, as far as having [[ThrowTypeError]] on builtins for these (and these semantics would probably be added to these accessors at that point), but I'd like to take one step at a time.

jorendorff for the patch, bholley to double-check the security/censoring aspects of this -- particularly as fun_resolve has fewer cases to deal with than these accessors do.

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

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Remove some poison-pill stuff, just handle arguments/caller individually

Perhaps a bit small to split out, but I had it so, so...

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8372415 [details] [diff] [review]
Patch

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

r=bholley on the caller censorship with the below.

::: js/src/jsfun.cpp
@@ +246,5 @@
> +
> +    // Censor the caller if we don't have full access to it.  If we do, but the
> +    // caller is a function with strict mode code, throw a TypeError per ES5.
> +    // If we pass these checks, we can return the computed caller.
> +    {

Rather than first checking for a wrapper, then checking for a security policy, and then doing an UncheckedUnwrap, let's just do one CheckedUnwrap. If that returns null, we do args.rval().setNull() and return. Otherwise, we proceed.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8372415 [details] [diff] [review]
Patch

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

Jeff, I'm sorry for not taking this stand earlier, but I don't think we should remove the own properties .arguments and .callee from sloppy functions. It's an observable change and it's contrary to what other implementations do. Why should we do this?

The only advantage I see is that as a user, you could then delete those two properties from Function.prototype and then you've killed off a capability that JS code really shouldn't have. But it is very hard to imagine any program ever actually doing that usefully. You can apply the getters cross-origin, right? So it's hard to actually stamp out that capability for good. (Note that `new Realm().global.Function.prototype` is coming soon.)

Making them accessor properties is also observable and contrary to other implementations, but having data properties silently change value is horrible, so I'm willing to try.

I like the other cleanups here and I applaud the long-term goal of killing off fun_resolve --- by doing the trick where function objects are created with all their own properties already existing, caching the needed Shape pointers somewhere to make that fast. That was the plan anyway, right?

Clearing review, with apologies. I can quickly stamp a version that turns these into accessor properties and all the rest, but leaves them on all functions.
 Sorry, too, for the GlobalObject slots that will require. :-P

::: js/src/jscntxt.cpp
@@ +141,5 @@
>          return clone;
>  
> +    MOZ_ASSERT(fun->isSelfHostedBuiltin(),
> +               "only self-hosted builtin functions may be cloned at call sites, and "
> +               "Function.prototype.caller relies on this");

Good assertion.

::: js/src/jsfun.cpp
@@ +110,5 @@
> +    for (; !iter.done(); ++iter) {
> +        if (!iter.isFunctionFrame() || iter.isEvalFrame())
> +            continue;
> +        if (iter.callee() == fun)
> +            return true;

Well that's kind of awkwardly worded. It's in the original though I guess.

@@ +171,5 @@
> +    // Disabling compiling of this script in IonMonkey.  IonMonkey doesn't
> +    // guarantee |f.arguments| can be fully recovered, so we try to mitigate
> +    // observing this behavior by detecting its use early.
> +    JSScript *script = iter.script();
> +    jit::ForbidCompilation(cx, script);

Also pre-existing, but consider moving this junk to ArgumentsObject::createUnexpected or something it calls.

@@ +254,5 @@
> +                args.rval().setNull();
> +                return true;
> +            }
> +
> +            callerFun = &UncheckedUnwrap(caller)->as<JSFunction>();

Not super crazy about using the object returned by Unwrap here. Doing a CheckedUnwrap as bholley suggests sounds fine... but isn't callerFun just going to be the same as maybeClone, which we already had? I'd like for the code to make it clear that the CheckedUnwrap is a security check, and nothing more.

@@ +267,5 @@
> +            return false;
> +        }
> +    }
> +
> +    args.rval().setObjectOrNull(caller);

We can .setObject(*caller) here, right?

::: js/src/tests/ecma_5/extensions/builtin-function-arguments-caller.js
@@ +78,5 @@
> +}
> +
> +// First arguments.
> +
> +assertThrowsError(makeCallWith(argumentsGetter, function(...r) {}), TypeError);

Also test what happens if the rest-function is actually on the stack when argumentsGetter is called on it.

@@ +83,5 @@
> +assertThrowsError(makeCallWith(argumentsGetter, strictFunction), TypeError);
> +
> +// Then caller.
> +
> +assertThrowsError(makeCallWith(callerGetter, strictFunction), TypeError);

() => callerGetter.call(strictFunction)

is actually shorter than what you've got here!

::: js/src/tests/ecma_5/extensions/function-caller-strict-cross-global.js
@@ +15,5 @@
> +}
> +catch (e)
> +{
> +  assertEq(e.constructor.name, "TypeError",
> +           "expected TypeError accessing strict .caller across globals, got " +

Wow, this really passes in the shell? I'm just surprised... I thought there was no security policy for cross-compartment wrappers between newGlobal() globals in the shell, so I would expect this to die with "failed to throw".

Comment 4

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> The only advantage I see is that as a user, you could then delete
> those two properties from Function.prototype and then you've killed
> off a capability that JS code really shouldn't have. But it is very
> hard to imagine any program ever actually doing that usefully.

Actually it's pretty easy.  The original Facebook app stuff (I think they use different things now) source-rewrote untrusted JS to permit Facebook app development.  Every computed property access had to pass the computed name through a function call, to disallow arguments/caller (and __parent__ and __proto__) accesses that might escape the app code and go into the "trusted" Facebook-provided intermediating machinery.  Had these properties been Function.prototype accessors (and with __parent__ gone and __proto__ accessorized), they could have just deleted them and not had to intercept any computed access.

> (Note that `new Realm().global.Function.prototype` is coming soon.)

They'll just have to replace Realm with a censoring wrapper (or delete it) as they do for Function/eval.  No biggie.

> I like the other cleanups here and I applaud the long-term goal of killing
> off fun_resolve --- by doing the trick where function objects are created
> with all their own properties already existing, caching the needed Shape
> pointers somewhere to make that fast. That was the plan anyway, right?

Eventually, yes.

> I can quickly stamp a version that turns these into accessor
> properties and all the rest, but leaves them on all functions.

It's not that simple, if the point here is also to reduce the size of the resolve hook.  (And, one hopes, not create umpteen different arguments/caller getter functions, one for each function where the property is accessed.)

The problem is there's no chokepoint for function creation, that can be changed to add builtin accessors from birth.  String/Error/RegExp do have such, but Function is deeply intertwined with object creation in general.  And when a function's created, we don't know that it's non-strict, because the script is associated with the object (often compiled, even) after the object's created (and its initial shape is assigned).  The rejiggering to fix this dearly needs its own bug.

Retry on the idea of making these accessors on Function.prototype?  The complexity just noted really shouldn't be dragged into this bug.

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> > (Note that `new Realm().global.Function.prototype` is coming soon.)
> 
> They'll just have to replace Realm with a censoring wrapper (or delete it)
> as they do for Function/eval.  No biggie.

I meant: new features are added from time to time. Each new way of accessing another global defeats all existing nutty security schemes that rely on deleting builtin capabilities.

More fundamentally, nobody is going to write Firefox-specific magic for this. Other implementations don't have delete-able caller/callee and aren't likely to get it.

> > I can quickly stamp a version that turns these into accessor
> > properties and all the rest, but leaves them on all functions.
> 
> It's not that simple, if the point here is also to reduce the size of the
> resolve hook.

There seems little point of reducing the size of the resolve hook if we already know how to get rid of it entirely.

Clearing review flag.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Remove all poison pills everywhere, do it all through Function.prototype.{arguments,caller} accessors

Following up on recent IRC discussion, an experiment:

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

The setters for both arguments/caller are a little screwy, but they have to be that way for assignments to arguments/caller on strict mode functions to throw even in non-strict mode code.

Comment 7

[Jeff Walden [:Waldo]]

There's one small behavioral difference here, that goes beyond simply a handful of functions not having own poison pills any more, and Function.prototype having accessors for arguments/caller now:

[jwalden@find-waldo-now src]$ dbg/js/src/js # with patch
js> function f() {}
js> "use strict"; f.caller = 5
5
js> 
[jwalden@find-waldo-now src]$ dbg/js/src/js # without patch
js> function f() {}
js> "use strict"; f.caller = 5
typein:2:14 TypeError: setting a property that has only a getter

Basically, setting caller/arguments on a non-strict function *used* to hit setting-a-property-with-only-a-getter throw-a-TypeError-in-strict-mode before.  But now, there *is* a setter, to handle the function-is-strict case.  Given that setter functions have no way of knowing that they're called from strict mode code (without bad things like sniffing pc, which we may have removed and which we have no interest in reintroducing), this seems unavoidable.  But it converts an error case, into a case where no error happens.  A change in that direction seems acceptable to me, in trade for the large overall simplification here.

Comment 8

[Jeff Walden [:Waldo]]

...and, of course, given that who's going to try setting arguments/caller on a non-strict function, in strict mode code, anyway.  :-)  Test suites only (...but not ours, or seemingly test262's, unless it's in a test we haven't imported yet), I strongly suspect.

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> [...] A change in that direction seems acceptable to me, in trade for
> the large overall simplification here.

Agreed.

The spec doesn't really anticipate this approach, but what you're implementing here seems clearly better than the spec --- Web compat permitting.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8469673 [details] [diff] [review]
Remove all poison pills everywhere, do it all through Function.prototype.{arguments,caller} accessors

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

Overall, thrilled with how this turned out.

I wonder if there are any tests at all for this stuff on arrow functions or asm.js functions. ...What happens if you try to walk the stack using .caller on asm.js code, from an FFI call back into JS, anyway? :-P

::: js/src/jsfun.cpp
@@ +113,5 @@
> +ArgumentsGetterImpl(JSContext *cx, CallArgs args)
> +{
> +    MOZ_ASSERT(IsFunction(args.thisv()));
> +
> +    // Beware: this function can be invoked on *any* function!  It can't assume

Blast-from-the-past nit: Prevailing style is to use French (or "non-French", as they say in France) spacing, with but a single space between sentences.

@@ +116,5 @@
> +
> +    // Beware: this function can be invoked on *any* function!  It can't assume
> +    // it'll never be invoked on natives, strict mode functions, bound
> +    // functions, or anything else that ordinarily has immutable .arguments
> +    // defined with [[ThrowTypeError]].

immutable .arguments?

This comment is confusing, I think you can just say

// ... *any* function! That includes
// natives, strict mode functions, bound functions, arrow functions,
// self-hosted functions, and probably a few others I forgot.
// Turn back and save yourself while you can. It's too late for me.

Well, that came out a little dramatic, but you get the picture.

@@ +121,5 @@
> +    RootedFunction fun(cx, &args.thisv().toObject().as<JSFunction>());
> +
> +    // The "arguments" property on strict mode and bound functions is a poison
> +    // pill.  Implement the same semantics here.  (Note: asm.js functions
> +    // aren't builtins and aren't interpreted.)

The wording is weirder than it needs to be, given that the poison pills are going away. Now it can just say 'If the argument is a strict-mode function or bound function, throw." Right?

@@ +147,5 @@
> +    }
> +
> +    // FIXME: Bug 929642 will give builtin functions poison pill "arguments"
> +    //        and "caller" properties.  This accessor should probably behave
> +    //        identically when that happens.

Again, avoiding "poison pill" here seems to improve the comment, ymmv: 'Bug 929642 will break "arguments" and "caller" on builtin functions. [...]'

@@ +194,5 @@
> +    RootedFunction fun(cx, &args.thisv().toObject().as<JSFunction>());
> +
> +    // The "arguments" property on strict mode and bound functions is a poison
> +    // pill.  Implement the same semantics here.  (Note: asm.js functions
> +    // aren't builtins and aren't interpreted.)

Same comment on these two comments as above in the getter.

@@ +216,5 @@
> +    if (!JS_ReportErrorFlagsAndNumber(cx, JSREPORT_WARNING | JSREPORT_STRICT, js_GetErrorMessage,
> +                                      nullptr, JSMSG_DEPRECATED_USAGE, js_arguments_str))
> +    {
> +        return false;
> +    }

Please factor out the roughly 30 lines of apparently shared code here. Same comment for the other getter and setter below. This contains a lot of tricky comments and some tricky code. DRY.

@@ +918,5 @@
> +    if (!throwTypeError)
> +        return nullptr;
> +
> +    // So long as arguments/caller are added to builtins via resolve/enumerate,
> +    // we must install this *before* marking it non-extensible.  (Otherwise

But with this patch, they aren't added to builtins anymore. Right?

::: js/src/jsfun.h
@@ +168,5 @@
>  
>      /* Returns the strictness of this function, which must be interpreted. */
>      bool strict() const {
> +        MOZ_ASSERT(isInterpreted());
> +        return isInterpretedLazy() ? lazyScript()->strict() : nonLazyScript()->strict();

+1.

::: js/src/tests/ecma_5/extensions/function-caller-strict-cross-global.js
@@ +2,5 @@
> +// Any copyright is dedicated to the Public Domain.
> +// http://creativecommons.org/licenses/publicdomain/
> +
> +var g1 = newGlobal();
> +g1.evaluate("function f() { return arguments.callee.caller; }");

In case I'm missing something -- is there a reason not to say |return f.caller;| here?

::: js/src/tests/poison-pill-changes.diff
@@ +198,5 @@
> ++    $ERROR("#3: " + name + " should not appear as a descriptor");
> ++  }
> ++}
> ++
> ++var argumentsPoison =

I get that this is the minimal change, but it's a bit confusing having it named "poison" now that it's not really poison but just legacy cruft we're carrying around...

Comment 11

[Jeff Walden [:Waldo]]

I think I have a modified patch done, but I need bug 1055152 fixed before I can be sure my patch doesn't break any tests.  (I can run the tests with timeout limit bumped up, but that's awfully dodgy, and I don't even know how far a bump-up would be needed to determine this, in any event.  If I get desperate I'll give it a try.)

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> I wonder if there are any tests at all for this stuff on arrow functions or
> asm.js functions. ...What happens if you try to walk the stack using .caller
> on asm.js code, from an FFI call back into JS, anyway? :-P

NonBuiltinFrameIterator skips over asm.js frames entirely, treating them as if they were builtin frames.  At least, as far as my manual gdb pokery suggested.  This behavior is apparently basically untested right now.  The two asm.js jit-tests for caller behavior don't actually exercise any of this.  I'll write some tests.  Curse you, conscience!  :-(

> Blast-from-the-past nit: Prevailing style is to use French (or "non-French",
> as they say in France) spacing, with but a single space between sentences.

One of these days we will see sanity and realize this is 1) not worth trying to maintain or be consistent about, and 2) impossible to be consistent about unless people can manage to change the way they unconsciously type, *only* when typing comments in js/ code.

> Well, that came out a little dramatic, but you get the picture.

I like it!

> The wording is weirder than it needs to be, given that the poison pills are
> going away. Now it can just say 'If the argument is a strict-mode function
> or bound function, throw." Right?

Yeah, changed.

> Again, avoiding "poison pill" here seems to improve the comment, ymmv: 'Bug
> 929642 will break "arguments" and "caller" on builtin functions. [...]'

Yup.

> Please factor out the roughly 30 lines of apparently shared code here. Same
> comment for the other getter and setter below. This contains a lot of tricky
> comments and some tricky code. DRY.

When I factor it out it actually only comes to basically three ifs, which isn't too bad.  But you're right, it's factorable.  I did this to both the arguments and caller paths, as ArgumentsRestrictions and CallerRestrictions functions.  It's a good cleanup.

> But with this patch, they aren't added to builtins anymore. Right?

True, patch detritus from earlier.

> ::: js/src/tests/ecma_5/extensions/function-caller-strict-cross-global.js
> @@ +2,5 @@
> > +// Any copyright is dedicated to the Public Domain.
> > +// http://creativecommons.org/licenses/publicdomain/
> > +
> > +var g1 = newGlobal();
> > +g1.evaluate("function f() { return arguments.callee.caller; }");
> 
> In case I'm missing something -- is there a reason not to say |return
> f.caller;| here?

Not particularly.  I do kind of like that it's more explicit this way about walking the stack, versus finding a named function's caller in the stack (where you have to recognize that the function being named is the current function).  But whatever.  I don't remember if I made this change in my current patch, think I did, but it's not really worth double-checking.

> I get that this is the minimal change, but it's a bit confusing having it
> named "poison" now that it's not really poison but just legacy cruft we're
> carrying around...

True.  It seems best to keep changes to tests-from-test262 to a minimum, tho, and rather pick up changes from there.  If this sticks we can fix things upstream.

Comment 12

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2e00997d30

Filed bug 1057208 on .caller tests walking across/through different sorts of functions, as followup work.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/9f2e00997d30

Comment 14

[Mark S. Miller]

See https://bugs.ecmascript.org/show_bug.cgi?id=3113