1286908 - ExtensionTestUtils.loadExtension should not require an id when useAddonManager is true

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

It seems that there is a requirement to pass an id as the second argument to ExtensionTestUtils.loadExtension when you are also including the useAddonManager: true in the options passed into the first argument.

I hit this issue when trying to write a test for a new management API method and it took awhile to figure out what the problem was (aswan eventually figured it out). No error is thrown, but things don't quite work as expected if you do not pass the id into the second argument.

It would be nice to remove this limitation.

Comment 1

[Kris Maglione [:kmag]]

This is meant to work. If you don't pass an ID, one is automatically generated.

What are the exact differences between when you pass an ID and when you don't? What ID are you passing?

Comment 2

[Andrew Swan [:aswan]]

The specific problem is that the underlying SpecialPowers code that gets invoked creates an ID if one isn't provided:
https://dxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#1924-1927

But then the actual Extension object that gets created gets a different ID (either from the manifest or something assigned by the AddonManager).  The result is that the startup promise here never resolves since it is not looking for the right ID:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#1196

Comment 3

[Bob Silverberg [:bsilverberg]]

The ID I am using is `[e-mail]`. I am specifying it in the manifest via:

    applications: {
      gecko: {
        id: id,
        update_url: "https://updates.mozilla.com/",
      },
    },

If I do not pass the same ID as a second argument to ExtensionTestUtils.loadExtension then something goes wrong which seems to prevent the test from communicating with the extension. A reduced test case can be seen at https://gist.github.com/bobsilverberg/4503bb4d2436803d6ef4def943ee646c. In that simple example, with `useAddonManager: true` I never see the final message "-------- startup complete... ----------" (the test times out), whereas with `useAddonManager: false` I do see that message. 

A more functional example had me sending a message from the background script back to the test, via `browser.test.sendMessage`. With `useAddonManager: true` the message is never received by the test (and the test times out), whereas with `useAddonManager: false` the message is received.

As well, if I keep `useAddonManager: true` but also pass in the id as the second argument to ExtensionTestUtils.loadExtension, then everything works, so that seemed to be the solution, but you are suggesting that shouldn't be required.

Comment 4

[Kris Maglione [:kmag]]

Yes. If you specify an ID in the manifest, you need to specify the same ID when you call loadExtension. If you call loadExtension with an ID, though, it will automatically be added to the manifest.

Maybe we should add an assertion to check for that.

Comment 5

[Andrew Swan [:aswan]]

Is there a good reason to not have loadExtension pull the id from the manifest rather than making the caller provide it twice?

I think there's still the issue that this doesn't work if useAddonManager is true and there's no ID in the manifest (i.e., the AddonManager assigns the ID).  I think in that case, we could have the test wrappers just inject an ID into the manifest so the startup event can be correlated with the right extension instance?

Comment 6

[Kris Maglione [:kmag]]

There's no need to provide it twice. If there's no ID in the manifest, we add it. We could pull the existing ID from the manifest if there is one, I suppose, but we already need the logic that adds the ID to the manifest for the case where we generate it, so I'd rather not also do it the other way around if we can avoid it.

Comment 7

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #6)
> There's no need to provide it twice. If there's no ID in the manifest, we
> add it. We could pull the existing ID from the manifest if there is one, I
> suppose, but we already need the logic that adds the ID to the manifest for
> the case where we generate it, so I'd rather not also do it the other way
> around if we can avoid it.

Your argument is sensible but from a user-of-the-api perspective, it is confusing and surprising that this function that takes the manifest as an argument actually requires that the ID be passed in a separate argument and that it fails in a non-obvious way if you neglect to do so.

Comment 8

[Andrew Swan [:aswan]]

Attachment: Bug 1286908 Part 1: refactor existing webextensions browser tests

These tests all assume that an extension id will be allocated as part
of loadExtension().  To pave the way for removing the code that allocates
ids from special powers, adapt these tests so that they don't reference
the extension's id until after the extension has been started.

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

Comment 9

[Andrew Swan [:aswan]]

Attachment: Bug 1286908 Remove id allocation from SpecialPowers loadExtension()

Prior to this change, SpecialPowers used the extension id to identiy
extension instances in inter-process messaging.  This required that
an id be allocated from the content process side when loadExtension()
was called, but that made it impossible to test code that exercises the
code path in the AddonManager that allocates ids for extensions that do
not include an id in the manifest (it also made the loadExtension() api
clunky).

With this change, SpecialPowers allocates an internal identifier for
messaging, but this identifier is separate from extension ids.
Confusingly, we still store the actual extension id in an id property
on the object returned by loadExtension(), but there are enough tests
that reference this that it would be unnecessarily disruptive to get
rid of it so it stays for now...

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

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8777143 [details]
Bug 1286908 Part 1: refactor existing webextensions browser tests

https://reviewboard.mozilla.org/r/68756/#review66802

