Closed Bug 1642671 Opened 1 month ago Closed 1 month ago

Add test to verify that falsey return values from runtime.onMessage are not ignored

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox79 fixed)

RESOLVED FIXED
mozilla79
Tracking Status
firefox79 --- fixed

People

(Reporter: robwu, Assigned: robwu)

Details

Attachments

(1 file)

This doesn't look right: https://hg.mozilla.org/mozilla-central/rev/ca3057f6cdc5#l3.50

A consequence of the value && value check is that falsey return values are ignored, and the caller will receive undefined due to the ultimate fallback.

I'll fix this and add some tests.

I was mistaken about the meaning of that line.

zombie clarified: that's not a bug, that listener returns an array of response values, or undefined if there are no listeners.

This bug is only about writing tests then, if we don't have them already.

No longer regressed by: 1583484
Summary: falsey return values from runtime.onMessage are ignored → Add test to verify that falsey return values from runtime.onMessage are not ignored

I patched MessageEvent.emit by triggering a failure when runtime.onMessage's callback returns a falsey value other than undefined. Surprisingly out of all extension tests, only one test failed, because its onMessage handler coincidentally received a null value. The caller didn't check the returned result though. So I have added a test to make sure that we have sufficient coverage for the scenario of receiving a falsey result in onMessage.

This is the patch that I used to run the check:

diff --git a/toolkit/components/extensions/ExtensionChild.jsm b/toolkit/components/extensions/ExtensionChild.jsm
index a9e8a2c576576..14a1b2462f579 100644
--- a/toolkit/components/extensions/ExtensionChild.jsm
+++ b/toolkit/components/extensions/ExtensionChild.jsm
@@ -181,6 +181,17 @@ class MessageEvent extends SimpleEventAPI {
       .map(fire => this.wrapResponse(fire, message, sender))
       .filter(x => x !== undefined);
 
+    const abort = (msg) => {
+      (this.context.chromeObj || this.context.cloneScope.wrappedJSObject.browser).test.fail(msg);
+    };
+    all.forEach(x => {
+      if (x && x instanceof Promise) {
+        x.then(v => {
+          v || v === undefined || abort("FALSEY??? " + v);
+        }, () => {});
+      }
+    });
+
     return all.length ? Promise.race(all).then(v => [v]) : [];
   }
 

Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/36f295b440e6
Add tests for falsey result from runtime.onMessage r=zombie,geckoview-reviewers,agi
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
You need to log in before you can comment on or make changes to this bug.