Closed Bug 1826290 Opened 2 years ago Closed 1 year ago

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

Categories

(Core :: JavaScript Engine, enhancement, P3)

Firefox 102
enhancement

Tracking

()

RESOLVED FIXED
115 Branch
Tracking Status
firefox115 --- fixed

People

(Reporter: sebastian-keller, Assigned: nbp)

References

Details

Attachments

(2 files)

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.

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.

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).

Blocks: sm-meta
Severity: -- → N/A
Priority: -- → P3

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.)

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.

Assignee: nobody → nicolas.b.pierron
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

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();
}
Pushed by npierron@mozilla.com: 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

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

Flags: needinfo?(nicolas.b.pierron)
Pushed by npierron@mozilla.com: 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
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch

This seems to be working great. Many thanks!

Flags: needinfo?(nicolas.b.pierron)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: