GNOME Bugzilla – Bug 649384
context: add a callback to control gjs_log_exception() behavior
Last modified: 2018-01-27 11:47:41 UTC
Add gjs_context_set_exception_logger(), and update gjs_log_exception() to use the callback set there. The default implementation is exactly the same as it was before.
Created attachment 187203 [details] [review] context: add a callback to control gjs_log_exception() behavior
Review of attachment 187203 [details] [review]: A few comments ::: gjs/jsapi-util.c @@ +1016,3 @@ + } + + if (!gjs_string_to_utf8(context, STRING_TO_JSVAL(s), &message)) { I'd prefer we use gjs_value_debug_string here. The value of it is that if you have say binary array data in your exception message string, we will still try to print it rather than overwriting the exception and making things more confusing. Except...heh, that function calls gjs_log_exception. Would need to be fixed. @@ +1026,3 @@ + if (exception_logger) { + exception_logger(js_context); I presume you have a gnome-shell patch for this. But if all it's doing is stringifying the exception and printing it to stderr - really, we should just be passing it as a UTF-8 string to the callback, and not have to do too much JSAPI stuff in there. @@ +1031,3 @@ + "Exception was: %s", + message); + gjs_log_exception_props(context, exc); Actually, this is already recursive because gjs_log_exception_props() -> gjs_log_object_props() -> gjs_value_debug_string().
(In reply to comment #2) > I presume you have a gnome-shell patch for this. bug 649385 > But if all it's doing is > stringifying the exception and printing it to stderr - really, we should just > be passing it as a UTF-8 string to the callback, and not have to do too much > JSAPI stuff in there. Yeah, I thought about that... I end up stringifying it slightly differently though. (It mostly uses gjs methods though, with only a little raw jsapi.) I had also thought about passing the exception to the logger directly, but then we'd have to have jsval exposed in the gjs API. (You can't just pass a gpointer and say it's a JSObject*, because you can throw non-objects...)
(In reply to comment #3) > (In reply to comment #2) > > I presume you have a gnome-shell patch for this. > > bug 649385 > > > But if all it's doing is > > stringifying the exception and printing it to stderr - really, we should just > > be passing it as a UTF-8 string to the callback, and not have to do too much > > JSAPI stuff in there. > > Yeah, I thought about that... I end up stringifying it slightly differently > though. (It mostly uses gjs methods though, with only a little raw jsapi.) After looking at the g-s patch, why not pull it down into gjs and have a combined callback which handles uncaught exceptions and the stuff from JS_SetErrorReporter?
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > I presume you have a gnome-shell patch for this. > > > > bug 649385 > > > > > But if all it's doing is > > > stringifying the exception and printing it to stderr - really, we should just > > > be passing it as a UTF-8 string to the callback, and not have to do too much > > > JSAPI stuff in there. > > > > Yeah, I thought about that... I end up stringifying it slightly differently > > though. (It mostly uses gjs methods though, with only a little raw jsapi.) > > After looking at the g-s patch, why not pull it down into gjs and have a > combined callback which handles uncaught exceptions and the stuff from > JS_SetErrorReporter? A way to look at it is that it's lame we're just blowing away gjs' default gjs_error_reporter. I'd expect that thrown exceptions and the like would be handled by /usr/bin/gjs too in a uniform way.
I guess I was assuming I should preserve the current behavior as much as possible for non-gnome-shell users... but yeah, an integrated logging function makes sense. Although then I'm going to want gjs_debug() in too... Is there any reason the stuff in util/log.c does not currently go through g_log? It seems like we want: gjs_fatal() should wrap g_error() gjs_debug() should wrap g_debug() gjs_log() should wrap g_message() rather than calling gjs_debug(GJS_DEBUG_LOG, ...), and the GJS_DEBUG_LOG enum value should go away Likewise, gjs_debug(GJS_DEBUG_ERROR, ...) should turn into gjs_warning(), which should wrap g_warning(), and GJS_DEBUG_ERROR should go away (This then fixes the "gnome-shell needs to set environment variables at startup to tell gjs what to log" bug, since log() would be going through g_message() and errors would go through g_warning(), and everything else that goes through gjs_debug() actually is debugging stuff that we don't normally want to see.) gjs_log_exception() should just call JS_ReportUncaughtException(), which will send the exception through the error reporter. Or maybe we should just not set JSOPTION_DONT_REPORT_UNCAUGHT? (context.c seems to be claiming that it's not possible to call JS_GetPendingException() from inside an error reporter, but this does not seem to be true from looking at the code.) At any rate, removing JSOPTION_DONT_REPORT_UNCAUGHT would probably require changes to error handling throughout the code... For the error reporter itself, the reason my gnome-shell patch called JS_SetErrorReporter() instead of adding a gjs wrapper for it is that the error reporter gets a JSErrorReport struct from jsapi.h, which would have been awkward to pass in a GjsContext callback. So we'd need to do something with that. Possibilities include: #include <jsapi.h> in context.h; put a forward declaration for JSErrorReport (and JSContext?) in context.h; make a GjsErrorReport type; decide which JSErrorReport fields are useful and then pass them explicitly.
> context.c seems to be claiming that it's not > possible to call JS_GetPendingException() from inside > an error reporter If you mean this comment: /* JSOPTION_DONT_REPORT_UNCAUGHT: Don't send exceptions to our * error report handler; instead leave them set. This allows us * to get at the exception object. */ I _think_ that means if you report uncaught, you can't use JS_GetPendingException() (i.e. the "uncaught reporter" handles and clears the exception) Presumably if you change the flag you'll see what breaks, anyhow.
Right, that's what I was assuming it meant, but the end of JS_ReportUncaughtException is: /* Pass the exception object. */ JS_SetPendingException(cx, exn); js_ReportErrorAgain(cx, bytes, reportp); JS_ClearPendingException(cx); Maybe that wasn't there when gjs was first written.
(In reply to comment #6) > I guess I was assuming I should preserve the current behavior as much as > possible for non-gnome-shell users... but yeah, an integrated logging function > makes sense. Although then I'm going to want gjs_debug() in too... > > Is there any reason the stuff in util/log.c does not currently go through > g_log? It seems like we want: > > gjs_fatal() should wrap g_error() > > gjs_debug() should wrap g_debug() > > gjs_log() should wrap g_message() rather than calling > gjs_debug(GJS_DEBUG_LOG, ...), and the GJS_DEBUG_LOG enum value > should go away There is no gjs_log(). Also for gjs_debug, note that it's used a *lot*, and in the header says: /* These defines are because we have some pretty expensive and * extremely verbose debug output in certain areas, that's useful * sometimes, but just too much to compile in by default. */ It's probably worth at least looking at the performance hit were we to always call them. > For the error reporter itself, the reason my gnome-shell patch called > JS_SetErrorReporter() instead of adding a gjs wrapper for it is that the error > reporter gets a JSErrorReport struct from jsapi.h, which would have been > awkward to pass in a GjsContext callback. So we'd need to do something with > that. Possibilities include: #include <jsapi.h> in context.h; No, I explicitly wanted to avoid that. > make a > GjsErrorReport type; decide which JSErrorReport fields are useful and then pass > them explicitly. Prefer this.
I don't think the comment is about calling GetPendingException _inside_ an error reporter, I think it's about calling GetPendingException _outside_ an error reporter. Maybe that's the confusion. I don't know if gjs still needs to do this; it may have been used only in the old multi-context setup. I don't remember how Colin and Owen ended up fixing that so maybe it isn't relevant anymore.
Created attachment 192187 [details] [review] log: remove GJS_DEBUG_STRACE_TIMESTAMPS Nothing is currently using it, and it behaves completely differently from everything else in gjs_debug(), so if it were going to be added back, it should be a separate "gjs_debug_timestamp()" function or something.
Created attachment 192188 [details] [review] log: route debug/error output through g_log Make gjs_fatal() just a wrapper around g_error(), gjs_debug(GJS_DEBUG_ERROR...) a wrapper around g_warning, and gjs_debug(anything else...) a wrapper around g_debug(). Remove the log file and timestamping support; if an application wants these, it can implement them by redirecting the g_log output. Add a g_log handler to console.c to make the output look like it used to.
Created attachment 192189 [details] [review] Push exception handling into gjs_error_reporter Make gjs_log_exception() call JS_ReportPendingException() to send the exception to the error handler, and log it from there.
Created attachment 192190 [details] [review] context: add gjs_context_set_error_handler() Allow overriding the JS error handler for a context.
Review of attachment 192187 [details] [review]: Committing this one, and rejecting the other patches; those should be rewritten to use structured logging.
Review of attachment 192188 [details] [review]: See previous comment.
Review of attachment 192189 [details] [review]: Ditto.
Review of attachment 192190 [details] [review]: Ditto.
Comment on attachment 192187 [details] [review] log: remove GJS_DEBUG_STRACE_TIMESTAMPS Attachment 192187 [details] pushed as 2da18da - log: remove GJS_DEBUG_STRACE_TIMESTAMPS
(In reply to Philip Chimento from comment #15) > Committing this one, and rejecting the other patches; those should be > rewritten to use structured logging. Note that I [the original reporter] haven't touched gjs in years and don't work on anything that uses it any more, and also the gnome-shell bug that depends on these changes (bug 649385) has sat idle for 6 years. So if you don't care about these changes, then you might as well just close the bug.
(In reply to Dan Winship from comment #20) > (In reply to Philip Chimento from comment #15) > > Committing this one, and rejecting the other patches; those should be > > rewritten to use structured logging. > > Note that I [the original reporter] haven't touched gjs in years and don't > work on anything that uses it any more, and also the gnome-shell bug that > depends on these changes (bug 649385) has sat idle for 6 years. So if you > don't care about these changes, then you might as well just close the bug. I don't see why we wouldn't do this, when we basically get it a lot of it for free with structured logging, so I'll keep it open; I wanted to switch to structured logging anyway, to clean up the debug topic stuff a bit.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gjs/issues/58.