Closed Bug 1317387 Opened 3 years ago Closed 3 years ago

%ThrowTypeError% should be frozen

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Test case:
---
ThrowTypeError = (function(){"use strict"; return Object.getOwnPropertyDescriptor(arguments,"callee").get; })();

print(Object.isFrozen(ThrowTypeError));
---

Expected: Prints "true"
Actual: Prints "false"
Attached patch bug1317387.patch (obsolete) — Splinter Review
I wasn't sure if deleting %ThrowTypeError%'s "name" property or changing it to non-configurable is the better choice. In the end I decided to make it non-configurable instead of deleting it, because all other built-in anonymous functions currently have an empty name in SpiderMonkey, and I wanted to keep this consistent.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8810933 - Flags: review?(till)
Comment on attachment 8810933 [details] [diff] [review]
bug1317387.patch

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

Sure. I kinda wish test262 didn't care about the names and lengths of builtins like this: I can't think of a reason for differences in these to pose any interoperability issues, so I'd rather we spend our time on things that actually do matter.

Having said that, the Promise tests that fail because of the names of the resolve/reject functions *do* annoy me very much, so I fully understand why you're actually doing this. I.e., I really just wish the tests didn't exists.

::: js/src/jsfun.cpp
@@ +886,5 @@
> +
> +    // Non-standard: Also change "name" to non-configurable. ECMAScript defines
> +    // %ThrowTypeError% as an anonymous function, i.e. it shouldn't actually
> +    // get an own "name" property. To be consistent with other built-in,
> +    // anonymous functions, we don't delete %ThrowTypeError%'s "name" property.

It'd be nice to have support for this and have the other anonymous builtins actually be anonymous, too. It's also really, really very much not important. At all.

::: js/src/tests/ecma_6/Function/throw-type-error.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Please remove this header: tests should be CC0 - and are by default, so no header is required.
Attachment #8810933 - Flags: review?(till) → review+
(In reply to Till Schneidereit [:till] from comment #2)
> Sure. I kinda wish test262 didn't care about the names and lengths of
> builtins like this: I can't think of a reason for differences in these to
> pose any interoperability issues, so I'd rather we spend our time on things
> that actually do matter.

With my implementer's hat put on, I also don't care that much about the name and length values of built-ins. But it's a totally different story when I put on my specification editor's hat. :-)
But for this issue I was more concerned about ensuring that %ThrowTypeError% is a frozen object, because that was an explicit design choice for the Secure EcmaScript project.
Attached patch bug1317387.patch (obsolete) — Splinter Review
Updated to replace MPL with CC0 in test file. Carrying r+ from Till.
Attachment #8810933 - Attachment is obsolete: true
Attachment #8811350 - Flags: review+
Keywords: checkin-needed
Attached patch bug1317387.patchSplinter Review
Now even with the updated version.
Attachment #8811350 - Attachment is obsolete: true
Attachment #8811354 - Flags: review+
(In reply to André Bargull from comment #3)
> With my implementer's hat put on, I also don't care that much about the name
> and length values of built-ins. But it's a totally different story when I
> put on my specification editor's hat. :-)

Hah, yes. I do very much understand that.

> But for this issue I was more concerned about ensuring that %ThrowTypeError%
> is a frozen object, because that was an explicit design choice for the
> Secure EcmaScript project.

Right, I wasn't talking about that part, as that absolutely makes sense.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf4ecf3ac6b8
The intrinsic %ThrowTypeError% function should be frozen. r=till
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cf4ecf3ac6b8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
We should probably look into backporting this.
Flags: needinfo?(andrebargull)
The patch is relatively simple, but I'm not sure it's worth the effort to backport this one, because %ThrowTypeError% is still unfrozen in JSC and Chakra, so we're not the only engine with a spec violation for %ThrowTypeError%.
Flags: needinfo?(andrebargull)
You need to log in before you can comment on or make changes to this bug.