Closed Bug 894653 Opened 6 years ago Closed 4 years ago

Self-host Error.prototype.toString

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(3 files)

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.
Attached patch 1 - Fix IsObjectSplinter Review
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)
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 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+
Attached patch 2 - Take twoSplinter Review
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 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+
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
Whiteboard: [leave open]
maybe this caused also the assertion failure report from bug 895298
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.
Depends on: 1133143
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.