1325830 - Race in tabReady listeners for tabs not opened by extension

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Sören Hentzschel]

Attachment: test-addon-1.0.xpi

I developed a new version of SubToMe (https://addons.mozilla.org/de/firefox/addon/subtome-subscribe-button/) as WebExtension. Unfortunately a change in Firefox 52 broke the tabs API (browser.tabs.executeScript).

21:01.27 INFO: Last good revision: 0e13038823b5d200467b7a3bc8f2715c34964ac3
21:01.27 INFO: First bad revision: 314075c09505c28c056f8f44f37acfd28342edca
21:01.27 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0e13038823b5d200467b7a3bc8f2715c34964ac3&tochange=314075c09505c28c056f8f44f37acfd28342edca

21:02.82 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1301862

Simplified code example:

manifest.json:

{
  "manifest_version": 2,
  "name": "test-addon",
  "version": "1.0",
  "background": {
    "scripts": [
      "js/background.js"
    ]
  },
  "permissions": [
    "<all_urls>"
  ]
}

js/background.js:

const testaddon = {
  executeScript : function () {
    browser.tabs.executeScript(null, { file : 'js/testscript.js' });
  }
};

browser.tabs.onUpdated.addListener(testaddon.executeScript);

js/testscript.js:

console.error('testaddon debug output');

STR:

I attached the test add-on.

1. set xpinstall.signatures.required to false
2. install the add-on
3. open the browser console (you can filter for 'testaddon')
4. open a new tab tab and load a website, for example google.com
5. Have a look at the browser console
6. repeat step 4
7. repeat step 5

Expected result:

In step 5 you should see a debug output in the console.
In step 7 you should see a debug output in the console.

Actual result (Firefox 50 & Beta 51):

In step 5 you can see a debug output in the console.
In step 7 you can see a debug output in the console.

Actual result (Developer Edition 52 & Nightly 53):

In step 5 you can see a debug output in the console.
The debug output of step 7 is missing. In fact browser.tabs.executeScript only works once and is broken after the first time.

Comment 1

[Kris Maglione [:kmag]]

This works as expected for me, provided that I change 'js/testscript.js' to either 'testscript.js' or '/js/testscript.js'gg

Comment 2

[Sören Hentzschel]

Changing 'js/testscript.js' to either 'testscript.js' or '/js/testscript.js' does not work for me. :-/ (changed the code and file location locally and tested with web-ext run --firefox="/Applications/Firefox Nightly.app")

If important: Tested on macOS Sierra.

Comment 3

[Kris Maglione [:kmag]]

Sorry, I meant to remove that comment.

Comment 4

[Rob Wu [:robwu]]

I think that what happens is that when tabs.executeScript is called in tabs.onUpdated, that tab.linkedBrowser.innerWindowID in ext-tabs.js (http://searchfox.org/mozilla-central/rev/60785fce6d52289c2e147364863f9d42207c5f91/browser/components/extensions/ext-tabs.js#256) is falsey, preventing the promise from being resolved. Here is the relevant snippet:

  /**
   * Returns a promise that resolves when the tab is ready.
   * Tabs created via the `tabs.create` method are "ready" once the location
   * changed to the requested URL. Other tabs are always assumed to be ready.
   *
   * @param {XULElement} tab The <tab> element.
   * @returns {Promise} Resolves with the given tab once ready.
   */
  awaitTabReady(tab) {
    let deferred = this.tabReadyPromises.get(tab);
    if (!deferred) {
      deferred = PromiseUtils.defer();
      if (!this.initializingTabs.has(tab) && tab.linkedBrowser.innerWindowID) {
        deferred.resolve(tab);
      } else {
        this.tabReadyPromises.set(tab, deferred);
      }
    }

So if we call tabListener.initTabReady before storing deferred in the tabReadyPromises set, then this bug would be fixed.

I tested my hypothesis by calling chrome.tabs.create at least once, and then the scripts ran as expected.

I'll compile the latest version with a patch and test locally if it indeed fixes the bug, and if so, submit a patch.

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1325830 - initialize tabListener if needed

To verify, run the test in isolation:
mach mochitest browser/components/extensions/test/browser/browser_ext_tabs_executeScript_no_create.js

Without the fix, the test fails in e10s; with the fix, the test passes.

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

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8821871 [details]
Bug 1325830 - initialize tabListener if needed

https://reviewboard.mozilla.org/r/100982/#review104300

::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript_no_create.js:22
(Diff revision 1)
> +  function background() {
> +    // Using variables to prevent listeners from running more than once, instead
> +    // of removing the listener. This is to minimize any IPC, since the bug that
> +    // is being tested is sensitive to timing.
> +    let ignore = false;
> +    let url = "content of URL variable here";

No initial value, please.

::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript_no_create.js:29
(Diff revision 1)
> +      if (changeInfo.status === "loading" && tab.url === url && !ignore) {
> +        ignore = true;
> +        browser.tabs.executeScript(tabId, {
> +          code: "document.URL",
> +        })
> +        .then(results => {

Nit: Move to previous line.

::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript_no_create.js:32
(Diff revision 1)
> +          code: "document.URL",
> +        })
> +        .then(results => {
> +          browser.test.assertEq(url, results[0], "Content script should run");
> +          browser.test.notifyPass("executeScript-at-onUpdated");
> +        }, e => {

Nit: s/e/error/

Comment 7

[Rob Wu [:robwu]]

Comment on attachment 8821871 [details]
Bug 1325830 - initialize tabListener if needed

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

Comment 8

[Rob Wu [:robwu]]

Comment on attachment 8821871 [details]
Bug 1325830 - initialize tabListener if needed

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2138d8adcb4e
initialize tabListener if needed r=kmag

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2138d8adcb4e

Comment 11

[Rob Wu [:robwu]]

Comment on attachment 8821871 [details]
Bug 1325830 - initialize tabListener if needed

Approval Request Comment
[Feature/Bug causing the regression]: bug 1325830
[User impact if declined]: In Firefox 52, the following WebExtension APIs would malfunction (see bug for details): browser.tabs.captureVisibleTab, browser.tabs.detectLanguage, browser.tabs.executeScript, browser.tabs.insertCSS.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Not risky
[Why is the change risky/not risky?]: The change consists of adding one line that registers an event listener, plus unit tests.
[String changes made/needed]: N/A

Merge notes:
The unit test ([browser_ext_tabs_executeScript_no_create.js]) is registered in browser/components/extensions/test/browser/browser-common.ini
In the aurora branch, this should be browser/components/extensions/test/browser/browser.ini

Comment 12

[Julien Cristau [:jcristau] (back August 5)]

Comment on attachment 8821871 [details]
Bug 1325830 - initialize tabListener if needed

webextensions tab api fix, aurora52+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/628947537e85