Closed
Bug 894653
Opened 12 years ago
Closed 10 years ago
Self-host Error.prototype.toString
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
Attachments
(3 files)
697 bytes,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
5.29 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
5.91 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
I wrote up a self-hosted version of this to update MDN docs for the method (which as noted in bug 894446 were buggy). We might as well dump it in our tree now that it's written.
Assignee | ||
Comment 1•12 years ago
|
||
This has been lying around incorrect for a bit (used only in assertions, I believe), but I need it now. Time to fix it.
Attachment #776732 -
Flags: review?(till)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #776735 -
Flags: review?(till)
Comment 3•12 years ago
|
||
Comment on attachment 776732 [details] [diff] [review]
1 - Fix IsObject
Review of attachment 776732 [details] [diff] [review]:
-----------------------------------------------------------------
Urgh, yes.
Attachment #776732 -
Flags: review?(till) → review+
Comment 4•12 years ago
|
||
Comment on attachment 776735 [details] [diff] [review]
2 - Self-host it, with a test for the issue the previous patch precognitively fixes
Review of attachment 776735 [details] [diff] [review]:
-----------------------------------------------------------------
Only nits.
::: js/src/builtin/Error.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* ES5 15.11.4.4, with subsequent errata. */
Given that this implementation still fits the bill (with s/3-4/3-5/ and s/5-6/6-8/, and +1 for subsequent steps), why not refer to ES6 15.11.3.4? I at least tried to keep spec references as up-to-date as possible.
::: js/src/tests/ecma_5/extensions/error-tostring.js
@@ +1,5 @@
> +// Any copyright is dedicated to the Public Domain.
> +// http://creativecommons.org/licenses/publicdomain/
> +
> +//-----------------------------------------------------------------------------
> +var BUGNUMBER = 999999;
You're probably aware, but this needs changing.
@@ +4,5 @@
> +//-----------------------------------------------------------------------------
> +var BUGNUMBER = 999999;
> +var summary =
> + "Error.prototype.toString called on function objects should work as on any " +
> + "object";
I get that if it works on one kind of object, it works on all of them, the way the implementation works. Still a bit weird to read "any object" here, when only functions are actually tested.
@@ +22,5 @@
> +// extension-land test.
> +
> +assertEq(ErrorToString(function f(){}), "f");
> +assertEq(ErrorToString(function g(){}), "g");
> +assertEq(ErrorToString(function(){}), "Error");
It took me a while to understand that this isn't really WAT-worthy. It *is* a bit of a weird way to test this, though, as functions have a property name "name" for entirely unrelated reasons.
Not important at all, really, but I'd prefer tests that use object literals along the lines of {name : 'foo', message : 'bar'}.
Attachment #776735 -
Flags: review?(till) → review+
Assignee | ||
Comment 5•12 years ago
|
||
Turns out, the spec *doesn't* actually say that name/message both equal "" means "Error" should be returned, as the ES6 draft says when I consulted it to renumber stuff. Fixed that, fixed the tests busted by that, and now I think we're good here.
Attachment #777285 -
Flags: review?(till)
Comment 6•12 years ago
|
||
Comment on attachment 777285 [details] [diff] [review]
2 - Take two
Review of attachment 777285 [details] [diff] [review]:
-----------------------------------------------------------------
Nice. And sorry for not catching the all-the-fields-are-empty-strings scenario.
Attachment #777285 -
Flags: review?(till) → review+
Assignee | ||
Comment 7•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e0530773d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/44347cbdf6a0
But it seems the second part causes the jsapi-test testChromeBuffer to fail. I didn't see the obvious problem in a few minutes of looking, so I backed out the second bit:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8526e71759
I'll look into this tomorrow.
Target Milestone: --- → mozilla25
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave open]
Comment 8•12 years ago
|
||
maybe this caused also the assertion failure report from bug 895298
Comment 9•12 years ago
|
||
Assignee | ||
Comment 10•11 years ago
|
||
Investigated the testChromeBuffer thing, finally -- the problem is, in that test, the line that does:
return 'From trusted: ' + e;
with an InternalError exception, in a situation where we're into the trusted-principal reserved-stack space. When Error.prototype.toString is a native, we never call InterpreterStack::allocateFrame and so never throw for too-much-recursion because we're calling past the max frames count. But when it's scripted as here, we do try to allocate a frame, and that immediately throws.
I've fixed that locally by changing the line to:
return 'From trusted: ' + e.name + ': ' + e.message;
which for the code here has the same effect but, because these properties are data properties, doesn't try to allocate a stack frame.
https://tbpl.mozilla.org/?tree=Try&rev=3e73adc7b8dd is looking pretty good at this point, with the bug 895298 issue no longer apparent (and the stack mentioned in that bug mentions all sorts of things that have changed in our bootstrapping code, so it's probably dead from something else). So I'll likely push this and a couple other queued-up patches later in the morning.
Comment 11•11 years ago
|
||
Flags: in-testsuite+
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in
before you can comment on or make changes to this bug.
Description
•