894653 - Self-host Error.prototype.toString

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: 1 - Fix IsObject

This has been lying around incorrect for a bit (used only in assertions, I believe), but I need it now.  Time to fix it.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: 2 - Self-host it, with a test for the issue the previous patch precognitively fixes

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 776732 [details] [diff] [review]
1 - Fix IsObject

Review of attachment 776732 [details] [diff] [review]:
-----------------------------------------------------------------

Urgh, yes.

Comment 4

[Till Schneidereit [:till]]

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'}.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: 2 - Take two

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.

Comment 6

[Till Schneidereit [: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.

Comment 7

[Jeff Walden [:Waldo]]

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.

Comment 8

[Carsten Book [:Tomcat]]

maybe this caused also the assertion failure report from bug 895298

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2e0530773d5

Comment 10

[Jeff Walden [:Waldo]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/364dae502747