Closed
Bug 1213341
Opened 9 years ago
Closed 8 years ago
Error.prototype and <NativeError>.prototype should be ordinary objects
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: claude.pache, Assigned: evilpies)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(5 files, 2 obsolete files)
11.90 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
5.11 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
3.67 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
1.04 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
1.93 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
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]"
Reporter | ||
Updated•9 years ago
|
Summary: Error.prototype and other <NativeError>.prototype should be an ordinary object → Error.prototype and <NativeError>.prototype should be ordinary objects
Updated•9 years ago
|
Keywords: dev-doc-needed
Updated•9 years ago
|
Assignee: nobody → jorendorff
Stealing, this should "just" involve some ClassSpec changes and probably some fuzzing with tests.
Assignee: jorendorff → evilpies
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.
Comment 4•8 years ago
|
||
(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 6•8 years ago
|
||
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+
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8812148 -
Attachment is obsolete: true
Attachment #8812591 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 12•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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
Updated•8 years ago
|
Attachment #8812591 -
Flags: review?(arai.unmht) → review+
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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)
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8812795 -
Flags: review?(arai.unmht)
Comment 18•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8812594 -
Flags: review?(bobbyholley) → review+
Comment 19•8 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
Comment 20•8 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 21•8 years ago
|
||
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•