Last Comment Bug 744842 - don't include actual args in error.stack.toString()
: don't include actual args in error.stack.toString()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 836916
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-12 10:46 PDT by Luke Wagner [:luke]
Modified: 2013-02-05 08:24 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rm (19.50 KB, patch)
2012-04-12 10:46 PDT, Luke Wagner [:luke]
dmandelin: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-04-12 10:46:12 PDT
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:
 - 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] 2012-04-12 11:32:02 PDT
(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 3 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-13 04:32:50 PDT
https://hg.mozilla.org/mozilla-central/rev/c7d6c4305f93
Comment 4 javascriptjedi 2013-02-05 07:17:52 PST
(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] 2013-02-05 08:24:51 PST
(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

Note You need to log in before you can comment on or make changes to this bug.