Closed Bug 1383310 Opened 5 years ago Closed 5 years ago

Devtools panel receives messages from content script but is unable to respond

Categories

(WebExtensions :: Developer Tools, defect, P1)

55 Branch
defect

Tracking

(firefox57 verified, firefox58 verified, firefox59 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 --- verified
firefox58 --- verified
firefox59 --- verified

People

(Reporter: manish.jethani, Assigned: rpl)

Details

(Whiteboard: triaged investigate)

Attachments

(3 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

Please use the attached example.zip to reproduce this issue.

Steps:

 1.  Load the extension
 2.  Open a new page (preferably without frames, e.g. manishjethani.com)
 3.  Open the web console
 4.  Click on the Example tab to view the devtools panel for the extension
 5.  Reload the page


Actual results:

The devtools panel (devtools-panel.js) receives a message from the content script in its runtime.onMessage listener. It sends a response, but the content script never receives it. Furthermore, if the runtime.onMessage listener in the devtools panel throws an error, the listener in the background page runs successfully, yet somehow the content script never receives the response.


Expected results:

Either the devtools panel should not receive the message from the content script (how it works in Chrome), or it should be able to respond successfully or throw an error without affecting the listener in the background page.
Summary: Devtools panel receives messages from content script → Devtools panel receives messages from content script but is unable to respond
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Luca could you comment and prioritise please?
Component: WebExtensions: Untriaged → WebExtensions: Developer Tools
Flags: needinfo?(lgreco)
The devtools panel should not receive messages directly from the content script, especially not on the runtime.onMessage listener.

I'll look into it (with the goal of filtering them out as expected)
Assignee: nobody → lgreco
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(lgreco)
Priority: -- → P1
Whiteboard: triaged investigate
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
Keywords: stale-bug
Attachment #8908171 - Flags: review?(tomica)
Comment on attachment 8908171 [details]
Bug 1383310 - Extensions Devtools panels should not receive messages or ports from content scripts.

https://reviewboard.mozilla.org/r/179850/#review185066

Looks mostly good, I just have a concern about extension pages.

::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:24
(Diff revision 1)
> +    browser.runtime.onMessage.addListener((msg, sender) => {
> +      browser.test.sendMessage("content_script_message_received");
> +    });
> +
>      browser.runtime.onConnect.addListener((port) => {
> +      if (port.sender.tab) {

I don't understand why this check?  And if there is a good reason, then why not in the listener above?

::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:58
(Diff revision 1)
> +
> +    browser.runtime.onMessage.addListener((msg, sender) => {
> +      // Fail if a content script message has been received by the devtools page (Bug 1383310).
> +      if (sender.tab) {
> +        browser.test.fail(`A DevTools page should not receive messages from content scripts`);
> +      }

Without thinking too much, since this is running in a different process from the background page, it might be theoretically possible the test could receive the `content_script_*_received` messages and close the whole test before this listener executes?  

Even if that's not possible, I think another round-trip could be useful to make the test easier to reason about.

::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:94
(Diff revision 1)
>        devtools_page: "devtools_page.html",
> +      content_scripts: [
> +        {
> +          js: ["content_script.js"],
> +          runt_at: "document_idle",
> +          matches: ["<all_urls>"],

nit: please restrict this to only the page you are expecting in the test.

::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:124
(Diff revision 1)
>  
> -  await extension.awaitFinish("devtools_page_connect.done");
> +  await extension.awaitMessage("devtools_page_connect.done");
> +
> +  // Send a message from the content script and expect it to be received from
> +  // the background page.
> +  extension.sendMessage("content_script.send_message");

does something guarantee the page is loaded (and the content script executed) when this is sent?

::: toolkit/components/extensions/ExtensionChild.jsm:358
(Diff revision 1)
>      this.optionalFilter = optionalFilter;
> +
> +    this.excludeContentScriptSender = false;
> +
> +    // Exclude messages coming from content scripts for the devtools extension contexts.
> +    if (this.context.envType === "devtools_child") {

nit: just assign the result to the property

::: toolkit/components/extensions/ExtensionChild.jsm:403
(Diff revision 1)
>        let listener = {
>          messageFilterPermissive: this.optionalFilter,
>          messageFilterStrict: this.filter,
>  
>          filterMessage: (sender, recipient) => {
> +          if (this.excludeContentScriptSender && sender.tab) {

Wont this also filter out messages from any extension pages opened in a tab?
Attachment #8908171 - Flags: review?(tomica)
Comment on attachment 8908171 [details]
Bug 1383310 - Extensions Devtools panels should not receive messages or ports from content scripts.

https://reviewboard.mozilla.org/r/179850/#review185066

> I don't understand why this check?  And if there is a good reason, then why not in the listener above?

This one is also used to ensure that the devtools page can connect a port to the background page and exchange messages using the port.

But I do agree that it would be better to test explicitly the sender in both the listeners.

Patch updated as suggested.

> Without thinking too much, since this is running in a different process from the background page, it might be theoretically possible the test could receive the `content_script_*_received` messages and close the whole test before this listener executes?  
> 
> Even if that's not possible, I think another round-trip could be useful to make the test easier to reason about.

The devtools_page and the background page are running in the same process, anyway I agree on the general point and so I've updated the test to send two messages and two ports to ensure that, if there is a regression related to this issue, the devtools page has the chance to receive them and turn the test into a failure as expected.

> nit: please restrict this to only the page you are expecting in the test.

done.

> does something guarantee the page is loaded (and the content script executed) when this is sent?

For the devtools page, it was implicit (we are waiting that the background page receives the port created by the devtools page), but I preferred to update the patch to make it more explicit as suggested (so that if the test fails for a timeout on one of the `await` statement, we can get more helpful information from the test failure logs to aid the investigation of the underlying issue).

> nit: just assign the result to the property

yeah, it looks nicer.

> Wont this also filter out messages from any extension pages opened in a tab?

you are definitely right, great catch!

The sender of an extension tab page contains the tab details, but the devtools page can reach them without using the tabs API, and so the devtools page should be able to receive messages and ports from an extension tab page.

I've changed the approach a bit: a much more explicit "isContentScript" property has now been added to the sender details of the content scripts (with the additional change applied to ExtensionContent.jsm in the updated patch) and in the Messenger class we can use this property to decide if we have to filter out the received message or port.

I've also added an additional test case that explicitly test that the devtools page can exchange messages and ports with an extension tab page (so that we can be much more confident that it doesn't regress).
Comment on attachment 8908171 [details]
Bug 1383310 - Extensions Devtools panels should not receive messages or ports from content scripts.

https://reviewboard.mozilla.org/r/179850/#review185758

::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:222
(Diff revisions 1 - 2)
> +    manifest: {
> +      devtools_page: "devtools_page.html",
> +    },
> +    files: {
> +      "extension_tab.html": `<!DOCTYPE html>
> +      <html>

nit: please indent the content

::: toolkit/components/extensions/ExtensionContent.jsm:509
(Diff revision 2)
> -  let sender = {id: this.extension.id, frameId: this.frameId, url: this.url};
> +  let sender = {
> +    id: this.extension.id, frameId: this.frameId, url: this.url,
> +    // Mark the sender as a contentScript, so that we can filter out
> +    // messages coming from a content scripts from the devools page and panels
> +    // (See Bug 1383310).
> +    isContentScript: true,

Might be better if `sender` included the `envType` instead.
Attachment #8908171 - Flags: review?(tomica) → review+
Comment on attachment 8908171 [details]
Bug 1383310 - Extensions Devtools panels should not receive messages or ports from content scripts.

https://reviewboard.mozilla.org/r/179850/#review185758

> Might be better if `sender` included the `envType` instead.

yeah, good call, this way the patch is also simpler, because the change can be completely implemented in the Messenger class defined in ExtensionChild.jsm, without any change needed in ExtensionContent.jsm.

Patch updated to include the context envType into the sender object.
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/2a168e11b817
Extensions Devtools panels should not receive messages or ports from content scripts. r=zombie
https://hg.mozilla.org/mozilla-central/rev/2a168e11b817
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
I am unable to reproduce the bug on 57.0a1 (20170905100117). When I install the ex extension I get a "Reading manifest: Error processing background.persistent: Event pages are not currently supported. This will run as a persistent background page." warning message. When I click on the "Example" button nothing is displayed in the console. Please provide updated steps or updated extenion or the “qe-verify-“ flag.
Flags: needinfo?(lgreco)
Attached image Bug1383310.png
I can reproduce this issue on Firefox 56.0.2 (20171024165158) under Wind 7 64-bit.

In the browser console a message from the devtools-panel.js is received:
Tab #1 says: Hello, background.js  devtools-panel.js:9

This issue is verified as fixed on Firefox 59.0a1(20171114100042), Firefox 58.0b3 (20171114032831) and Firefox 57.0 (20171112125346) under Wind 7 64-bit and Mac OS X 10.13. 

Please see the attached screenshot.
Flags: needinfo?(lgreco)
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.