Pretty simple to capture the stack here...

MozReview-Commit-ID: 611aFsbDvKJ

I'm curious what our plans are for these methods as we move away from nsIConsoleService.

This stuff could use all sorts of cleanup. I mostly did this because Gijs had a concrete need for something like this, but in general the current setup where we have at least two different stack representations and a whole mess of other complications is a bit silly. I am totally open to coming up with a better design here.

Comment on attachment 8951277 [details] [diff] [review] Include a stack, not just a location, when reporting to console via nsContentUtils::ReportToConsole This seems to cause leaks on dom/html/test/browser_bug1108547.js. I haven't figured out why yet.

We have a project underway using sentry to look for JS errors on nightly. It uses the console service to watch for nsIScriptError instances and tries to get a stack off that. We've noticed that a significant proportion of errors don't have a stack. Would this patch be likely to help those at all? It looks like it would... If so, would it be possible to chase this some more? I or Osmose may be able to try to help with getting this landed if the test issues are the major stumbling block here. Confusingly, I'm seeing missing stacks even for places where I'd expect stacks. E.g. https://sentry.prod.mozaws.net/operations/nightly-js-errors/issues/3483724/ is for https://dxr.mozilla.org/mozilla-central/rev/c23c7481957f7b982cffc0ce1d25979c69ca2c2f/devtools/shared/protocol.js#113 which does throw Error("undefined passed where a value is required"); which I would expect to have a stack (though it's hard to debug why, without having steps to reproduce the error or a stack that gives a clue as to that).

> Would this patch be likely to help those at all? Unclear without further data. I _think_ the test issue the the major stumbling block here. Specifically, I suspect the captured stack entrains a window (because spidermonkey's stack frame thing is JSObjects and they entrain their global), and the error is kept alive by the console service (because it has a buffer), so the upshot service is that the console service keeps stuff alive and the test detects this as a window leak. Now of course this can be a problem in real life too, not just in tests. The issue is that I'm not sure what to do about it. There are some possible plans for a non-global-entraining stack representation, last I talked to jimb, but they're not very concrete and have no timeline or resourcing attached afaict. > Confusingly, I'm seeing missing stacks even for places where I'd expect stacks I really don't know what's going on there, without knowing what sentry is doing. That said, it's possible that JS exception is being converted to an XPConnect exception and losing the stack in the process... if so, that would not be helped by this patch.

> I really don't know what's going on there, without knowing what sentry is doing. The code that pulls the stacktrace is here: https://searchfox.org/mozilla-central/source/browser/modules/BrowserErrorReporter.jsm#305-351 In short, we take the message as returned by the console service listener, call `QueryInterface(Ci.nsIScriptError)` on it, and check to see if it has a `.stack` attribute. The missing stacks in Sentry indicate that the stack attribute was falsy when we checked.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #8) > > Would this patch be likely to help those at all? > > Unclear without further data. OK. > I _think_ the test issue the the major stumbling block here. Specifically, > I suspect the captured stack entrains a window (because spidermonkey's stack > frame thing is JSObjects and they entrain their global), and the error is > kept alive by the console service (because it has a buffer), so the upshot > service is that the console service keeps stuff alive and the test detects > this as a window leak. > > Now of course this can be a problem in real life too, not just in tests. > The issue is that I'm not sure what to do about it. There are some possible > plans for a non-global-entraining stack representation, last I talked to > jimb, but they're not very concrete and have no timeline or resourcing > attached afaict. Uh, OK, interesting. Does this mean that if an exception/error gets thrown that *does* have a stackframe without this patch, we also leak that window? Because that sounds pretty serious, and in that case I'd kind of also expect us to see this type of leak much more frequently... Do they generate stackframes where the jsobjects for the stack frame has a different global or something?

> The missing stacks in Sentry indicate that the stack attribute was falsy when we checked. OK. So the thing is, a |new Error| is not an nsIScriptError, obviously. Something needs to produce an nsIScriptError or nsIConsoleMessage from it. The thing that does that is what needs to make sure to copy over the stack, as things stand. We could try to do some auditing of what could be messing that up, I guess... > Does this mean that if an exception/error gets thrown that *does* have a > stackframe without this patch, we also leak that window? For the time that the error message is queued in the console service, yes, afaict. > and in that case I'd kind of also expect us to see this type of leak much more frequently Well, it's not a leak through shutdown, and it gets cleared out by "minimize memory usage" in about:memory, so how would you see it? Also the queue has a max of 200 items, and on many pages just the CSS errors blow through that. Also also, I just dug a bit more and nsConsoleService::Observe should be clearing messages for the window when "inner-window-destroyed" is notified (which is when it gets unloaded, not when it gets actually destroyed). So maybe my hypothesis is just wrong... I'll try to make time to verify what's going on here. > Do they generate stackframes where the jsobjects for the stack frame has a different global or something? No.