::: browser/components/extensions/test/browser/browser_ext_incognito_popup.js:109
(Diff revision 1)
> -  yield extension.startup();
> +  yield extension.awaitMessage("ready");
> +  extension.sendMessage("go");

Why are these changes necessary?

::: browser/components/extensions/test/browser/browser_ext_pageAction_context.js:112
(Diff revision 1)
>      }
>    }
>  
>    let testNewWindows = 1;
>  
> -  let awaitFinish = new Promise(resolve => {
> +  extension.sendMessage("runTests");

Why is this change necessary?

::: browser/components/extensions/test/browser/browser_ext_pageAction_popup.js:172
(Diff revision 1)
> +    if (expecting.expectClosed) {
> +      break;
> +    }

This doesn't seem right at all. Why can't this stay an onMessage listener?

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8777144 [details]
Bug 1286908 Remove id allocation from SpecialPowers loadExtension()

https://reviewboard.mozilla.org/r/68758/#review66806

::: browser/components/extensions/test/browser/browser_ext_runtime_openOptionsPage.js:6
(Diff revision 1)
>  /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
>  /* vim: set sts=2 sw=2 et tw=80: */
>  "use strict";
>  
>  function* loadExtension(options) {
> +  const ID = "[e-mail]";

We shouldn't use the same ID for every run of this.

::: testing/specialpowers/content/specialpowersAPI.js:1924
(Diff revision 1)
> -      let uuidGenerator = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
> -      id = uuidGenerator.generateUUID().number;
> -    }
> +    // Note, this is not the addon is as used by the AddonManager etc,
> +    // this is just an identifier used for specialpowers messaging
> +    // between this content process and the chrome process.
> +    let id = this._extensionCount++;

Please give this a name that makes its purpose clearer.

::: toolkit/components/extensions/Extension.jsm:62
(Diff revision 1)
>                                    "resource://gre/modules/AddonManager.jsm");
>  
>  Cu.import("resource://gre/modules/ExtensionContent.jsm");
>  Cu.import("resource://gre/modules/ExtensionManagement.jsm");
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "uuidgen",

`uuidGen`

::: toolkit/components/extensions/Extension.jsm:1141
(Diff revision 1)
>      let uuidGenerator = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
>      let bgScript = uuidGenerator.generateUUID().number + ".js";

Please use the uuidGen global here.

::: toolkit/components/extensions/Extension.jsm
(Diff revision 1)
> - * @param {string} id
>   * @param {nsIFile} file
>   * @param {nsIURI} rootURI
>   */
> -function MockExtension(id, file, rootURI) {
> +function MockExtension(file, rootURI) {
> -  this.id = id;

This class should still have an `id` property.

::: toolkit/components/extensions/Extension.jsm:1213
(Diff revision 1)
>    this.rootURI = rootURI;
> +  this.addon = null;
>  
>    let promiseEvent = eventName => new Promise(resolve => {
>      let onstartup = (msg, extension) => {
> -      if (extension.id == this.id) {
> +      if (extension.id == this.addon.id) {

What if `this.addon` is still undefined at this point?

::: toolkit/components/extensions/ExtensionXPCShellUtils.jsm:23
(Diff revision 1)
>  XPCOMUtils.defineLazyServiceGetter(this, "uuidGenerator",
>                                     "@mozilla.org/uuid-generator;1", "nsIUUIDGenerator");

This isn't used anymore.

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript.html:45
(Diff revision 1)
>    function contentScript() {
>      let manifest = browser.runtime.getManifest();
> -    void manifest.applications.gecko.id;
> +    if (manifest.manifest_version != null) {
> -    chrome.runtime.sendMessage(["chrome-namespace-ok"]);
> +      chrome.runtime.sendMessage(["chrome-namespace-ok"]);
> +    } else {
> +      throw new Error("getManifest() failed in content script");

Ick. This isn't necessary. In any case, we need to check a deeper path in the return value to make sure structured cloning works as expected. Better to just use a try-catch and `browser.test.fail` if it fails.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension.js
(Diff revision 1)
>  
>    return promiseInstallAllFiles([addonFile]).then(() => {
>      Services.obs.notifyObservers(addonFile, "flush-cache-entry", null);
>      return promiseAddonStartup();
> -  }).then(() => {
> -    return promiseAddonByID(aData.id);

I'd rather we continue returning an Addon from this function. Can we just extract the ID from the manifest data?

Comment 12

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/68756/#review66840

::: browser/components/extensions/test/browser/browser_ext_incognito_popup.js:109
(Diff revision 1)
> -  yield extension.startup();
> +  yield extension.awaitMessage("ready");
> +  extension.sendMessage("go");

Because the test sends click-{browser,page}Action messages to generate clicks, so we need those handlers attached before the test code runs.  But the message handlers need the extension id to find the right elements, and the purpose of this patch is to eliminate all instances where we refer to the extension id before extension.startup() has resolved (which sets the stage for the next patch)
So the simple fix is to not run the test code immediately from the background script but to send it a message when the click- message handlers have been installed.

::: browser/components/extensions/test/browser/browser_ext_pageAction_context.js:112
(Diff revision 1)
>      }
>    }
>  
>    let testNewWindows = 1;
>  
> -  let awaitFinish = new Promise(resolve => {
> +  extension.sendMessage("runTests");

The basic problem to be solved is the same as the one above.  Trying to write this code with a bunch of cooperating message handlers was driving me crazy, nudging it toward the async Task style made it make more sense to me, I can try to re-obfuscate it if you are strongly opposed to this :-)

::: browser/components/extensions/test/browser/browser_ext_pageAction_popup.js:172
(Diff revision 1)
> +    if (expecting.expectClosed) {
> +      break;
> +    }

This is a variant on the same issue from above.  It could be kept as an onMessage listener but we would need to change the background script logic to wait for a message from the main test before running so that the onMessage listener doesn't run before extension.startup() resolves.  Again, moving to the Task async style seemed clearest to me, I'm open to doing it differently if you feel strongly.

Comment 13

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/68756/#review66840

> Because the test sends click-{browser,page}Action messages to generate clicks, so we need those handlers attached before the test code runs.  But the message handlers need the extension id to find the right elements, and the purpose of this patch is to eliminate all instances where we refer to the extension id before extension.startup() has resolved (which sets the stage for the next patch)
> So the simple fix is to not run the test code immediately from the background script but to send it a message when the click- message handlers have been installed.

We should be able to attach an ID to the extension object before any extension code runs, so I'd rather just do that rather than refactor the test.

> This is a variant on the same issue from above.  It could be kept as an onMessage listener but we would need to change the background script logic to wait for a message from the main test before running so that the onMessage listener doesn't run before extension.startup() resolves.  Again, moving to the Task async style seemed clearest to me, I'm open to doing it differently if you feel strongly.

It's already task-style, though. This message handler is more of an RPC than a loop, and now we have two different places/ways that we determine when the test is done.

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8777144 [details]
Bug 1286908 Remove id allocation from SpecialPowers loadExtension()

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

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8777144 [details]
Bug 1286908 Remove id allocation from SpecialPowers loadExtension()

https://reviewboard.mozilla.org/r/68758/#review66806

> I'd rather we continue returning an Addon from this function. Can we just extract the ID from the manifest data?

We already have duplicates of that logic in several places and I'm reluctant to make more copies.  As long as this function is local to this file, it doesn't seem like a big deal.

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8777144 [details]
Bug 1286908 Remove id allocation from SpecialPowers loadExtension()

https://reviewboard.mozilla.org/r/68758/#review67412

::: browser/components/extensions/test/browser/browser_ext_pageAction_popup.js:146
(Diff revision 2)
> +    if (!pageActionId) {
> +      pageActionId = `${makeWidgetId(extension.id)}-page-action`;
> +    }
> +    if (!panelId) {
> +      panelId = `${makeWidgetId(extension.id)}-panel`;
> +    }

No need for the `if`s. These operations are cheap.

::: toolkit/mozapps/extensions/test/browser/browser_inlinesettings_browser.js:18
(Diff revision 2)
>  
>  var installedAddons = [];
>  
>  function installAddon(details) {
>    let id = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator)
> -                                              .generateUUID().number;
> +      .generateUUID().number;

Why the indentation change?

Comment 17

[Andrew Swan [:aswan]]

Comment on attachment 8777144 [details]
Bug 1286908 Remove id allocation from SpecialPowers loadExtension()

https://reviewboard.mozilla.org/r/68758/#review67418

::: toolkit/mozapps/extensions/test/browser/browser_inlinesettings_browser.js:18
(Diff revision 2)
>  
>  var installedAddons = [];
>  
>  function installAddon(details) {
>    let id = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator)
> -                                              .generateUUID().number;
> +      .generateUUID().number;

oops, no good reason, reverted

Comment 18

[Andrew Swan [:aswan]]

Comment on attachment 8777144 [details]
Bug 1286908 Remove id allocation from SpecialPowers loadExtension()

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

Comment 19

[Andrew Swan [:aswan]]

Comment on attachment 8777144 [details]
Bug 1286908 Remove id allocation from SpecialPowers loadExtension()

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

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8777144 [details]
Bug 1286908 Remove id allocation from SpecialPowers loadExtension()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68758/diff/4-5/

Comment 21

[Andrew Swan [:aswan]]

Comment on attachment 8777144 [details]
Bug 1286908 Remove id allocation from SpecialPowers loadExtension()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68758/diff/5-6/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88ea7345ceac
Remove id allocation from SpecialPowers loadExtension() r=kmag

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/88ea7345ceac