User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.50 Safari/537.36 Steps to reproduce: Using chrome.runtime.sendMessage send a message to the background script and respond in the background script Actual results: The callback in the popup was not called Expected results: The callback in the popup should be called

See https://github.com/markus-hartung-avira/web-ext-popup-messaging-bug for minimal example It is reproducable in FF 51.0a2 (2016-10-06) (32-bit) developer edition, but not in the regular FF 49 version.

[Tracking Requested - why for this release]: This is a serious regression that was introduced in Firefox 51

Tracking 52+ for the reason noted in Comment 2.

This is caused by the fact that the popup is initially a preloaded browser. 1) A quick work-around is to route the message through the process manager instead of the frame message manager. Originally, responses went through the process manager as well, but I changed it since frame message managers make more sense and no tests failed: https://hg.mozilla.org/mozilla-central/rev/329102a4c68241bc15f4cd775d9f269e588a91f9#l7.9 2) Another solution is to track browser swaps, e.g. as done in https://reviewboard.mozilla.org/r/78662/diff/4 3) Another option is to queue extension API invocations until the popup browser is swapped (the API is asynchronous, so this shouldn't be a problem, right?). Which of the options do you prefer (short-term for backporting, and long-term)?

We need to handle browser swapping correctly so we don't lose resposnes from tab pages either. Do you mind if I take this bug? I have a working fix that I needed to write so I could test it properly.

Not at all - thanks for fixing it.

Track 51+ as regression.

Comment on attachment 8799977 [details] Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped. https://reviewboard.mozilla.org/r/85014/#review83888 ::: browser/components/extensions/test/browser/browser_ext_popup_sendMessage.js:25 (Diff revision 1) > > + files: { > + "popup.html": scriptPage("popup.js"), > + "popup.js": function() { > browser.runtime.onMessage.addListener(msg => { > - if (msg == "close-popup") { > + if (msg == "popup-ping") { Turn this into a `browser.test.assertEq`. We are not expecting any other messages (recall that sendMessage is not supposed to dispatch onMessage in the same frame). Similarly, for `onMessage` in the background script. ::: browser/components/extensions/test/browser/browser_ext_popup_sendMessage.js:52 (Diff revision 1) > -}); > + }); > > -add_task(function* testBrowserActionInPanel() { > - yield testInArea(CustomizableUI.AREA_PANEL); > + return new Promise(resolve => { > + // Wait long enough that we're relatively sure the docShells have > + // been swapped. > + setTimeout(resolve, 250); Where is the 250 coming from? It seems to be chosen such that it is "more than POPUP_LOAD_TIMEOUT_MS" from ext-utils.js. If that is indeed the case, please add a comment in case the value of that arbitrarily chosen constant ever changes. ::: browser/components/extensions/test/browser/browser_ext_popup_sendMessage.js:81 (Diff revision 1) > > - EventUtils.synthesizeMouseAtCenter(widget.node, {type: "mouseup", button: 0}, window); > + { > + clickPageAction(extension); > > - yield mouseUpPromise; > + let pong = yield extension.awaitMessage("popup-pong"); > + is(pong, "background-pong", "Got pong"); The use of the same string for two independent test paths is a bit confusing. Please rename one of them, e.g. this one to "background-pong-reply". Similarly for popup-pong -> popup-pong-reply. ::: toolkit/components/extensions/MessageChannel.jsm:147 (Diff revision 1) > + * destroyed. > + */ > + dispose() { > + if (this.eventTarget) { > + this.removeListeners(this.eventTarget); > + this.eventTarget = null; Move this line inside `removeListeners`. ::: toolkit/components/extensions/MessageChannel.jsm:149 (Diff revision 1) > + dispose() { > + if (this.eventTarget) { > + this.removeListeners(this.eventTarget); > + this.eventTarget = null; > + } else { > + this.messageManager = null; This is read-only (but configurable). Change it to `delete this.messageManager;` ::: toolkit/components/extensions/MessageChannel.jsm:213 (Diff revision 1) > + * Removes docShell swap listeners to the message manager owner. > + * > + * @param {Element} target > + * The target element. > + */ > + removeListeners(target) { Remove the in-parameter and use this.eventTarget directly. It is always the same anyway, and then it is obvious that it does not accept arbitrary objects. ::: toolkit/components/extensions/MessageChannel.jsm:778 (Diff revision 1) > } > } > } > > target.sendAsyncMessage(MESSAGE_RESPONSE, response); > + }).then(() => { FYI: The above calls to `target.sendAsyncMessage` may throw. Then this `dispose` call will never happen.

Kris, is there an action based on the review above?

Yes, but I've been bogged down dealing with critical uplifts for the past couple of weeks.

Comment on attachment 8799977 [details] Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped. https://reviewboard.mozilla.org/r/85014/#review88936 Looks good to me. Would a test with an options page be useful? (you know the internals here much better than I do, but I think in that case there isn't a docshell swap so that test would confirm this all still works in that case) ::: toolkit/components/extensions/MessageChannel.jsm:116 (Diff revision 2) > XPCOMUtils.defineLazyModuleGetter(this, "PromiseUtils", > "resource://gre/modules/PromiseUtils.jsm"); > XPCOMUtils.defineLazyModuleGetter(this, "Task", > "resource://gre/modules/Task.jsm"); > +/** > + * Acts as a proxy for a message manager or message manager owner, and A clarification that the proxy is only for sending messages and not for receiving would be helpful (it becomes obvious from the code and the use below, but for somebody starting reading with this class like I did, its a useful bit of information)

Comment on attachment 8799977 [details] Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped. https://reviewboard.mozilla.org/r/85014/#review88936 It couldn't hurt, I guess, but options pages shouldn't be affected by this particular issue. > A clarification that the proxy is only for sending messages and not for receiving would be helpful (it becomes obvious from the code and the use below, but for somebody starting reading with this class like I did, its a useful bit of information) Yeah, I guess. I was considering extending it at some point, since there are other places where we could benefit from this helper.

Comment on attachment 8799977 [details] Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped. https://reviewboard.mozilla.org/r/85014/#review88936 > Yeah, I guess. I was considering extending it at some point, since there are other places where we could benefit from this helper. Cool, the comment can be updated then along with those extensions.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd43923701cd09a0630be439b19d43fa745ff36 Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped. r=aswan

Hi :kmag, Do you think this is worth uplifting to 51 aurora?

Comment on attachment 8799977 [details] Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped. Approval Request Comment [Feature/regressing bug #]: Bug 1259093 [User impact if declined]: This causes some extensions which rely on receiving replies to messages sent from their browserAction popups shortly after opening to fail in unpredictable ways. [Describe test coverage new/current, TreeHerder]: The related features are covered by extensive tests, and new tests have been added for this issue. [Risks and why]: Low. This change simply tracks docshell swaps so that response messages are sent to the correct message manager. If we were likely to see issues from it, they would almost certainly have been seen by now. [String/UUID change made/needed]: None.

Comment on attachment 8799977 [details] Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped. Fix a webextension issue related to message handling. Take it in 51 aurora.

I was able to reproduce the initial issue on Firefox 51.0a2 (2016-11-07) under Windows 10. Manually verified as fixed on Firefox 52.0a1 (2016-11-07) under Windows 10 64-bit and Ubuntu 16.04 32-bit. The callback is successfully called in the popup: http://screencast.com/t/N5p0Tk5K

Confirm that this bug is also fixed on Firefox 51.0a2 (2016-11-08) under Windows 10 64-bit and Ubuntu 16.04 32-bit.