Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NativeError.prototype.toString? #1794

Closed
bathos opened this issue Nov 24, 2019 · 14 comments
Closed

NativeError.prototype.toString? #1794

bathos opened this issue Nov 24, 2019 · 14 comments

Comments

@bathos
Copy link
Contributor

bathos commented Nov 24, 2019 •
edited
Loading

I believe that Firefox is correct and that none of these properties ought to exist:

NativeError property Chrome Firefox Safari
EvalError.prototype.toString 1 3 2
RangeError.prototype.toString 1 3 2
ReferenceError.prototype.toString 1 3 2
SyntaxError.prototype.toString 1 3 2
TypeError.prototype.toString 1 3 2
URIError.prototype.toString 1 3 2
WebAssembly.CompileError.prototype.toString 1 3 3
WebAssembly.LinkError.prototype.toString 1 3 3
WebAssembly.RuntimeError.prototype.toString 1 3 3
  1. toString is an own data property, and its value is the same as Error.protototype.toString
  2. toString is an own data property, and its value is a unique function
  3. toString is not present (but is inherited from Error.prototype)

I included WebAssembly errors above because they’re defined in terms of the ‘NativeError Object Structure’ and because they’re interesting due to being outlying cases in Safari.

Probably none of these results are technically implementation errors because implementations are permitted to add additional properties. But it seems likely that these differences aren’t intentional, and they’re a fairly weird/seemingly pointless spot to diverge.

If somebody can confirm which of these aren’t correct, I’ll open issues in the appropriate places. I didn’t turn up any existing reports.
@ljharb
Copy link
Member

ljharb commented Nov 24, 2019

The spec would say it should be 3.

Are there test262 tests that cover this at all?

As for changing it, i suspect it would be easily web compatible for the 2s to change to 3s, which would be better, but I’m slightly less confident about the 2s and 3s changing to 1s.

@devsnek
Copy link
Member

devsnek commented Nov 24, 2019 •
edited
Loading

it appears that what V8 has rn was an accident (introduced in v8/v8@9211dee), I'll open a CL to change it and hopefully it will be accepted.

I also couldn't find any test262 tests covering this.

@devsnek devsnek mentioned this issue Nov 24, 2019
Closed
@ljharb
Copy link
Member

ljharb commented Nov 24, 2019

Sounds like the first step is to make test262 tests to cover what the spec says.

@bathos
Copy link
Contributor Author

bathos commented Nov 24, 2019 •
edited
Loading

I figured the reason there weren’t tests was that it’s not a conformance issue for these other properties to exist from 262’s perspective. That is, even though intuitively we can tell that this divergence was unlikely to have been intentional, I thought checking this was outside the scope of test262 due to

A conforming implementation of ECMAScript may provide additional types, values, objects, properties, and functions beyond those described in this specification. In particular, a conforming implementation of ECMAScript may provide properties not described in this specification, and values for those properties, for objects that are described in this specification.

@ljharb
Copy link
Member

ljharb commented Nov 24, 2019

I think that's technically true, but i don't personally think this divergence is in the spirit of that loophope.

@mathiasbynens
Copy link
Member

cc @schuay re: v8/v8@9211dee

@devsnek
Copy link
Member

devsnek commented Nov 25, 2019

@mathiasbynens fyi https://chromium-review.googlesource.com/c/v8/v8/+/1933054

@devsnek
Copy link
Member

devsnek commented Nov 26, 2019

I also opened https://bugs.webkit.org/show_bug.cgi?id=204629

@allenwb
Copy link
Member

allenwb commented Nov 27, 2019 •
edited by ljharb
Loading

3 is what the spec. has said going all the way back to ES3. This specified behavior was reviewed and confirmed during ES5 development Even though there there was considerable variation of the actual placement of properties among implementations at that time (ie, 2008) only one change was made to the specification. The initial value of NativeError.prototype.message was respecified to be the empty string. In ES3 it had been specified as "implementation defined".

However, it appears that the ES5 change did miss something. In ES3 the first paragraph in the "NativeError Objct Structure" clause ended with:

and in the implementation-defined message property of the prototype object.

The "implementation-defined" adjective should have been removed when the ES5 change was made. It wasn't and that incorrect text remains in the working draft. It should be removed.

@ljharb
Copy link
Member

ljharb commented Jan 3, 2020

To summarize my interpretation of the state of this issue:

  • editorial: the phrase "implementation-defined" should be removed from https://tc39.es/ecma262/#sec-nativeerror-object-structure, a PR is welcome (@bathos?)
  • existential: should the spec find some way of ensuring that implementation extensions do not shadow inherited properties on a builtin with an own property that the spec does not define?

Thoughts?

joyeecheung pushed a commit to joyeecheung/v8 that referenced this issue Jan 16, 2020
@devsnek
Remove own toString from NativeError prototype
cf7646c 
In 5742da0, the toString property was
accidentally applied to all NativeError prototypes, when it should only
be inherited from Error.prototype.

Refs: tc39/ecma262#1794
Bug: v8:10017
Change-Id: I2af9a31f463deb9871dd7a4a5a2e4dd7485ed38c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1933054
Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65355}
@rkirsling
Copy link
Member

As of the latest V8 and WebKit revisions (Chrome and Safari releases notwithstanding) the chart should now be 3s everywhere.

@devsnek
Copy link
Member

devsnek commented Jan 21, 2020

^ chakra still has this bug, but its not in any browser

@ljharb
Copy link
Member

ljharb commented Jan 21, 2020

Has an issue been filed on ChakraCore?

@devsnek devsnek mentioned this issue Jan 21, 2020
Closed
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
@devsnek @webkit-commit-queue
Remove own toString from NativeError prototype
96efcf1 
https://bugs.webkit.org/show_bug.cgi?id=204629

Patch by Gus Caplan <me@gus.host> on 2020-01-20
Reviewed by Ross Kirsling.

NativeError prototypes are expected to inherit toString from
Error.prototype. See tc39/ecma262#1794
for additional details.

JSTests:

* stress/nativeerror-prototype-tostring.js:

Source/JavaScriptCore:

* runtime/ErrorPrototype.cpp:
(JSC::ErrorPrototypeBase::ErrorPrototypeBase):
(JSC::ErrorPrototypeBase::finishCreation):
(JSC::ErrorPrototype::ErrorPrototype):
(JSC::ErrorPrototype::create): Deleted.
(JSC::ErrorPrototype::finishCreation): Deleted.
* runtime/ErrorPrototype.h:
(JSC::ErrorPrototype::createStructure): Deleted.
* runtime/NativeErrorPrototype.cpp:
(JSC::NativeErrorPrototype::NativeErrorPrototype):
* runtime/NativeErrorPrototype.h:

LayoutTests:

* http/tests/security/regress-52192-expected.txt:
* http/tests/security/regress-52192.html:

Canonical link: https://commits.webkit.org/219579@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@254842 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@devsnek
Copy link
Member

devsnek commented Mar 14, 2023

This seems to have been long fixed everywhere :)

@devsnek devsnek closed this as completed Mar 14, 2023
@drakedevel drakedevel mentioned this issue Mar 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@ljharb @mathiasbynens @allenwb @rkirsling @devsnek @bathos