403162 - [FIX]"Error: this._handleClick is not a function" closing Firefox tabs by clicking close button

Status: VERIFIED FIXED

(Core :: XBL, defect, P3)

Description

[Phil Ringnalda (:philor)]

Error: this._handleClick is not a function
Source File: chrome://global/content/bindings/button.xml
Line: 140

STR:
1. In a new profile (which should mean you have two tabs open, firstrun and default home page), go to about:config in one tab, and toggle javascript.options.showInConsole to true
2. Open the error console
3. Close the other tab by clicking the close button on the tab

Regression from bug 282180, which may have baked in SeaMonkey for 3 years, but apparently didn't consider things like http://mxr.mozilla.org/mozilla/source/browser/base/content/tabbrowser.xml#3010 (which I'm just guessing is the difference, and thus the source of the problem).

So far, I haven't spotted any overt problems from the error, but then I never was very good at reproducing the "double-click closes and opens" bug that the close-button handler hacks around.

Comment 1

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b2pre) Gecko/2007110905 Minefield/3.0b2pre] (nightly) (W2Ksp4)

Bug confirmed, per steps.

Oddly, if I use '-jsconsole' instead of step 2, I need to restart FireFox to get the error :-/

Comment 2

[[e-mail]]

I'm not sure whether this is a core bug, but I've written a workaround.

Basically tabbrowser-close-tab-button removes its binding parent from the DOM when it's clicked. This then tears down all its XBL methods. However its base binding's click handler still fires, and gets upset for obvious reasons.

Comment 3

[[e-mail]]

Attachment: Don't bother inheriting the binding

Comment 4

[Asaf Romano (gone)]

Comment on attachment 288144 [details] [diff] [review]
Don't bother inheriting the binding

r=mano.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So really, we shouldn't be firing the event handler here, right?  That would make the most sense to me.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So just undo what InstallEventHandlers does, I would think.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We have nsXBLBinding::UnhookEventHandlers, but it doesn't seem to be call everywhere where needed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible core patch

Does this fix it too?  The steps to reproduce don't reproduce for me, even without this patch....

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

Comment 10

[Phil Ringnalda (:philor)]

Comment on attachment 288249 [details] [diff] [review]
Possible core patch

Yep, that fixes it for me.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Neil, you ok if I move this to Core and take it, or should I file a separate bug on the core change?

Comment 12

[[e-mail]]

(In reply to comment #11)
>Neil, you ok if I move this to Core and take it, or should I file a separate
>bug on the core change?
If I'm not going to need my patch then feel free to move this to Core.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 288250 [details] [diff] [review]
Same as diff -w

This makes unhooking mirror hookup more closely, and calls it when we tear down style bindings.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 288250 [details] [diff] [review]
Same as diff -w

Does it really matter if we use target->RemoveGroupedEventListener or manager->RemoveEventListenerByType? I agree it's good to make install and unhook be similar so the change seems good. I'm mostly curious.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Does it really matter if we use target->RemoveGroupedEventListener or
> manager->RemoveEventListenerByType?

I have no idea!  So I figured I'd play it safe.  Maybe we should remove some of these APIs....  ;)

Checked the patch in.  Working on tests.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Checked in a test for this.

Comment 17

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b2pre) Gecko/2007111605 Minefield/3.0b2pre] (nightly) (W2Ksp4)

V.Fixed.