Closed Bug 1309473 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: XPConnect, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file, 1 obsolete file)

+++ 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.
I'm assuming this isn't a regression so I'm going to set it as P3.
Priority: -- → P3
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.
that option will also make it possible to show stack trace to stderr only on xpcshell
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?
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #8801521 - Flags: feedback?(bobbyholley)
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?
Attachment #8801521 - Flags: feedback?(bobbyholley) → feedback-
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.
Changed to just set browser.dom.window.dump.enabled.
Attachment #8801521 - Attachment is obsolete: true
Attachment #8802799 - Flags: review?(bobbyholley)
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"
Attachment #8802799 - Flags: review?(bobbyholley) → review+
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
https://hg.mozilla.org/mozilla-central/rev/21d6deffe4cb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.