Closed Bug 857648 Opened 7 years ago Closed 5 years ago

stack property on DOMException errors is missing/undefined

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jonas, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-needed)

Attachments

(6 files)

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.43 Safari/537.31

Steps to reproduce:

Executing the following code
function trace() {
    try {
        document.body.removeChild(document.createElement('div'));
    }
    catch(e) {
        alert(e.stack);
    }
}
trace();


Actual results:

e.stack is undefined.


Expected results:

e.stack should have contained a stack trace like documented in https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Error/Stack

There is no reason not to fill a stack trace here, exceptions related to dom manipulation is one area that stack traces are really important to have.
FWIW, I see this with testing against Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.8.1.20) Gecko/20081217 Firefox/2.0.0.20 ID:2008121709 too.
Assignee: nobody → general
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
JS allows throwing arbitrary objects as exceptions, unfortunately, and what's thrown here is not an Error object at all.
There's no point in keeping this an unconfirmed JS bug. Moving to DOM, since an arbitrary object in this case is DOMException.

This is related to bug 125612, which asks for the JS engine to store source information when throwing arbitrary objects.
Assignee: general → nobody
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → DOM
Ever confirmed: true
OS: Windows 8 → All
Hardware: x86_64 → All
Summary: Error object stack property is undefined → stack property on DOMException errors is missing/undefined
Version: 19 Branch → Trunk
Blocks: 866788
Getting this info out of the JS engine right now in finite time is rocket science.  Once bug 972045 lands maybe it will be better.
Depends on: 972045
Nick, does the work in bug 972045 make it any simpler to take a stack snapshot and format it the way Error.stack is formatted?
Er, Nick, see comment 5.
Flags: needinfo?(nfitzgerald)
The |SavedFrame.prototype.toString| method should use the same format: http://mxr.mozilla.org/mozilla-central/source/js/src/vm/SavedStacks.cpp#311
Flags: needinfo?(nfitzgerald)
Excellent.  Do you think it makes sense to convert JS::DescribeStack to returning something that holds SavedFrames instead of what it does right now?
Flags: needinfo?(nfitzgerald)
Hrm.  Looks like SavedFrame does line number computations eagerly, as well as atomization of the filename.  :(  Not sure what that means in terms of performance in practice, depending on how often insertFrames is called...
(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #9)
> Hrm.  Looks like SavedFrame does line number computations eagerly, as well
> as atomization of the filename.  :(  Not sure what that means in terms of
> performance in practice, depending on how often insertFrames is called...

Yeah, ideally this would not be the case, and we have some ideas that we talked about here: https://lists.mozilla.org/pipermail/dev-tech-js-engine-internals/2014-January/001555.html
Flags: needinfo?(nfitzgerald)
Do you know whether there are concrete plans for that?

The laziness currently in DescribeStack is there because the eager computation was being a "real-world" (or at least benchmark) performance problem.  So I'm looking for something that has not-worse performance characteristics in cases where lots of exceptions are thrown in the same function (which might have different callers) but allows me to get the .stack string value sanely (which the current return value of DescribeStack does not).
It's not on the top of my TODO list, but it is something I'd like to do eventually.
Depends on: 1004110
Nick filed bug 1004110 to track lazy line number computation.
No longer depends on: 1027305
Depends on: 1030938
Depends on: 1031143
Depends on: 1031152
Depends on: 1031168
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8448914 - Flags: review?(jimb) → review+
Comment on attachment 8448907 [details] [diff] [review]
part 3.  Switch from using JS::DescribeStack to JS::CaptureCurrentStack for producing JSStackFrames

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

::: dom/bindings/Exceptions.cpp
@@ +206,5 @@
>    {
>      return false;
>    }
>  
> +  virtual nsresult GetLineno(int32_t* aLineNo)

boo

@@ +340,5 @@
>  
>  /* readonly attribute AString filename; */
>  NS_IMETHODIMP JSStackFrame::GetFilename(nsAString& aFilename)
>  {
>    if (!mFilenameInitialized) {

Seems like there's an opportunity for a "GetPropertyFromStack" function that consolidates a bunch of this code across the various functions.
Attachment #8448907 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #21)
> Seems like there's an opportunity for a "GetPropertyFromStack" function that
> consolidates a bunch of this code across the various functions.

Although I guess you have to have the ThreadSafeAutoJSContext and a Rooted outside the function to receive the result, so maybe it doesn't buy anything ...
Comment on attachment 8448912 [details] [diff] [review]
part 5.  Expose a .stack property on DOMExceptions

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

::: dom/webidl/DOMException.webidl
@@ +58,5 @@
>    readonly attribute nsISupports?            data;
> +
> +  // Formatted exception stack
> +  [Throws]
> +  readonly attribute DOMString               stack;

Should this be nullable, since with a null mLocation we won't assign to the string?
Attachment #8448912 - Flags: review?(khuey) → review+
> boo

Yeah, I couldn't think of a better way.

> Seems like there's an opportunity for a "GetPropertyFromStack" function 

It doesn't buy that much, and I hope most of that goop will go away when bug 1031152 happens.

> Should this be nullable, since with a null mLocation we won't assign to the string?

No, that just means null mLocation means stack == "".  That's fine.

Thanks for the reviews!
Yeah, this is causing some of the oranges.  Filed bug 1034463 on that.
Depends on: 1034463
And some of the failures are definitely due to SavedStacks not picking up the whole stack.  That's bug 1034477.
Depends on: 1034477
Blocks: 1036527
Looks like we need to make this property replaceable, because code out there (e.g. Task.jsm) will set .stack on exception objects.
I've added this note to Firefox 33 for devs ( https://developer.mozilla.org/en-US/Firefox/Releases/33 ):

A non-standard DOMException.stack property has been added. It returns a DOMString with a human-friendly formatted stack (bug 857648).

Reading the webidl, I think this is correct, but I'm not sure it is complete enough (Didn't find a usage of the Exception i/f). Bz, can you confirm or infirm?
Flags: needinfo?(bzbarsky)
Thanks!  I fixed up the description to make it clear that this has the same behavior as .stack on built-in Error objects.
Flags: needinfo?(bzbarsky)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.