1438553 - nsContentUtils::ReportToConsole should include a stack, not just a location

Status: NEW

(Core :: DOM: Core & HTML, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Pretty simple to capture the stack here...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Include a stack, not just a location, when reporting to console via nsContentUtils::ReportToConsole

MozReview-Commit-ID: 611aFsbDvKJ

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Simple testcase that shows no stack without the patch, shows one with

Comment 3

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32ec8743c39bfda2dd420c6ed7890bfd86a56d2a

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[:Gijs (he/him)]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Osmose [:osmose, :mkelly]]

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

Comment 10

[:Gijs (he/him)]

(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?

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, I haven't really made progress on this. Fundamentally, we are going to need to think about the SpiderMonkey side of this and either change the stack representation there to not entrain things or change how we work with it in some way to clone it into a different data structure, or StackFrames from some throwaway global, or something.

And of course maybe we just need to retest and everything is fine now... Either way, this will need a new owner at this point.