jonas Reporter
	Description • 11 years ago

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.

	(mostly gone) XtC4UaLL [:xtc4uall]
	Comment 1 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 11 years ago

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

	Nickolay_Ponomarev
	Comment 3 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 10 years ago

Er, Nick, see comment 5.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 7 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 10 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 10 • 10 years ago

(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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 10 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 12 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 10 years ago

Nick filed bug 1004110 to track lazy line number computation.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 10 years ago

For my future reference, performance testcases:

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 21 • 10 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 22 • 10 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 23 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 24 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 25 • 10 years ago

   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

	Carsten Book [:Tomcat]
	Comment 26 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 27 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 28 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 29 • 10 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 30 • 10 years ago

   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

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 10 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 32 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 33 • 10 years ago

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