1254230 - Console service can hold windows alive via stacks if exceptions are thrown late enough

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

The code added in bug 814497 has one slight problem: if an exception is thrown _after_ FreeInnerObjects has happened on the window (which tries to clear out things from the console service), you end up with the console message sitting around in the console service queue for arbitrary amounts of time, holding the window alive via the stack.  Yet again, the stack representation comes back to bite us.

We should probably not actually send a stack along if the window has had FreeInnerObjects called, or something...

But better would be to send along a stack in some format that doesn't entail keeping the entire window alive just because we need a stack.  That would also help us send along stacks for non-window cases, as in bug 1237904.

Note that the hueyfix does NOT help here because we're holding a direct reference to the exception object, not a cross-compartment wrapper for one.

Comment 1

[Jim Blandy :jimb]

Sorry, which part of the stack entrains the window, exactly? Is it simply that the SavedFrame objects have the window as their global?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yes, precisely.  So it can't go away, and then the code starting at https://hg.mozilla.org/mozilla-central/file/b6acf4d4fc20431a8ae14bf32cdc6e43a9c0f9ad/testing/mochitest/browser-test.js#l640 finds those objects are still alive and treats them as leaks, which is what caught this for me in tests.  Outside of tests, the leak is "until enough other things get sent to the console service".

Comment 3

[Boris Zbarsky [:bzbarsky]]

And note that for test purposes the notifications sent at https://hg.mozilla.org/mozilla-central/file/b6acf4d4fc20431a8ae14bf32cdc6e43a9c0f9ad/testing/mochitest/ShutdownLeaksCollector.jsm#l34 could be used to clear the console service buffer, or just stacks from things in it.  But that wouldn't help the non-test situation.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, so this is seriously blocking me landing bug 1254393; all sorts of browser chrome tests randomly hit .  I'm going to have to do the "not actually send a stack along if the window has had FreeInnerObjects called" thing, I guess.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: kinda-fix. Make sure to never send script errors with stacks attached to the console service if the associated windows have already had FreeInnerObjects called on them

Comment 6

[Jim Blandy :jimb]

What's dumb here is that that global is in no way necessary for the stacks to serve their purpose.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yes, indeed...

Comment 8

[:Gijs (he/him)]

Apologies for the dumb question but, is this going to break JS stacks for exceptions that fall into this category? Because that would be sad...

Comment 9

[Boris Zbarsky [:bzbarsky]]

> is this going to break JS stacks for exceptions that fall into this category?

It (being my patch) will stop showing them in the console in the nice way added in bug 814497 for exceptions that are thrown after a window is navigated away from or closed.  If you catch the exception, it's .stack will still be there, of course.

I agree that this is sad.  I think we should come up with some way of communicating this stack that does not involve SavedFrame objects; see comment 0...  I just don't have time to do that myself right now.  :(

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8727709 [details] [diff] [review]
kinda-fix.  Make sure to never send script errors with stacks attached to the console service if the associated windows have already had FreeInnerObjects called on them

Review of attachment 8727709 [details] [diff] [review]:
-----------------------------------------------------------------

Repeating this hack in 3 places sucks. Can we make FindExceptionStack do this check instead?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Repeating this hack in 3 places sucks. Can we make FindExceptionStack do this check instead?

We can, as long as we then realize it's no longer a general-purpose utility.  I'm probably OK with that if you are.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> > Repeating this hack in 3 places sucks. Can we make FindExceptionStack do this check instead?
> 
> We can, as long as we then realize it's no longer a general-purpose utility.
> I'm probably OK with that if you are.

Sure. Should probably rename it to reflect that.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: kinda-fix. Make sure to never send script errors with stacks attached to the console service if the associated windows have already had FreeInnerObjects called on them

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6622bc6dd8d1

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6622bc6dd8d1