1237904 - Handling non-window-global error messages in nsIConsoleService

Status: NEW

(Core :: XPConnect, defect, P2)

Description

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Not all error messages are associated with a valid inner window ID, so the stack information cannot be printed.

See also bug 1208641 comment 11 and bug 1208641 comment 12.

Comment 1

[BMO Automation]

Firefox OS is not being worked on

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This applies throughout Gecko, not just Firefox OS.

Comment 3

[Brian Hackett [Laid off!]]

Are there STR for the problem here? If I open up the browser toolbox then console messages with stacks are shown for nsIScriptErrors that have no inner window ID.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I went over some of the referenced bugs and couldn't find any usable test case that would help me to reproduce this bug. I am closing it, so we don't waste our time.

Feel free to reopen if there is a test case we can use to see the issue on our machine!

Honza

Comment 5

[Alexandre Poirot [:ochameau]]

The missing stacks are for any exception being thrown from any context that isn't a document/window.

Here is a patch that highlights one exception without any stack:

diff --git a/devtools/shared/Loader.jsm b/devtools/shared/Loader.jsm
index d87adac28da0..5ee930d09115 100644
--- a/devtools/shared/Loader.jsm
+++ b/devtools/shared/Loader.jsm
@@ -243,3 +243,8 @@ Object.defineProperty(this.devtools, "Toolbox", {
 Object.defineProperty(this.devtools, "TargetFactory", {
   get: () => this.require("devtools/client/framework/target").TargetFactory,
 });
+
+let {setTimeout} = ChromeUtils.import("resource://gre/modules/Timer.jsm");
+setTimeout(function() {
+  throw new Error("foo"); // <== this error won't have stack in the browser console
+});

There is very useful context in bug 814497 and bug 1254230.

This issue is related to this very particular piece of code:
https://searchfox.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp#337-341
and:
https://searchfox.org/mozilla-central/source/xpcom/base/nsConsoleService.cpp#90-117

Storing messages in the console cache possibly leak the global from which they were emitted.
While we have a typical way to know about Document/Window destruction, it is less clear
if we have something in place to know when a Sandbox, a JSM, ... is destroyed.
To prevent leaks, we have to remove the related messages from the console cache once the related global is destroyed.
We currently only do that for the documents and so only save the stacks for these type of contexts.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks Alex for the pointers.

Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I tested this and I can nicely reproduce the issue:

1. Apply Alexe's patch
2. Build & run Firefox
3. Check the Browser Console
4. No stack trace for Error: foo -> BUG

Honza

---

I also tried to introduce some sync frames in my patch (they are not visible either in the stack):

diff --git a/devtools/shared/Loader.jsm b/devtools/shared/Loader.jsm
--- a/devtools/shared/Loader.jsm
+++ b/devtools/shared/Loader.jsm
@@ -238,8 +238,25 @@ this.require = this.devtools.require.bin

 // For compatibility reasons, expose these symbols on "devtools":
 Object.defineProperty(this.devtools, "Toolbox", {
   get: () => this.require("devtools/client/framework/toolbox").Toolbox,
 });
 Object.defineProperty(this.devtools, "TargetFactory", {
   get: () => this.require("devtools/client/framework/target").TargetFactory,
 });
+
+let {setTimeout} = ChromeUtils.import("resource://gre/modules/Timer.jsm");
+setTimeout(function() {
+  a();
+});
+
+function a() {
+  b();
+}
+
+function b() {
+  c();
+}
+
+function c() {
+  throw new Error("foo"); // <== this error won't have stack in the browser console
+}
\ No newline at end of file

Comment 8

[Alexandre Poirot [:ochameau]]

Tom, Would you happen to have fixed this long term issue in your recent work around stacks for promises? (bug 1595046 or bug 1636624)
It sounds likely as it looks like you are working around leaks, which were the main blocker for doing this bug...
...and... well, it works against a recent build :)

(There may be a few duplicates, like bug 1237904)

Comment 9

[Tom S [:evilpie]]

Ah yeah. I think that was actually unintentional and we might want to revert to the old behavior of not showing stacks. But as long as we don't get any leaks, maybe we are good?

I think most likely the behavior change was in this area: https://hg.mozilla.org/integration/autoland/rev/58bcc2778e01#l12.38
Before we would not keep the stack for mWindowID = 0. Now we probably call CreateScriptError and aWin is nullptr so we do keep the stack, because we can't get the WindowID.