Rob Wu [:robwu] Assignee
	Description • 8 years ago

1. Load addon.
2. Look at global console.
3. Visit example.com.
4. Look at example.com's console (in the tab).

Expected:
4x PASS

Actual:
"FAIL: Expected return value for chrome.runtime.sendMessage(message) to be undefined (in http://example.com/), got [object Promise]"test.js:6:9
"PASS: Expected return value for chrome.runtime.sendMessage(message, callback) to be undefined (in http://example.com/)"test.js:4:9
"FAIL: Expected return value for chrome.runtime.sendMessage(message) to be undefined (in moz-extension:...), got [object Promise]"test.js:6:9
"PASS: Expected return value for chrome.runtime.sendMessage(message, callback) to be undefined (in moz-extension:...)

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 2 • 8 years ago

I'm having trouble understanding what's going on here. Can you explain more what you think the correct behavior should be and also why what we're doing now is wrong?

	Rob Wu [:robwu] Assignee
	Comment 3 • 8 years ago

The chrome.runtime.sendMessage method must not return anything (because Chrome doesn't either). Currently we return a Promise if the response callback is not set, which should be undefined.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 4 • 8 years ago

How do we end up returning a promise?

	Rob Wu [:robwu] Assignee
	Comment 5 • 8 years ago

(In reply to Bill McCloskey (:billm) from comment #4)
> How do we end up returning a promise?

See the commit message (https://reviewboard.mozilla.org/r/71860/). Their schemas do not contain the flags "async" or "returns", so the function is treated as a function that does not return anything, and the generated function is handled by wrapper callFunctionNoReturn: http://searchfox.org/mozilla-central/rev/9ec085584d7491ddbaf6574d3732c08511709172/toolkit/components/extensions/Schemas.jsm#1182.

Because the schema expects no return value it should be OK to replace "return context.callFunctionNoReturn" with "context.callFunctionNoReturn" (the expected return value is always the undefined value).
But after doing that (in bug 1293132), some tests started to fail. And that's because sendMessage DOES actually return a value, namely a wrapped promise (=undefined if a callback is set, a Promise otherwise) http://searchfox.org/mozilla-central/rev/9ec085584d7491ddbaf6574d3732c08511709172/toolkit/components/extensions/ExtensionUtils.jsm#1287.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 6 • 8 years ago
mozreview-review

Comment on attachment 8781403 [details]
Bug 1295473 - Fix return type of {tabs,runtime}.sendMessage

https://reviewboard.mozilla.org/r/71860/#review70020

	Pulsebot
	Comment 7 • 8 years ago

Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c50cb95d1f42
Fix return type of {tabs,runtime}.sendMessage r=billm

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 8 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c50cb95d1f42

	Vasilica Tamas, QA [:vtamas]
	Comment 9 • 8 years ago

I was able to reproduce the initial issue on Firefox 51.0a1 (2016-08-15) under windows 1- 64-bit.

Tested on Firefox 51.0a1 (2016-09-06) under Windows 10 64-bit and I received the following logs:
PASS: Expected return value for chrome.runtime.sendMessage(message) to be undefined (in moz-extension://8dd85ef6-f8ce-47ac-8b90-c6c33bf2bf86/_generated_background_page.html)  test.js:4:9
PASS: Expected return value for chrome.runtime.sendMessage(message, callback) to be undefined (in moz-extension://8dd85ef6-f8ce-47ac-8b90-c6c33bf2bf86/_generated_background_page.html)  test.js:4:9
"FAIL: Expected return value for chrome.runtime.sendMessage(message) to be undefined (in http://example.com/), got [object Promise]"  test.js:6
"PASS: Expected return value for chrome.runtime.sendMessage(message, callback) to be undefined (in http://example.com/)"  test.js:4

One of them is still “FAIL”. Rob, any thoughts about this?

	Rob Wu [:robwu] Assignee
	Comment 10 • 8 years ago

I'm aware of it - it's tracked at bug 1296896.

	Vasilica Tamas, QA [:vtamas]
	Comment 11 • 8 years ago

(In reply to Rob Wu [:robwu] from comment #10)
> I'm aware of it - it's tracked at bug 1296896.
Great!

I am marking this bug as Verified since the other issue is tracked separately.