Closed Bug 1213341 Opened 9 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: claude.pache, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 2 obsolete files)

Blocks: 797686
Summary: Error.prototype and other <NativeError>.prototype should be an ordinary object → Error.prototype and <NativeError>.prototype should be ordinary objects
Assignee: nobody → jorendorff
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Stealing, this should "just" involve some ClassSpec changes and probably some fuzzing with tests.
Assignee: jorendorff → evilpies
Blocks: test262
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)
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.
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.
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
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.
Attachment #8812148 - Attachment is obsolete: true
Attachment #8812591 - Flags: review?(arai.unmht)
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)
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.
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)
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+
Attachment #8812594 - Flags: review?(bobbyholley)
Attached patch TestsSplinter Review
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+
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
Blocks: 1320198
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: