Closed Bug 473156 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox :: General, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 3.1b3

People

(Reporter: v.obishchenko, Assigned: mfinkle)

References

Details

Attachments

(1 file)

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.
I think the logic should be:

function hasFilter(element) {
    return (element.event != aEvent) || (element.event == aEvent && element.listener != aListener);
}
Attached patch patchSplinter Review
* fixes the removal of events
* adds test specifically for this bug
* fixes some issues with other tests as fallout
Assignee: nobody → mark.finkle
Attachment #356775 - Flags: review?(dtownsend)
Severity: normal → major
Target Milestone: --- → Firefox 3.1b3
Flags: blocking-firefox3.1?
Is this a regression? Will it affect add-on compatibilty?
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3.1? → wanted-firefox3.1?
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!
Flags: wanted-firefox3.1? → wanted-firefox3.1+
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.
Attachment #356775 - Flags: review?(dtownsend) → review+
(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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Version: unspecified → Trunk
(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.
You need to log in before you can comment on or make changes to this bug.