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

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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(-)
Attachment #614441 - Flags: review?(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.
Attachment #614441 - Flags: review?(dmandelin) → review+
(Assignee)

Comment 2

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d6c4305f93
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/c7d6c4305f93
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 836916

Comment 4

5 years ago
(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?
(Assignee)

Comment 5

5 years ago
(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
You need to log in before you can comment on or make changes to this bug.