53929 - console service fails to UnregisterListener

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[John Bandhauer]

nsConsoleService::RegisterListener builds a proxy around the listener and saves 
a pointer to that proxy. nsConsoleService::UnregisterListener tries to find and 
remove the *raw* listener. Since the raw listener is never saved it is never 
found and removed. It is thus leaked (along with its window) and later attempts 
to call it fail. The fix is to use the proxy service to get the proxy in 
UnregisterListener just like in RegisterListener. Since the proxy service caches 
live proxies it will do the right lookup and find the same proxy for the same 
listener.

This bug is a real annoying impedement to using the JS console in a debug build. 
If one opens and then *ever* closes the console then you will hit an assert in 
xpconnect for each and every JS error or warning (and we now have lots of JS 
warnings). The assert fires because the (leaked!) listener holds a reference to 
the (leaked!) console window global object. But that global object has had its 
properties cleared so has no 'Components' object. The call to the observer via 
xpconnect needs to be able to find the Components object in order to build a 
wrapper for the param of the observer. When it fails to find that Components 
object it asserts.

This is ugly and is easily fixed with the patch I will attach. I've tested the 
patch and no xpconnect objects are leaked and no asserts are fired.

Comment 1

[John Bandhauer]

Attachment: proposed fix

Comment 2

[Mike McCabe]

Thanks for finding this!  It's been bugging me for a long time now... of course
the fix had to be obvious and simple.

r=mccabe, with one change.  The entire UnregisterListener function is currently
nsAutoLock'ed, but I think that only the RemoveElement call needs to be.  I'd
put the lock and the RemoveElement call in a block.

Also, a style question - GetProxyForListener is implemented as a private member
function.  Is there a reason to prefer a member function over a static function?

Is this worth fixing in the release?

Comment 3

[John Bandhauer]

I agree on the lock.

On file static vs. private member... I don't think it matters much. Especially 
when no instance data is used. Choose whatever you like.

I'm sure jar would say this is not critical. On the other hand, it is a real 
pain for those people building the product and trying to see JS errors. Still, 
the "don't close the console" workaround is obvious. I'd say get it on the trunk 
as soon as that becomes possible again. The branch is probably off limits 
forever for stuff like this.

Please make this your own. I'll be happy to review any tweaks.

Comment 4

[Mike McCabe]

Attachment: New tweak of fix with locking change

Comment 5

[John Bandhauer]

r=jband

Comment 6

[John Bandhauer]

hey, let's get this checked into the trunk.

Comment 7

[Mike McCabe]

Fix checked in to trunk.