Rob Wu [:robwu] Reporter
	Description • 6 years ago

I was rebased the patches for bug 1215376, and tests that previously passed started to fail unexpectedly.

I found that this is caused by the patch for bug 1215376. The patch replaces the EventManager for `test.onMessage` with the following implementation:

    class TestEventManager extends EventManager {
      addListener(callback, ...args) {
        super.addListener(function(...args) {
          try {
            callback.call(this, ...args);
          } catch (e) {
            assertTrue(false, `${e}\n${e.stack}`);
          }
        }, ...args);
      }
    }

This is problematic and demonstrated by the following test:

add_task(async function test_onMessage_removeListener() {
  let extension = ExtensionTestUtils.loadExtension({
    background() {
      let counter = 0;
      browser.test.onMessage.addListener(function listener(msg) {
        browser.test.assertEq(1, ++counter, "expected one event");
        browser.test.assertEq("first", msg, "expected message");
        browser.test.onMessage.removeListener(listener);
      });
      
      let countForever = 0;
      browser.test.onMessage.addListener(function() {
        if (++countForever === 2) {
          browser.test.notifyPass("received two messages");
        }
      });
    },
  });
  await extension.startup();
  extension.sendMessage("first");
  extension.sendMessage("second");
  await extension.awaitFinish("received two messages");
  await extension.unload();
});


Test results:
Entering test bound test_onMessage_removeListener
Extension loaded
TEST-PASS | path/to/test.js | expected one event - Expected: 1, Actual: 1 - 
TEST-PASS | path/to/test.js | expected message - Expected: first, Actual: first - 
TEST-UNEXPECTED-FAIL | path/to/test.js | expected one event - Expected: 1, Actual: 2 - 
Stack trace:
    listener@moz-extension://59b348b6-b670-41c0-a9a2-7fa5b526405a/%7B21310df8-0eda-4f0d-a6b0-c0b59512b321%7D.js:4:9
    
TEST-UNEXPECTED-FAIL | path/to/test.js | expected message - Expected: first, Actual: second - 
Stack trace:
    listener@moz-extension://59b348b6-b670-41c0-a9a2-7fa5b526405a/%7B21310df8-0eda-4f0d-a6b0-c0b59512b321%7D.js:5:9
    
TEST-PASS | path/to/test.js | received two messages - 
TEST-PASS | path/to/test.js | test result correct - 
Leaving test bound test_onMessage_removeListener


This clearly shows that the event listener was not removed by removeListener.
This is because the implementation of addListener in TestEventManager does not pass the original function to super.addListener, but a new function. A quick way to fix this is to store the event listeners in a Map or WeakMap, and override the removeListener/hasListener methods.

	Rob Wu [:robwu] Reporter
	Comment 1 • 6 years ago

s/I was rebased the patches f/I rebased/

The following patch fixes the issue (test in the initial bug report).
I wonder whether we should limit this behavior (notifying callers of failures) to test.onMessage only, instead of reporting a global error. Currently any errors in events are silently swallowed, which hurts extension developers because they may not notice errors in their code.



diff --git a/toolkit/components/extensions/ext-c-test.js b/toolkit/components/extensions/ext-c-test.js
index c728780775c4..da504847650d 100644
--- a/toolkit/components/extensions/ext-c-test.js
+++ b/toolkit/components/extensions/ext-c-test.js
@@ -86,14 +86,35 @@ this.test = class extends ExtensionAPI {
     }
 
     class TestEventManager extends EventManager {
+      constructor(...args) {
+        super(...args);
+        this.wrappedTestCallbacks = new Map();
+      }
+
       addListener(callback, ...args) {
-        super.addListener(function(...args) {
-          try {
-            callback.call(this, ...args);
-          } catch (e) {
-            assertTrue(false, `${e}\n${e.stack}`);
-          }
-        }, ...args);
+        if (!this.wrappedTestCallbacks.get(callback)) {
+          this.wrappedTestCallbacks.set(callback, function(...args) {
+            try {
+              callback.call(this, ...args);
+            } catch (e) {
+              assertTrue(false, `${e}\n${e.stack}`);
+            }
+          });
+        }
+        super.addListener(this.wrappedTestCallbacks.get(callback));
+      }
+
+      removeListener(callback) {
+        super.removeListener(this.wrappedTestCallbacks.get(callback));
+      }
+
+      hasListener(callback) {
+        return super.hasListener(this.wrappedTestCallbacks.get(callback));
+      }
+
+      close() {
+        super.close();
+        this.wrappedTestCallbacks.clear();
       }
     }