473156 - FUEL: fuelIEvents.removeListener removes all listeners for an event.

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Vitalii Obishchenko]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.5) Gecko/2008120121 Firefox/3.0.5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.5) Gecko/2008120121 Firefox/3.0.5

fuelIEvents.removeListener removes all listeners for an event. 
For example, if i add two different listeners of same event, and then try to remove one of them, second listener will be removed too.

Reproducible: Always

Steps to Reproduce:
I wrote example code:

var listener1 = {
    handleEvent: function(event) {
	Application.console.log('1: ' + event.type + ' - ' + event.data);
    }
};
var listener2 = {
    handleEvent: function(event) {
	Application.console.log('2: ' + event.type + ' - ' + event.data);
    }
};

Application.prefs.events.addListener('change', listener1);
Application.prefs.setValue('test_pref', 'true');
Application.prefs.events.addListener('change', listener2);
Application.prefs.setValue('test_pref', 'false');
Application.prefs.events.removeListener('change', listener1);
Application.prefs.setValue('test_pref', 'true');
Application.prefs.events.removeListener('change', listener2);
Actual Results:  
After running this code I see in console three lines:

1: change - test_pref
 ----------
1: change - test_pref
 ----------
2: change - test_pref

Expected Results:  
But I expected to see four lines, last from listener 2:

1: change - test_pref
 ----------
1: change - test_pref
 ----------
2: change - test_pref
 ----------
2: change - test_pref

I think problem is in filter function in removeListener method.
This function:

function hasFilter(element) {
    return element.event != aEvent && element.listener != aListener;
}

in fuelApplication.js will be return false for all listeners of aEvent.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

I think the logic should be:

function hasFilter(element) {
    return (element.event != aEvent) || (element.event == aEvent && element.listener != aListener);
}

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

* fixes the removal of events
* adds test specifically for this bug
* fixes some issues with other tests as fallout

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Is this a regression? Will it affect add-on compatibilty?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

It is not a regression - it was always been broken. It should not affect add-on compat. If an add-on was using this in a way that manifested the bug, the add-on would be broken somehow. So I imagine add-ons would have avoided the bug using some other workarounds.

OK, maybe not a "blocker", be definitely "wanted". It's an accident waiting to happen for any poor extension or Ubiquity developer. Do I have a shot in Hell of getting a wanted-1.9.1 in?

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

You definitely have a shot in hell. Your shot improves with:

 - getting Mossop to review your patch
 - getting your patch baked on trunk
 - including tests!

Comment 6

[Dave Townsend [:mossop]]

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

>-      return element.event != aEvent && element.listener != aListener;
>+      return (element.event != aEvent) || (element.event == aEvent && element.listener != aListener);

Actually just |return (element.event != aEvent) || (element.listener != aListener)|
What you have there already does that, but unless the js interpreter is clever it will be slightly less efficient.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #6)
> (From update of attachment 356775 [details] [diff] [review])
> >-      return element.event != aEvent && element.listener != aListener;
> >+      return (element.event != aEvent) || (element.event == aEvent && element.listener != aListener);
> 
> Actually just |return (element.event != aEvent) || (element.listener !=
> aListener)|

done

http://hg.mozilla.org/mozilla-central/rev/209cf95bda41

Comment 8

[[e-mail]]

(In reply to comment #6)
>(From update of attachment 356775 [details] [diff] [review])
>>-      return element.event != aEvent && element.listener != aListener;
>>+      return (element.event != aEvent) || (element.event == aEvent && element.listener != aListener);
>Actually just return (element.event != aEvent) || (element.listener != aListener)
>What you have there already does that, but unless the js interpreter is clever
>it will be slightly less efficient.
Or in fact just s/&&/||/ was the original bug.