1213341 - Error.prototype and <NativeError>.prototype should be ordinary objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Claude Pache]

Reference:

http://www.ecma-international.org/ecma-262/6.0/#sec-properties-of-the-error-prototype-object
http://www.ecma-international.org/ecma-262/6.0/#sec-properties-of-the-nativeerror-prototype-objects

Test case:

js> Object.prototype.toString.call(Error.prototype) // should be: "[object Object]"

Comment 1

[Tom S]

Stealing, this should "just" involve some ClassSpec changes and probably some fuzzing with tests.

Comment 2

[Tom S]

Attachment: Set correct prototype for Error objects

This patch does two things: For NativeErrors (TypeError, RangeError etc.) it sets NativeError.prototype.__proto__ == Error.prototype and also for the constructor we set NativeError.__proto__ == Error.

There are some open questions about how to define properties on the prototype. For example we could define name and message via the ClassSpec properties list, but that would mean a new JSPropertySpec and ClassSpec per error type!

Because Error.prototype isn't a error instance anymore Error.prototype.stack now throws, do we need to special case that? ErrorObject_checkAndUnwrapThis has this example: `Object.create(Error.prototype)`, which would now throw when looking up .stack. Aside: How is this even supposed to work? Looks like .stack would still always be an empty string.

Comment 3

[Tom S]

Oh I just noticed I forgot to add a new ClassSpec for the proto classes.

Comment 4

[Tooru Fujisawa [:arai]]

