844433 - Event cannot be reassigned using self.port.removeListener() and self.port.once() again

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Description

[imam.developers]

Test case: https://builder.addons.mozilla.org/package/175199/latest/

I removed a listener from an event by using self.port.removeListener() and then assigned a different listener to the same event using self.port.once(). When the event is raised, the OLD listener ist still called, plus an exception is thrown in the Error log.

Content Script: -----------------------------------

function listener1() {
    window.console.log("listener1 called!")
}

function listener2() {
    window.console.log("listener2 called!")
}

// Let listener1 handle the 'alert' event
self.port.once('alert', listener1);

// I changed my mind; better let listener2 handle it
self.port.removeListener(listener1);
self.port.once('alert', listener2);

Addon-Script: ---------------------------------------

require("page-mod").PageMod({
  include: "*",
  contentScriptFile: require("self").data.url("content.js"),
  onAttach: function(worker) {
  
    console.log("attached");
    worker.port.emit('alert');
  }
});

Result in console log: ----------------------------------

See screenshot: http://www.abload.de/img/addonbugl3qyj.png

Expected result: ----------------------------------------

- info: event-test-case: attached
- info: event-test-case: listener2 called!  // 2, not 1 !

---------------------------------------------------------

That means that removeListener() does not really remove the old listener, but caches it somewhere and reproduces it later when it shouldn't.

The behaviour is the same when there is user interaction, other event handling, and minutes of idling between the two assignments, so removeListener() should have had all the time to do its job.

Tested with Addon SDK 1.11 and 1.13.2 with FF17, FF18, and FF19.

Comment 1

[imam.developers]

PS: Could have something to do with bug 816272. It's not the same though, because in my example no event is already queued when the listener changes.

Comment 2

[imam.developers]

Seeing on the web examples for both, removeListener(callback) and removeListener(type, callback), I tried both, without effect.

However, having a look inside the code I think I found the reason:
-------- /lib/sdk/content/content-worker.js -------------
      once: function once(name, callback) {
        eventEmitter.on(name, function onceCallback() {  // CALLBACK IS WRAPPED!
          eventEmitter.removeListener(name, onceCallback);
          callback.apply(callback, arguments);
        });
      },
      removeListener: function removeListener(name, callback) {
        if (!(name in listeners))
          return;
        let index = listeners[name].indexOf(callback); // SEARCH FOR UNWRAPPED C.
        if (index == -1)
          return;
        listeners[name].splice(index, 1);
      }
    });
--------------------------------------------------------------

While in eventEmitter.on() the given callback function is directly added to the listeners[] array, in eventEmitter.once() it is wrapped into dynamically created onceCallback(). But later eventEmitter.removeListener() looks inf the array for the original callback, and therefore cannot find the ones created with once().

Comment 3

[charles.jourdan]

Attachment: patch

I purpoise a patch to fix the bug in adding an attribute to the onceCallback function and rewriting a function to get the function index in the listeners list.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Hi imam,

Thanks a lot for the report and a fix! This is great, but I think we should approach it little different to avoid overhead on removal of listeners. Luckily we can use `WeakMap`-s, to do that, instead of setting property onto `onceCallback` you can do `listener.set(callback, onceCallback)` and on removal do something like `callback = listeners.get(callback)` to get either `oneCallback` associated with it or use a `callback` itself.

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Oops sorry for confusion credit for a patch goes to Charles instead :)
Charles do you wanna try to adjust your patch as suggested in comment above ?

Comment 6

[charles.jourdan]

Sorry for the late. I think I understand that you want but how can you get all callbacks associated to the event `name` (onEvent function) ? We need get the value of each key but you can't get this list.

Comment 7

[charles.jourdan]

Attachment: patch

I post a new patch using WeakMap. I launch cfx test and all seems good. I hope this not break other part of the addon-sdk.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

Hey Irakli, are you working on this one? if not could you please unassign yourself.

Comment 9

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562