Bob Silverberg [:bsilverberg] Reporter
	Description • 8 years ago

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.

	Kris Maglione [:kmag]
	Comment 1 • 8 years ago

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?

	Andrew Swan [:aswan] Assignee
	Comment 2 • 8 years ago

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

	Bob Silverberg [:bsilverberg] Reporter
	Comment 3 • 8 years ago

The ID I am using is `get_self_test@tests.mozilla.com`. 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.

	Kris Maglione [:kmag]
	Comment 4 • 8 years ago

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.

	Andrew Swan [:aswan] Assignee
	Comment 5 • 8 years ago

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?

	Kris Maglione [:kmag]
	Comment 6 • 8 years ago

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.

	Andrew Swan [:aswan] Assignee
	Comment 7 • 8 years ago

(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.

	Andrew Swan [:aswan] Assignee
	Comment 8 • 8 years ago

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/

	Andrew Swan [:aswan] Assignee
	Comment 9 • 8 years ago

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/

	Kris Maglione [:kmag]
	Comment 10 • 8 years ago

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?

	Kris Maglione [:kmag]
	Comment 11 • 8 years ago

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 = "options@tests.mozilla.org";

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?

	Andrew Swan [:aswan] Assignee
	Comment 12 • 8 years ago

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.

	Kris Maglione [:kmag]
	Comment 13 • 8 years ago

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.

	Andrew Swan [:aswan] Assignee
	Comment 15 • 8 years ago
mozreview-review-reply

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.

	Kris Maglione [:kmag]
	Comment 16 • 8 years ago
mozreview-review

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?

	Andrew Swan [:aswan] Assignee
	Comment 17 • 8 years ago
mozreview-review

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

	Pulsebot
	Comment 22 • 8 years ago

Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88ea7345ceac
Remove id allocation from SpecialPowers loadExtension() r=kmag

	Carsten Book [:Tomcat]
	Comment 23 • 8 years ago
bugherder

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