1309473 - xpcshell doesn't report uncaught exception in non-debug build

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P3)

Description

[Tooru Fujisawa [:arai]]

+++ This bug was initially created as a clone of Bug #1308947 +++

when evaluating file, or evaluating -e option in xpcshell, it doesn't show uncaught exception in non-debug build,
unless browser.dom.window.dump.enabled is set to true.

Comment 1

[Andrew Overholt [:overholt]]

I'm assuming this isn't a regression so I'm going to set it as P3.

Comment 2

[Tooru Fujisawa [:arai]]

The issue is here.

https://dxr.mozilla.org/mozilla-central/rev/7452437b3ab571b1d60aed4e973d82a1471f72b2/js/xpconnect/src/nsXPConnect.cpp#236
> void
> xpc::ErrorReport::LogToConsoleWithStack(JS::HandleObject aStack)
> {
>     // Log to stdout.
>     if (nsContentUtils::DOMWindowDumpEnabled()) {
>         nsAutoCString error;
> ...
>         fprintf(stderr, "%s\n", error.get());
>         fflush(stderr);
>     }

https://dxr.mozilla.org/mozilla-central/rev/7452437b3ab571b1d60aed4e973d82a1471f72b2/dom/base/nsContentUtils.cpp#7190
> bool
> nsContentUtils::DOMWindowDumpEnabled()
> {
> #if !(defined(DEBUG) || defined(MOZ_ENABLE_JS_DUMP))
>   // In optimized builds we check a pref that controls if we should
>   // enable output from dump() or not, in debug builds it's always
>   // enabled.
>   return nsContentUtils::sDOMWindowDumpEnabled;
> #else
>   return true;
> #endif
> }

It depends on nsContentUtils::sDOMWindowDumpEnabled on non-debug build.

Then, just changing nsContentUtils::sDOMWindowDumpEnabled (browser.dom.window.dump.enabled pref) to true won't be nice, as it will enable all other logging.
It would be nice if we can add some flag that gets true only on xpcshell, and use it too in xpc::ErrorReport::LogToConsoleWithStack.

Comment 3

[Tooru Fujisawa [:arai]]

that option will also make it possible to show stack trace to stderr only on xpcshell

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: WIP: Report uncaught error and stack trace in xpcshell non-debug build.

So, things to be done here are following:
  (A) print error to stderr on xpcshell non-debug build
  (B) print stack trace to stderr on xpcshell

for (A), we need to detect if the process is xpcshell, in AutoJSAPI or xpc::ErrorReport, and print error to stderr even if nsContentUtils::DOMWindowDumpEnabled() is false.

In this patch, added AutoJSAPI::mIsShell static member and AutoJSAPI::SetShell static method, and call it from XRE_XPCShellMain.
Also, added aAlwaysLogToStderr parameter to xpc::ErrorReport::LogToConsoleWithStack, and print error to stderr if it's true.
AutoJSAPI::ReportException passes AutoJSAPI::mIsShell to xpc::ErrorReport::LogToConsoleWithStack, so that the error will be printed to stderr on xpchsell, regardless of the value of nsContentUtils::DOMWindowDumpEnabled().

Also, for (B), after calling xpc::ErrorReport::LogToConsoleWithStack, added a code to print stack trace.
the operation needs JSContext, so I added to AutoJSAPI::ReportException, instead of xpc::ErrorReport::LogToConsoleWithStack.

Can I have some feedback?

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8801521 [details] [diff] [review]
WIP: Report uncaught error and stack trace in xpcshell non-debug build.

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

I'm not really wild about this approach, AutoJSAPI seems like the wrong places to store this.

Can we just set sDOMWindowDumpEnabled (via browser.dom.window.dump.enabled) from xpcshell instead?

Comment 6

[Tooru Fujisawa [:arai]]

Thank you for your feedback :)

apparently, my comment #2 about browser.dom.window.dump.enabled wasn't correct.
I don't see any other logging while testing locally, and it passed try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ede21f325983

will check a little more, and post it here.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Set browser.dom.window.dump.enabled to true in xpcshell to print error.

Changed to just set browser.dom.window.dump.enabled.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8802799 [details] [diff] [review]
Set browser.dom.window.dump.enabled to true in xpcshell to print error.

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

r=me if it does the right thing and passes try. Thanks for fixing this!

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +1426,5 @@
>              printf("NS_InitXPCOM2 failed!\n");
>              return 1;
>          }
>  
> +        // xpc::ErrorReport::LogToConsoleWithStack need this to print error to

nit: "needs this to print errors"

Comment 9

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21d6deffe4cb9eef85d80d2ec0c034ccedd3d7a9
Bug 1309473 - Set browser.dom.window.dump.enabled to true in xpcshell to print error. r=bholley

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21d6deffe4cb