A TypeError should be thrown when accessing 'arguments' or 'caller' on any of the new function types

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: zirakertan, Assigned: zirakertan)

Tracking

52 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 months ago
Created attachment 8821303 [details] [diff] [review]
Detect newer-style functions when accessing 'arguments' or 'caller'

User Agent: Mozilla/5.0 (Windows NT 10.0.14393; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2950.4 Safari/537.36

Steps to reproduce:

Section 16.2 of the ES spec, "Forbidden Extensions", specifies that newer-type functions should behave the same way as strict-mode functions when one accesses their 'arguments' and 'caller' properties, i.e. throw a TypeError.
By "newer-type functions", my intention is:
* Arrow functions
* Async functions
* Classes
* Generators
* Methods

In other words:

	function strictFoo() { "use strict" }
	strictFoo.arguments // TypeError, as expected

	async function asyncFoo() {}
	asyncFoo.arguments // Should throw

You can see the committee notes here: https://github.com/rwaldron/tc39-notes/blob/46d2396e02fd73121b5985d5a0fafbcdbf9c9072/es6/2014-07/jul-29.md#conclusionresolution-5
The parallel v8 bug: https://bugs.chromium.org/p/v8/issues/detail?id=3946

Attached is a patch with a few test cases.

Thanks for your time!
Attachment #8821303 - Attachment is patch: true
Attachment #8821303 - Attachment mime type: text/x-patch → text/plain
Attachment #8821303 - Flags: review?(shu)

Comment 1

6 months ago
Comment on attachment 8821303 [details] [diff] [review]
Detect newer-style functions when accessing 'arguments' or 'caller'

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

Thank you for the patch and good find!

I'd like to see a new version of the test. Everything else looks good.

::: js/src/jsfun.cpp
@@ +129,3 @@
>      return IsAsmJSStrictModeModuleOrFunction(fun);
>  }
> +static bool IsNewerTypeFunction(JSFunction* fun) {

Style nit: the SpiderMonkey style guide would do

static bool
IsNewerTypeFunction(JSFunction* fun)
{
  ...
}

@@ +146,4 @@
>      // a strict mode function, or a bound function.
>      // TODO (bug 1057208): ensure semantics are correct for all possible
>      // pairings of callee/caller.
> +    if (fun->isBuiltin() || IsFunctionInStrictMode(fun) || fun->isBoundFunction() || IsNewerTypeFunction(fun)) {

Style nit: SpiderMonkey limits columns to 100 characters wide, and, for multi-line conditions, the { goes on its own line. So this would look something like:

if (fun->isBuiltin() ||
    IsFunctionInStrictMode(fun) ||
    fun->isBoundFunction() ||
    IsNewerTypeFunction(fun))
{
  ...
}

@@ +233,4 @@
>      // a strict mode function, or a bound function.
>      // TODO (bug 1057208): ensure semantics are correct for all possible
>      // pairings of callee/caller.
> +    if (fun->isBuiltin() || IsFunctionInStrictMode(fun) || fun->isBoundFunction() || IsNewerTypeFunction(fun)) {

Ditto.

::: js/src/tests/ecma_6/extensions/newer-type-functions-caller-arguments.js
@@ +6,5 @@
> +// * Async functions
> +// * Generators
> +// * Method definitions
> +// Note that classes run in strict mode by default, so they are covered by the
> +// first clause.

As discussed on IRC, the reason we need to throw here is:

1. ES 9.2.7 AddRestrictedFunctionProperties adds setters to Function.prototype that throw on setting "caller" and "arguments".
2. ES 16.2 "Forbidden Extensions" forbids adding own properties for basically all functions except sloppy functions introduced via |function () { ... }|.
3. So implementations are allowed to add own properties named "caller" and "arguments" to sloppy functions that shadow the setters that throw. But, for all other kinds of functions, they are disallowed and thus must throw.

@@ +59,5 @@
> +
> +function isProperTypeError(e) {
> +    return e instanceof TypeError &&
> +        e.message === "'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them";
> +}

No need to test for the exact message.

For most of this test, you can replace the handwritten try-catches with things like

assertThrowsInstanceOf(() => f.arguments, TypeError)
Attachment #8821303 - Flags: review?(shu)
(Assignee)

Comment 2

6 months ago
Created attachment 8821369 [details] [diff] [review]
Fix review in comment #1

Addressed shu's review.
Attachment #8821369 - Flags: review+
(Assignee)

Updated

6 months ago
Attachment #8821369 - Flags: review+ → review?(shu)

Comment 3

6 months ago
Comment on attachment 8821369 [details] [diff] [review]
Fix review in comment #1

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

Looks great, thanks.
Attachment #8821369 - Flags: review?(shu) → review+
(Assignee)

Updated

6 months ago
Keywords: checkin-needed

Updated

6 months ago
Assignee: nobody → zirakertan
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core

Comment 4

6 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd9c8a64775
"A TypeError should be thrown when accessing 'arguments' or 'caller' on any of the new function types". r=shu
Keywords: checkin-needed

Comment 5

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7dd9c8a64775
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.