857648 - stack property on DOMException errors is missing/undefined

Status: RESOLVED FIXED

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

Description

[jonas]

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.

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

JS allows throwing arbitrary objects as exceptions, unfortunately, and what's thrown here is not an Error object at all.

Comment 3

[Nickolay_Ponomarev]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Er, Nick, see comment 5.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

The |SavedFrame.prototype.toString| method should use the same format: http://mxr.mozilla.org/mozilla-central/source/js/src/vm/SavedStacks.cpp#311

Comment 8

[Boris Zbarsky [:bzbarsky]]

Excellent.  Do you think it makes sense to convert JS::DescribeStack to returning something that holds SavedFrames instead of what it does right now?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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).

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

It's not on the top of my TODO list, but it is something I'd like to do eventually.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Nick filed bug 1004110 to track lazy line number computation.

Comment 14

[Boris Zbarsky [:bzbarsky]]

For my future reference, performance testcases:

https://bug932837.bugzilla.mozilla.org/attachment.cgi?id=8356109
https://bug920116.bugzilla.mozilla.org/attachment.cgi?id=8404981

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove unnecessary code that tries to filter out non-JS stack frames from a known-JS-only stack

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Split JSStackFrame into a generic class and a JS-specific class

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Switch from using JS::DescribeStack to JS::CaptureCurrentStack for producing JSStackFrames

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Add a formattedStack attribute on nsIStackFrame

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Expose a .stack property on DOMExceptions

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Remove the now-unused JS::DescribeStack API

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> 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!

Comment 25

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/97708994eae6
   https://hg.mozilla.org/integration/mozilla-inbound/rev/b617c11c9476
   https://hg.mozilla.org/integration/mozilla-inbound/rev/04b06ee60ebf
   https://hg.mozilla.org/integration/mozilla-inbound/rev/e4dd7f8cf275
   https://hg.mozilla.org/integration/mozilla-inbound/rev/a4524d7aec4f
   https://hg.mozilla.org/integration/mozilla-inbound/rev/03b7d111cc86

Comment 26

[Carsten Book [:Tomcat]]

sorry had to this out in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7231daefbb28 since one this changes caused bustages/test failures like

https://tbpl.mozilla.org/php/getParsedLog.php?id=43106706&tree=Mozilla-Inbound
and
https://tbpl.mozilla.org/php/getParsedLog.php?id=43107088&tree=Mozilla-Inbound

Comment 27

[Boris Zbarsky [:bzbarsky]]

Yeah, this is causing some of the oranges.  Filed bug 1034463 on that.

Comment 28

[Boris Zbarsky [:bzbarsky]]

And some of the failures are definitely due to SavedStacks not picking up the whole stack.  That's bug 1034477.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Looks like we need to make this property replaceable, because code out there (e.g. Task.jsm) will set .stack on exception objects.

Comment 30

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/ec821154d815
   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea43709c6675
   https://hg.mozilla.org/integration/mozilla-inbound/rev/842f6ec98e18
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f33a93527a75
   https://hg.mozilla.org/integration/mozilla-inbound/rev/5ac6aef80e07
   https://hg.mozilla.org/integration/mozilla-inbound/rev/2b6ec29ccdf9

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec821154d815
https://hg.mozilla.org/mozilla-central/rev/ea43709c6675
https://hg.mozilla.org/mozilla-central/rev/842f6ec98e18
https://hg.mozilla.org/mozilla-central/rev/f33a93527a75
https://hg.mozilla.org/mozilla-central/rev/5ac6aef80e07
https://hg.mozilla.org/mozilla-central/rev/2b6ec29ccdf9

Comment 32

[Jean-Yves Perrier [:teoli]]

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?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Thanks!  I fixed up the description to make it clear that this has the same behavior as .stack on built-in Error objects.