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

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: claude.pache, Assigned: evilpie)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla53
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Updated

3 years ago
Blocks: 797686
(Reporter)

Updated

3 years ago
Summary: Error.prototype and other <NativeError>.prototype should be an ordinary object → Error.prototype and <NativeError>.prototype should be ordinary objects
Keywords: dev-doc-needed
Assignee: nobody → jorendorff
(Assignee)

Updated

2 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 1

2 years ago
Stealing, this should "just" involve some ClassSpec changes and probably some fuzzing with tests.
Assignee: jorendorff → evilpies
(Assignee)

Updated

2 years ago
Blocks: 652780
(Assignee)

Comment 2

2 years ago
Created attachment 8812148 [details] [diff] [review]
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.
Attachment #8812148 - Flags: feedback?(arai.unmht)
(Assignee)

Comment 3

2 years ago
Oh I just noticed I forgot to add a new ClassSpec for the proto classes.
(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.
(Assignee)

Comment 5

2 years ago
Created attachment 8812380 [details] [diff] [review]
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 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.
Attachment #8812148 - Flags: feedback?(arai.unmht) → feedback+
about ClassSpec, filed bug 1318815.
(Assignee)

Comment 8

2 years ago
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
(Assignee)

Comment 9

2 years ago
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.
(Assignee)

Comment 11

2 years ago
Created attachment 8812591 [details] [diff] [review]
Use ordinary objects for Error prototypes
Attachment #8812148 - Attachment is obsolete: true
Attachment #8812591 - Flags: review?(arai.unmht)
(Assignee)

Comment 12

2 years ago
Created attachment 8812592 [details] [diff] [review]
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.
Attachment #8812592 - Flags: review?(arai.unmht)
(Assignee)

Comment 13

2 years ago
Created attachment 8812594 [details] [diff] [review]
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.
(Assignee)

Comment 14

2 years ago
Created attachment 8812595 [details] [diff] [review]
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.
Attachment #8812595 - Flags: review?(jdemooij)
(Assignee)

Updated

2 years ago
Attachment #8812380 - Attachment is obsolete: true
Attachment #8812591 - Flags: review?(arai.unmht) → review+
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
Attachment #8812592 - Flags: review?(arai.unmht) → review+
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
Attachment #8812595 - Flags: review?(jdemooij) → review+
(Assignee)

Updated

2 years ago
Attachment #8812594 - Flags: review?(bobbyholley)
(Assignee)

Comment 17

2 years ago
Created attachment 8812795 [details] [diff] [review]
Tests
Attachment #8812795 - Flags: review?(arai.unmht)
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.
Attachment #8812795 - Flags: review?(arai.unmht) → review+
Attachment #8812594 - Flags: review?(bobbyholley) → review+

Comment 19

2 years ago
Pushed by evilpies@gmail.com:
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
Depends on: 1319952
(Assignee)

Updated

2 years ago
Blocks: 1320198
Added to https://developer.mozilla.org/en-US/Firefox/Releases/53#JavaScript
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.