744842 - don't include actual args in error.stack.toString()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: rm

Currently, (new Error).stack.toString() includes the actual args passed to functions.  'stack' itself is a non-standard property.  jsc and v8 both have a 'stack', but only include function names and line numbers.  Including actual arguments:
 - caused a hard-to-find FF4 GC top-crash
 - caused trouble for c-p-g and required additional complexity be added
 - makes life harder for bug 659577 (where we'd have to dynamically determine where to find the arguments)
 - I suspect will cause further grief for IM (which does not have or even have the ability to eagerly create StackFrames).
 - is mostly redundant now that we have a proper JS debugger API

This patch removes the arguments (matching jsc/v8) and fixes tests.

 5 files changed, 50 insertions(+), 194 deletions(-)

Comment 1

[David Mandelin [:dmandelin]]

(In reply to Luke Wagner [:luke] from comment #0)
> Created attachment 614441 [details] [diff] [review]
> rm
> 
> Currently, (new Error).stack.toString() includes the actual args passed to
> functions.  'stack' itself is a non-standard property.  jsc and v8 both have
> a 'stack', but only include function names and line numbers.  Including
> actual arguments:

Good job make the case for deletion so clearly!

>  - caused a hard-to-find FF4 GC top-crash
>  - caused trouble for c-p-g and required additional complexity be added
>  - makes life harder for bug 659577 (where we'd have to dynamically
> determine where to find the arguments)
>  - I suspect will cause further grief for IM (which does not have or even
> have the ability to eagerly create StackFrames).
>  - is mostly redundant now that we have a proper JS debugger API
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Very important point! We should keep looking for more opportunities to remove things that are being done (or can be done better) by the Debugger API.

Comment 2

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d6c4305f93

Comment 3

[Marco Bonardo [:mak]]

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

Comment 4

[javascriptjedi]

(In reply to Luke Wagner [:luke] from comment #0)
> Created attachment 614441 [details] [diff] [review]
> rm
> 
> Currently, (new Error).stack.toString() includes the actual args passed to
> functions.  'stack' itself is a non-standard property.  jsc and v8 both have
> a 'stack', but only include function names and line numbers.

Firefox was superior to the others.  When you're ahead in a race, you don't slow down if you get a lead.

>Including> actual arguments:
>  - caused a hard-to-find FF4 GC top-crash
>  - caused trouble for c-p-g and required additional complexity be added
>  - makes life harder for bug 659577 (where we'd have to dynamically
> determine where to find the arguments)
>  - I suspect will cause further grief for IM (which does not have or even
> have the ability to eagerly create StackFrames).

OK, it was painful, I can't argue against that point.

>  - is mostly redundant now that we have a proper JS debugger API

A quick look at this API indicates it's only for use in chrome URLs.  Can it be used in an ordinary, unprivileged browser page, as try { } catch (e) { } can be?

Comment 5

[Luke Wagner [:luke]]

(In reply to javascriptjedi from comment #4)
> Firefox was superior to the others.  When you're ahead in a race, you don't
> slow down if you get a lead.

The race is bigger than Error.stack.

> A quick look at this API indicates it's only for use in chrome URLs.  Can it
> be used in an ordinary, unprivileged browser page, as try { } catch (e) { }
> can be?

You're right that content can't initiate debugging (yet, there are vague plans to allow same-origin Debugger use in the future), but you can from scratchpad.  One of the authors of the new Debugger API has a screencast doing this:
  https://blog.mozilla.org/jorendorff/2012/05/08/screencast-debugger-in-scratchpad