1394841 - WebExtensions - Messaging is not working in responsive mode

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect, P2)

Description

[ido.y]

Attachment: responsive_design_test_extension.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.101 Safari/537.36

Steps to reproduce:

In responsive mode,
Listen to message in content script
And send a message from background

Happens also in nightly version


Actual results:

Nothing


Expected results:

Should handle message

Comment 1

[Kris Maglione [:kmag]]

This is because the page loaded into a tab in responsive design mode is a developer tools page (which the extension is not allowed to access), with the actual web content loaded into a remote subframe.

I'm not really sure whether we should try to handle this corner case. It affects a lot of things other than messaging.

Comment 2

[ido.y]

Hi Kris,
We rely on this functionality, and we have cross-browser solution which is working on Chrome
Is there a simple way to solve this issue?

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Similar bugs have also been filed for legacy-style add-ons back in the day, such as bug 1320159.

I think it would be nice to have it working for a consistent experience, but not a major issue in the scheme of things.

I think I'll move this to RDM component, because RDM is really the cause and potentially where a fix might be made.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Seems to also be an issue for DevTools extensions like React:

https://github.com/facebook/react-devtools/issues/904

I think I'll investigate a bit to see how much work would be needed here.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

It looks like only a small number of changes are needed to support this, so I'll give it a try.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1394841 - Check iframes for related tabs in WebExt.

When using WebExt messaging with Responsive Design Mode, message manager
messages traveling from content to parent such as "MessageChannel:Messages" are
received with `event.target` set to the <iframe mozbrowser> inside RDM that
hosts the content.

Here we change `getSender` processing for messages like this to also check HTML
iframes for related tabs, in addition to XUL elements as before.

Review commit: https://reviewboard.mozilla.org/r/195582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195582/

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1394841 - Add WebExt messaging to RDM tunnel.

WebExt messaging sends several new message types to content that should be
tunneled by RDM.

This change allows them to reach content as expected in RDM mode.

Review commit: https://reviewboard.mozilla.org/r/195584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195584/

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1394841 - Test WebExt messaging with RDM.

This test verifies that WebExt messaging works as expected for both the
background page scripts and content scripts when RDM is used.

Review commit: https://reviewboard.mozilla.org/r/195586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195586/

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=525259d7b76d21cc45e1515e73e859e18258d73f

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8924342 [details]
Bug 1394841 - Test WebExt messaging with RDM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195586/diff/1-2/

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09eb8b575e3fdfe97014e9185ffb1913ff3dd988

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8924341 [details]
Bug 1394841 - Add WebExt messaging to RDM tunnel.

https://reviewboard.mozilla.org/r/195584/#review201798

Thanks for looking into that!

Comment 15

[Alexandre Poirot [:ochameau]]

Comment on attachment 8924342 [details]
Bug 1394841 - Test WebExt messaging with RDM.

https://reviewboard.mozilla.org/r/195586/#review201796

::: devtools/client/responsive.html/test/browser/browser_ext_messaging.js:100
(Diff revision 2)
> +          let { id } = sender;
> +          respond({ id });
> +        });
> +
> +        await new Promise(resolve => {
> +          browser.test.onMessage.addListener(resolve);

Are you waiting for "resume" message?
It may be helpful to comment about that here.

::: devtools/client/responsive.html/test/browser/browser_ext_messaging.js:120
(Diff revision 2)
> +    extension.awaitMessage("content-script-ready"),
> +    extension.awaitMessage("background-script-ready"),
> +  ]);
> +  extension.sendMessage("resume");
> +
> +  await extension.awaitFinish("rdm-messaging");

nit: for both extension.startup() and extension.sendMessage(resume) we do the action first and listen right after. This is the typical pattern to get intermittent/races.
I imagine this is unlikely here.
But for example it would make the wait for content-script-ready and background-script-ready easier. 
If you wait for them before startup you can then do:
  await extension.startup()
  await onContentScriptReady;
  await onBackgroundScriptReady;

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8924342 [details]
Bug 1394841 - Test WebExt messaging with RDM.

https://reviewboard.mozilla.org/r/195586/#review202584

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8924340 [details]
Bug 1394841 - Check iframes for related tabs in WebExt.

https://reviewboard.mozilla.org/r/195582/#review202588

::: browser/components/extensions/ext-browser.js:38
(Diff revision 1)
> -  } else if (target instanceof Ci.nsIDOMXULElement) {
> +  } else if (target instanceof Ci.nsIDOMXULElement ||
> +             ExtensionUtils.instanceOf(target, "HTMLIFrameElement")) {

How does this work? getBrowserData only knows how to handle <browser>s that are a child of <tabbrowser> in a browser window, or belong to about:addons.

Does the RDM UI have a stub gBrowser global, or something like that?

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8924340 [details]
Bug 1394841 - Check iframes for related tabs in WebExt.

https://reviewboard.mozilla.org/r/195582/#review202588

> How does this work? getBrowserData only knows how to handle <browser>s that are a child of <tabbrowser> in a browser window, or belong to about:addons.
> 
> Does the RDM UI have a stub gBrowser global, or something like that?

Yes, the RDM UI defines a usable `gBrowser` for call sites like this that want to look up the tab for the browser frame, so that's what allows this to work here as well.

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8924340 [details]
Bug 1394841 - Check iframes for related tabs in WebExt.

https://reviewboard.mozilla.org/r/195582/#review203018

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8924342 [details]
Bug 1394841 - Test WebExt messaging with RDM.

https://reviewboard.mozilla.org/r/195586/#review201796

> Are you waiting for "resume" message?
> It may be helpful to comment about that here.

Ah right, I'll add a comment, thanks!

> nit: for both extension.startup() and extension.sendMessage(resume) we do the action first and listen right after. This is the typical pattern to get intermittent/races.
> I imagine this is unlikely here.
> But for example it would make the wait for content-script-ready and background-script-ready easier. 
> If you wait for them before startup you can then do:
>   await extension.startup()
>   await onContentScriptReady;
>   await onBackgroundScriptReady;

The extension isn't installed until `startup`, so that one should be safe.  Anyway, I agree the other listeners should be moved earlier.  (I am usually watching out for this anyway, but I guess I missed it here.)  Thanks for catching this.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8924340 [details]
Bug 1394841 - Check iframes for related tabs in WebExt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195582/diff/1-2/

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8924341 [details]
Bug 1394841 - Add WebExt messaging to RDM tunnel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195584/diff/1-2/

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8924342 [details]
Bug 1394841 - Test WebExt messaging with RDM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195586/diff/2-3/

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82a140b3aa646fb0bef0674a257206cb312aeb6e

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4aa421a32de9
Check iframes for related tabs in WebExt. r=kmag
https://hg.mozilla.org/integration/autoland/rev/134b46ce0aa5
Add WebExt messaging to RDM tunnel. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/bbf3fb8a107c
Test WebExt messaging with RDM. r=kmag,ochameau

Comment 26

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4aa421a32de9
https://hg.mozilla.org/mozilla-central/rev/134b46ce0aa5
https://hg.mozilla.org/mozilla-central/rev/bbf3fb8a107c