I got an exception from one of the devtools components. It displayed in the browser toolbox as a string, with a link just to the immediate reporter, DevToolsUtils.js:reportException. I'll attach a screenshot. It would be more useful if the stack trace were displayed in a way that enabled clicking on each individual frame.

Bah, I spent all day working on a clever patch that would treat console.error(exception) specially, and then I realized that instead of deviating from the behavior of the console API (ad-hoc) standard, we should fix our bugs instead. The reason we serialize the stack in DevToolsUtils.reportException() and call Cu.reportError(message) is that Cu.reportError(exception) creates an nsIScriptError, which unfortunately doesn't retain the stack trace (bug 814497). See nsXPCComponents_Utils::ReportError for the details. Alternatively, we could introduce a special console method to log the stack trace from a provided error object, instead of the stack at the call site, like bug 639800 suggests. However, introducing a Firefox-only console.logException(msg, exception), seems equally bad to treating console.error(exception) differently for display purposes. I' setting a dependency on bug 814497 instead of duping, to ensure we fix this once we get platform support for doing so.

Here is a patch to ensure creating nsScriptErrorWithStack objects from Cu.reportError. This is the first usage of nsScriptErrorWithStack with stack objects being possibly from a compartment other than a document/window. It means that we possibly leak the related context from the console service messages cache. I'm not sure that's a big deal? (it will be freed when the cache will be pruned or overflown with new messages) I don't think we have any leak checks against contexts that are not document/windows. Try run will educate me about that.

Comment on attachment 8667165 [details] [diff] [review] patch v1 Tom, I'm especially interested in any feedback you may have regarding C++/jsapi here... (I still feel very weak when it comes to writing c++) And could you confirm your initial usecase gets correct stacks with this patch? (there is *many* other chrome codepath that don't get stacks, like the deprecated_rejected_but_not_catched_promise_report.)

Comment on attachment 8667165 [details] [diff] [review] patch v1 Review of attachment 8667165 [details] [diff] [review]: ----------------------------------------------------------------- I'm afraid I don't have that many good comments. I mostly have questions about things I don't know myself :-) Also there are a bunch of debugging prints left in there, but I didn't mention them specifically in the comments. Finally, I don't currently have an easy way to reproduce my original problem. How are you reproducing? I could give that a try. ::: js/xpconnect/src/XPCComponents.cpp @@ +2324,5 @@ > + nsCOMPtr<nsIScriptError> scripterr; > + > + if (errorObj) { > + printf("ErrorObj\n"); > + JSAutoCompartment ac(cx, errorObj); Is this needed? I don't understand why but I'd like to know if possible. @@ +2365,5 @@ > + printf("!scripterr 2\n"); > + scripterr = new nsScriptError(); > + } > + > + if (!scripterr) I thought "new" was infallible (generally), and if so this branch can't be taken. However, the new code uses "new" while the old code was using do_CreateInstance; and I don't know when you'd prefer one over the other.

Comment on attachment 8667165 [details] [diff] [review] patch v1 Clearing the flag this time.

(In reply to Tom Tromey :tromey from comment #5) > Comment on attachment 8667165 [details] [diff] [review] > > Finally, I don't currently have an easy way to reproduce my original > problem. How are you reproducing? I could give that a try. From the Browser Console: Cu.reportError('plop'); Cu.reportError(new Error('plop')); // For the "if (err) / It's a proper JS Error" codepath Or adding such instructions from any js file from /devtools/ > > ::: js/xpconnect/src/XPCComponents.cpp > @@ +2324,5 @@ > > + nsCOMPtr<nsIScriptError> scripterr; > > + > > + if (errorObj) { > > + printf("ErrorObj\n"); > > + JSAutoCompartment ac(cx, errorObj); > > Is this needed? I don't understand why but I'd like to know if possible. Copy paste. Actually I don't really know exactly either. jsapi/xpconnect is still made of dark matter to me. But looks like it works without the JSAutoCompartment. Also we should be using another API now, AutoJSAPI... > > @@ +2365,5 @@ > > + printf("!scripterr 2\n"); > > + scripterr = new nsScriptError(); > > + } > > + > > + if (!scripterr) > > I thought "new" was infallible (generally), and if so this branch can't be > taken. Ah! See? You know stuff I don't! > > However, the new code uses "new" while the old code was using > do_CreateInstance; and I don't know when you'd prefer one over the other. I have no idea either. I'm using "new" as I have to instanciate a nsScriptErrorWithStack instance, which, AFAIK, can't be created via CreateInstance. Then I'm also using "new" for nsScriptError.

I finally tried the patch and it works nicely for me. Thanks for doing this.

Rebased.

Comment on attachment 8714723 [details] [diff] [review] patch v3 Review of attachment 8714723 [details] [diff] [review]: ----------------------------------------------------------------- Bobby, Feel free to forward the review to whoever is best! In this patch I'm moving the call to getcurrentstack earlier, that to create a nsScriptErrorWithStack instead of just nsScriptError.

Comment on attachment 8714725 [details] [diff] [review] test v1 Review of attachment 8714725 [details] [diff] [review]: ----------------------------------------------------------------- And a test for this.

Comment on attachment 8714723 [details] [diff] [review] patch v3 Review of attachment 8714723 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that. ::: js/xpconnect/src/XPCComponents.cpp @@ +2316,5 @@ > + > + if (!scripterr) { > + nsCOMPtr<nsIStackFrame> frame; > + nsXPConnect* xpc = nsXPConnect::XPConnect(); > + xpc->GetCurrentJSStack(getter_AddRefs(frame)); Can you call dom::GetCurrentJSStack directly here? @@ +2322,5 @@ > + frame->GetFilename(fileName); > + frame->GetLineNumber(&lineNo); > + JS::Rooted<JS::Value> stack(cx); > + nsresult rv = frame->GetNativeSavedFrame(&stack); > + NS_ENSURE_SUCCESS(rv, NS_OK); I'd think you want NS_ERROR_FAILURE, since early-returning will cause this error to not be reported at all. If we actually care about recovering in this situation, you should remove the NS_ENSURE, and just check the failure code along with |stack.isObject()| and fall through to the |new nsScriptError| case.

Use dom::GetCurrentJSStack and stop using NS_ENSURE_SUCCESS.

Tweaked the patch to use a distinct, dedicated js file, so that line number we assert in test are stable.

https://hg.mozilla.org/integration/fx-team/rev/a5daf170c09a2cb138a3edcba915e7b2037cbeae Bug 1141222 - Create ScriptError with stack from Cu.reportError. r=bholley https://hg.mozilla.org/integration/fx-team/rev/a88ca50a3ed51f8a7592bae17c7cfc16c51b33d9 Bug 1141222 - Test Cu.reportError stack in browser console. r=vporof

https://hg.mozilla.org/mozilla-central/rev/a5daf170c09a https://hg.mozilla.org/mozilla-central/rev/a88ca50a3ed5