356599 - browser.tabs.closeButtons pref observer may cause ###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Nickolay_Ponomarev]

Split from bug 345529. The tabbrowser-tabs' _prefObserver in tabbrowser.xml may outlive the window it is in (since it's cleaned up by GC, which is not guaranteed to run right after the window is closed).

The binding doesn't remove the observer manually in the destructor, relying on the fact the observer is registered via a weak reference. So if you change the observed preference, browser.tabs.closeButtons, soon after closing a browser window, the observer will be called and you'll get this assertion:

###!!! ASSERTION: XPConnect is being called on a scope without a 'Components'
property!

This is pretty much always bad. It usually means that native code is
making a callback to an interface implemented in JavaScript, but the
document where the JS object was created has already been cleared and the
global properties of that document's window are *gone*. Generally this
indicates a problem that should be addressed in the design and use of the
callback code.
: 'Error', file
c:/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativesco
pe.cpp, line 587

Comment 1

[Nickolay_Ponomarev]

Attachment: patch

Switch to manually removing the observer and clean up a few stylistic oddities.

Comment 2

[Adam Guthrie]

See bug 339477 and bug 340547. The problem is that the the removeObserver call will never get called since the <destructor> is never called, causing a leak.

In the other bug we chocked this up to "XBL sucks". Maybe you could figure out why the dtor isn't getting called...

Comment 3

[Nickolay_Ponomarev]

The destructor is called, you can check it yourself. The problem is as described in comment 0.

Anyway, the destructor not being called is a pretty serious problem, which should have been filed as a bug and mentioned in a XXX comment in tabbrowser's <destructor> for other people to know. Saying "XBL sucks" somewhere in bugzilla comment and trying to wallpaper over the bug instead is utterly wrong.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Are you sure this patch doesn't regress bug 339477?

Are you sure that there's no other code that tries to hold a weak reference to this object? If not, you should probably leave it's QI intact.

Comment 5

[Nickolay_Ponomarev]

(In reply to comment #4)
> Are you sure this patch doesn't regress bug 339477?
> 
At least on windows, the destructor does get called and the observer is removed. Otherwise the patch wouldn't have fixed the assertion. I'll check linux later today.

> Are you sure that there's no other code that tries to hold a weak reference to
> this object? If not, you should probably leave it's QI intact.
> 
Yes. http://lxr.mozilla.org/seamonkey/search?string=_prefObserver plus the fact that the QI was added in bug 340547, followup to bug 339477.

Comment 6

[Nickolay_Ponomarev]

(In reply to comment #5)
> (In reply to comment #4)
> > Are you sure this patch doesn't regress bug 339477?
> > 
> At least on windows, the destructor does get called and the observer is
> removed. Otherwise the patch wouldn't have fixed the assertion. I'll check
> linux later today.
> 
Works fine on linux too. ispiked, can you confirm this change doesn't regress 339477?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 242216 [details] [diff] [review]
patch

(In reply to comment #5)
> (In reply to comment #4)
> plus the fact that the QI was added in bug 340547, followup to bug 339477.

Doh, I was looking at the wrong QI in blame :(

Comment 8

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/content/widgets/tabbrowser.xml 	1.209

Comment 9

[Adam Guthrie]

I verified that this didn't regress bug 339477. Marking as VERIFIED based on Nickolay's explanation in comment 0.

Is this fix something that's branch-worthy? That assertion sounds pretty bad. Could this ever cause a crash?

Comment 10

[Nickolay_Ponomarev]

I never heard of this assertion leading to a crash, but in this particular case this patch will wallpaper the crash in bug 345529. That crash shouldn't be very common, but it's still a crash.

Not sure what the policy on post-Fx2 branches is. Feel free to request branch approval if you think it's ok.