(In reply to Tom Schuster [:evilpie] from comment #2)
> There are some open questions about how to define properties on the
> prototype. For example we could define name and message via the ClassSpec
> properties list, but that would mean a new JSPropertySpec and ClassSpec per
> error type!

So, iiuc, that's the same situation as TypedArrays, and that won't be so much issue.
(or perhaps I'm missing some issue?)

> Because Error.prototype isn't a error instance anymore Error.prototype.stack
> now throws, do we need to special case that? ErrorObject_checkAndUnwrapThis
> has this example: `Object.create(Error.prototype)`, which would now throw
> when looking up .stack. Aside: How is this even supposed to work? Looks like
> .stack would still always be an empty string.

Just like RegExp.prototype.global getter, we could add exception to Error#stack getter, to ignore prototype and just return empty string, or maybe undefined.


I'll finish feedback by maybe later today.

Comment 5

[Tom S]

Attachment: Change Error#stack to use CallNonGenericMethod machinery

I came up with the same approach, but it seems like there a few test failures related to this that I need to figure out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7935a71c7d31829a13f0f8a095bec9b150af730c

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8812148 [details] [diff] [review]
Set correct prototype for Error objects

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

(In reply to Tom Schuster [:evilpie] from comment #3)
> Oh I just noticed I forgot to add a new ClassSpec for the proto classes.

If using ErrorObject::errorClassSpec_ works including Xray, it should be fine.
delegation was introduced before separating ClassSpec from Class.

We should rethink about it based on current structure,
and maybe we can remove delegation for some case.

::: js/src/jsexn.cpp
@@ +75,5 @@
> +        classSpecPtr \
> +    }
> +
> +const Class
> +ErrorObject::protoClasses[JSEXN_LIMIT] = {

it's pre-existing tho, we could add another enum value for the limit of actual error, excluding JSEXN_WARN.
So that we can reduce the number of elements here and classes.

@@ +169,4 @@
>      IMPLEMENT_ERROR_CLASS(RuntimeError,   &ErrorObject::nonGlobalErrorClassSpec_)
>  };
>  
> +

Please remove this line

@@ +497,4 @@
>          return nullptr;
>  
>      // The various prototypes also have .name in addition to the normal error
>      // instance properties.

This comment needs to be removed.

@@ +499,5 @@
>      // The various prototypes also have .name in addition to the normal error
>      // instance properties.
>      RootedPropertyName name(cx, ClassName(key, cx));
>      RootedValue nameValue(cx, StringValue(name));
> +    if (!DefineProperty(cx, proto, cx->names().name, nameValue, nullptr, nullptr, 0))

Just a rough idea, if it's really troublesome to have JSPropertySpec for each NativeErrors,
can we add MagicValue or something to property spec in addition to STRINGVALUE_WRAPPER and INT32VALUE_WRAPPER, that uses ClassName string value?
so that we don't need JSPropertySpec for each types to have different name

but it's only if having multiple JSPropertySpec has much demerit, since introducing magic value will make it complicated.
I think having JSPropertySpec for each types is okay.

Comment 7

[Tooru Fujisawa [:arai]]

about ClassSpec, filed bug 1318815.

Comment 8

[Tom S]

Thanks arai for the feedback, I think I agree with everything and I changed name/message to be defined by PropertySpec.

However I noticed a type failure on my new try push. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f238db07c086e9dc3fccb084ad736d01d3a4d56e&selectedJob=31514735

Comment 9

[Tom S]

We no longer end up calling "AddTypedPropertyId" for those properties, while creating the prototype DebugeeWouldRun prototype. I am not sure where we should call this instead.

Comment 10

[Tom S]

http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/js/src/vm/ObjectGroup.cpp#585

Comment 11

[Tom S]

Attachment: Use ordinary objects for Error prototypes

Comment 12

[Tom S]

Attachment: Handle the now ordinary error prototype object in stack

I went back to the old code with a small change to allow Error.prototype instead of error instances again. Right now I doubt we could get this landed if Error.prototype.stack threw.

Comment 13

[Tom S]

Attachment: Simplify Error Xray code

This patch simplifies some stuff mostly because `name` is defined by PropertySpec now. This did make we wonder if the now missing Error.protototype.{lineNumber, fileName, columnNumber} properties are go to cause issues somewhere.

Comment 14

[Tom S]

Attachment: Add TI for error properties assigned by the initial shape

IsErrorProtoKey(IdentifyStandardPrototype(obj)) is going to match eg. for RangeError.prototype, because the [[Prototype]] of that is Error.prototype. I assume adding incorrect type information isn't a big deal, considering that I believe "stack" was already wrong.

Comment 15

[Tooru Fujisawa [:arai]]

Comment on attachment 8812592 [details] [diff] [review]
Handle the now ordinary error prototype object in stack

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

::: js/src/jsexn.h
@@ +92,5 @@
> +static inline bool
> +IsErrorProtoKey(JSProtoKey key)
> +{
> +    JSExnType type = static_cast<JSExnType>(key - JSProto_Error);
> +    return type >= JSEXN_ERR && type < JSEXN_WARN;

we can use JSEXN_ERROR_LIMIT now

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8812595 [details] [diff] [review]
Add TI for error properties assigned by the initial shape

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

Yeah, this is fine for now.

::: js/src/vm/ObjectGroup.cpp
@@ +8,4 @@
>  
>  #include "jshashutil.h"
>  #include "jsobj.h"
> +#include "jsexn.h"

Nit: move jsexn.h before jshashutil.h

Comment 17

[Tom S]

Attachment: Tests

Comment 18

[Tooru Fujisawa [:arai]]

Comment on attachment 8812795 [details] [diff] [review]
Tests

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

::: js/src/tests/ecma_6/Error/constructor-proto.js
@@ +9,5 @@
> +];
> +
> +assertEq(Reflect.getPrototypeOf(Error), Function.prototype)
> +
> +for (const error of nativeErrors) {

block can be omitted.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6fc50106d3
Use ordinary objects for Error prototypes. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bcfbda6fe3d
Handle the now ordinary error prototype object in stack. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ec6fd3b9fd
Simplify Error Xray code. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2dcf9c03623
Add TI for error properties assigned by the initial shape. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdff3b61420
Tests. r=arai

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4e6fc50106d3
https://hg.mozilla.org/mozilla-central/rev/4bcfbda6fe3d
https://hg.mozilla.org/mozilla-central/rev/d4ec6fd3b9fd
https://hg.mozilla.org/mozilla-central/rev/d2dcf9c03623
https://hg.mozilla.org/mozilla-central/rev/ecdff3b61420

Comment 21

[Florian Scholz (Open Web Docs)]

Added to https://developer.mozilla.org/en-US/Firefox/Releases/53#JavaScript