1826290 - Add a public js::DumpBacktrace() variant that produces a string

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Sebastian Keller]

Steps to reproduce:

GNOME gjs logs error messages using the glib logging functions, which on systemd based systems directly talk to systemd/journald. Then it logs the corresponding trace via js::DumpBacktrace(cx, stderr). The systemd journal handles messages written to stderr at a different time than the corresponding error messages.

While this usually is not noticeable, this can lead to issues when gjs runs into situations that print errors + backtraces in some form of an endless loop. Then the backtraces might appear at a completely different location in the systemd journal, or in some cases apparently not at all.

This can be avoided by logging the error message and trace using the same glib logging functions, which requires the trace to be available as a string. For now gjs is going to use open_memstream() to generate a string using the existing js::DumpBacktrace(JSContext*, FILE*), but that is not available on all platforms and having js::DumpBacktrace() produce a string directly would be nicer anyway.

See https://gitlab.gnome.org/GNOME/gjs/-/merge_requests/832

Actual results:

The stack traces logged via js::DumpBacktrace(cx, stderr) appear separate from the corresponding gjs error message in the systemd journal.

Expected results:

If there was a js::DumpBacktrace() variant that produces a string instead of writing to a file, this could be used as part of the error message, rather than being logged separately.

Comment 1

[Nicolas B. Pierron [:nbp]]

Within SpiderMonkey we are using the GenericPrinter as a way to select either a File, or a string as a backend.
Would it be useful to provide a js::DumpBacktrace function which uses a GenericPrinter as a parameter?

This way you could avoid string allocation if this is not needed, or have different implementation logics.

Comment 2

[Sebastian Keller]

As far as I can tell GenericPrinter, Sprinter and js::DumpBacktrace(JSContext*, js::GenericPrinter&) are currently not public API and would need to be made public if we were to go that route. I'm not sure whether it would be better from the SpiderMonkey side to make all those public or to just have a js::DumpBacktrace() override that produces a string.

From the gjs side either would work. I don't think gjs would benefit from the flexibility that the GenericPrinter variant would provide, though. This is currently only used in places that just need a string and are not performance critical. However it would be good to get Philip's opinion on this too, because I'm not that familiar with gjs (or SpiderMonkey).

Comment 3

[Philip Chimento [:ptomato]]

From my side I agree either way would work. Currently, GJS wouldn't need the extra flexibility of GenericPrinter. (We only use the js::DumpBacktrace() API in situations where we may not be able to enter JSAPI. Otherwise, we use JS::FormatStackDump() anyhow.)

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1826290 - Move GenericPrinter and derivative to js/public/Printer.h.

This change moves the vm/Printer.h and classes to a public header, such that it
can be used by embedders when public function are using a GenericPrinter as
argument.

js::GenericPrinter provides a common interface for many internal functions, with
the intent of introspecting what is going on in SpiderMonkey. The reason they
are interesting for SpiderMonkey is that in many places, one might want to get
the information logged in a string (Sprinter), in a file (Fprinter) or in some
append-only-free-once buffer (LSprinter).

By implementing the debugging interface using a js::GenericPrinter, one can
target these various logging backends without looking at the implementation
details of each printer.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1826290 - Make DumpBacktrace with GenericPrinter argument public.

By providing this function, which is already used internally, embedders should
be able to produce a String output from the existing DumpBacktrace function as
follows:

JS::UniqueChars DumpBacktraceStr(JSContext* cx) {
  js::Sprinter sp(cx, false);
  js::DumpBacktrace(cx, sp);
  return sp.release();
}

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53614ecfb194
Move GenericPrinter and derivative to js/public/Printer.h. r=jandem
https://hg.mozilla.org/integration/autoland/rev/1feabc38ed2c
Make DumpBacktrace with GenericPrinter argument public. r=jandem
https://hg.mozilla.org/integration/autoland/rev/7df94cf5b708
apply code formatting via Lando

Comment 7

[Serban Stanca [:SerbanS]]

Backed out for causing build bustages in Disassembler-shared.h.

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/js/src/jit/shared/Disassembler-shared.h:23:21: error: variable has incomplete type 'class JS_PUBLIC_API'

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d85e933ffd60
Move GenericPrinter and derivative to js/public/Printer.h. r=jandem
https://hg.mozilla.org/integration/autoland/rev/35c42de01440
Make DumpBacktrace with GenericPrinter argument public. r=jandem

Comment 9

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/d85e933ffd60
https://hg.mozilla.org/mozilla-central/rev/35c42de01440

Comment 10

[Philip Chimento [:ptomato]]

This seems to be working great. Many thanks!