Closed Bug 1252871 Opened 8 years ago Closed 8 years ago

Implement chrome.runtime.onInstalled

Categories

(WebExtensions :: Untriaged, defect, P2)

defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Iteration:
52.1 - Oct 3
Tracking Status
firefox52 --- fixed

People

(Reporter: evilpie, Assigned: mattw)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [runtime][triaged])

Attachments

(3 files, 1 obsolete file)

This event is currently just ignored and never fires.
Whiteboard: [runtime][berlin]
Assignee: nobody → aswan
Priority: -- → P2
Whiteboard: [runtime][berlin] → [runtime][berlin]triaged
Whiteboard: [runtime][berlin]triaged → [runtime][triaged]
Assignee: aswan → mwein
Blocks: 1291627
From what I've seen, runtime.onInstalled is used for at least two purposes in Chrome extensions:
1) Performing post-install logic, e.g. updating data structures or opening a first run page.
2) Registering persistent rules like in the contextMenus API (especially with event pages).

Use case 1 can be emulated by an addon by saving the version value of chrome.runtime.getManifest().version (and if the browser version is relevant, navigator.userAgent) in localStorage or chrome.storage.local.

Firefox does not support event pages, so use case 2 can be addressed by unconditionally removing then registering the rules as follows:
chrome.contextMenus.removeAll(function() {
    chrome.contextMenus.create(...);
});


Note that for registering rules, Chrome's onInstalled is flawed when split incognito mode comes into play (https://crbug.com/258090, https://crbug.com/264963). The above suggestion is also a work-around to these issues.
@Rob Wu: Emulation solution not flawless yet due to other WebExtensions API issue re extension storage (see https://bugzilla.mozilla.org/show_bug.cgi?id=1293035)
Iteration: --- → 51.2 - Aug 29
Depends on: 1294287
Iteration: 51.2 - Aug 29 → 52.1 - Sep 26
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review80094

::: toolkit/components/extensions/ext-backgroundPage.js:107
(Diff revision 4)
>        AddonManager.getAddonByInstanceID(this.extension.addonData.instanceID)
>                    .then(addon => addon.setDebugGlobal(window));
>      }
>  
>      // TODO(robwu): This implementation of onStartup is wrong, see
>      // https://bugzil.la/1247435#c1

Now this `startup` event is used for both `onStartup` and `onInstalled`, I suggest to move the comment about `onStartup` to the `onStartup` implementation in `ext-runtime.js`.

::: toolkit/components/extensions/ext-runtime.js:31
(Diff revision 4)
>  extensions.registerSchemaAPI("runtime", "addon_parent", context => {
>    let {extension} = context;
>    return {
>      runtime: {
>        onStartup: new EventManager(context, "runtime.onStartup", fire => {
> -        extension.onStartup = fire;
> +        extension.on("startup", fire);

Can you add an extra line

```
let listener = () => fire();
```

and use it for `on` and `off`? Otherwise the `onStartup` events will be invoked with "startup" as parameter (because the callback to `on` is invoked with the event name as first parameter).

::: toolkit/components/extensions/ext-runtime.js:40
(Diff revision 4)
>        }).api(),
>  
> -      onInstalled: ignoreEvent(context, "runtime.onInstalled"),
> +      onInstalled: new EventManager(context, "runtime.onInstalled", fire => {
> +        let listener = () => {
> +          let prefs = PreferenceManager.get(extension.id, RUNTIME_MAP_PREF);
> +          if (!prefs.manifestVersion || prefs.manifestVersion != extension.manifest.version) {

Nit: `manifest.json` already has something called `manifest_version`, so this pref name is a bit confusing. Can you rename it to something less ambiguous and more descriptive, such as `prefs.lastInstalledVersion`?

::: toolkit/components/extensions/test/xpcshell/head.js:11
(Diff revision 4)
>  
>  Components.utils.import("resource://gre/modules/Task.jsm");
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  Components.utils.import("resource://gre/modules/Timer.jsm");
> +Components.utils.import("resource://testing-common/AddonTestUtils.jsm");
> +Components.utils.import("resource://gre/modules/AddonManager.jsm", {});

WHat's the purpose of this import? I thought that if you use a second argument the module will only be exported to that object. Seeing that you don't use the return value nor the second parameter, I think that this line can be removed.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:22
(Diff revision 4)
> +
> +Object.defineProperty(this, "gProfD", {
> +  get() {
> +    return AddonTestUtils.profileDir.clone();
> +  },
> +});

Why not

```
XPCOMUtils.defineLazyGetter(this, "gProfD", () => {
  return AddonTestUtils.profileDir.clone();
});
```

or even

```
const gProfD = AddonTestUtils.profileDir.clone();
```

?

Similarly for `gTmpD`.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:119
(Diff revision 4)
> +        browser.test.notifyPass("runtime.onInstalled");
> +      });
> +    }
> +  });
> +
> +  file.moveTo(addonsDir, `test_runtime_on_installed-2.0.xpi`);

Nitty nit: Normal quotes work fine too.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:154
(Diff revision 4)
> +  stagedDir.remove(false);
> +
> +  yield extension.markUnloaded();
> +  yield updated_addon.uninstall();
> +  yield promiseShutdownManager();
> +});

Can you also add a test for the following:

- onInstalled is not fired when an extension starts up normally (install, disable or remove, re-enable/startup, [test here]).
- onInstalled is not fired when an extension uses `runtime.reload()` to issue a reload.

To test that `onInstalled` is not fired (notifyFail), you could subscribe to the startup event (since onInstalled is currently tied to that), asynchronously (`Promise.resolve().then(...)`) send a message to the extension and wait for a reply (ping-pong, to make sure that the onInstalled event has had a chance to round-trip if it were to be fired; asynchronously to make sure that your test listener is executed last, long after any potential `onInstalled` triggers).
Comment on attachment 8794948 [details]
Bug 1252871 - Move and sort methods imported from AddonTestUtils

https://reviewboard.mozilla.org/r/81150/#review80202
Attachment #8794948 - Flags: review?(aswan) → review+
Comment on attachment 8794949 [details]
Bug 1252871 - Move promiseFindAddonUpdates to AddonTestUtils

https://reviewboard.mozilla.org/r/81152/#review80200

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:1102
(Diff revision 1)
> +    return new Promise((resolve, reject) => {
> +      let result = {};
> +      addon.findUpdates({
> +        onNoCompatibilityUpdateAvailable: function(addon2) {
> +          if ("compatibilityUpdate" in result) {
> +            do_throw("Saw multiple compatibility update events");

If you're going to use this, i think you need to get it from testScope, but I don't think you even need it, you can just throw an Error here (and all the other similar cases here)

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1106
(Diff revision 1)
>  do_register_cleanup(function addon_cleanup() {
>    if (timer)
>      timer.cancel();
>  });
>  
> -/**
> + /**

whitespace
Attachment #8794949 - Flags: review?(aswan) → review+
Comment on attachment 8794950 [details]
Bug 1252871 - Add a utility for managing extension preferences

https://reviewboard.mozilla.org/r/81154/#review80204

Do you have other applications in mind for this?  I'd rather see at least two different places where we're repeating similar code before creating something meant to be a reusable utility.
Attachment #8794950 - Flags: review?(aswan) → review-
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review80206

I've made some comments but I also agree with all of Rob's comments

::: toolkit/components/extensions/ExtensionXPCShellUtils.jsm:301
(Diff revision 4)
>  
>      return new ExtensionWrapper(extension, this.currentScope);
>    },
> +
> +  getExtension(id) {
> +    const { GlobalManager } = Cu.import("resource://gre/modules/Extension.jsm", {});

make this a lazy getter at the top of the file

::: toolkit/components/extensions/ExtensionXPCShellUtils.jsm:303
(Diff revision 4)
>    },
> +
> +  getExtension(id) {
> +    const { GlobalManager } = Cu.import("resource://gre/modules/Extension.jsm", {});
> +    let extension = GlobalManager.extensionMap.get(id);
> +    return new ExtensionWrapper(extension, this.currentScope);

The state for the new wrapper needs to be initialized here.  I don't think you can distinguish pending from running at this point, but just setting it to running (with a comment explaining that it may actually be pending) would be okay.

::: toolkit/components/extensions/ext-runtime.js:24
(Diff revision 4)
>  } = ExtensionUtils;
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "NativeApp",
>                                    "resource://gre/modules/NativeMessaging.jsm");
>  
> +const RUNTIME_MAP_PREF = "extensions.webextensions.runtime";

I think this name is way too vague.
extensions.webextensions.lastVersion?

::: toolkit/components/extensions/ext-runtime.js:42
(Diff revision 4)
> -      onInstalled: ignoreEvent(context, "runtime.onInstalled"),
> +      onInstalled: new EventManager(context, "runtime.onInstalled", fire => {
> +        let listener = () => {
> +          let prefs = PreferenceManager.get(extension.id, RUNTIME_MAP_PREF);
> +          if (!prefs.manifestVersion || prefs.manifestVersion != extension.manifest.version) {
> +            prefs.manifestVersion = extension.manifest.version;
> +            PreferenceManager.set(extension.id, RUNTIME_MAP_PREF, prefs);

The prefs entry needs to get cleaned up when the extension is uninstalled.

::: toolkit/components/extensions/test/xpcshell/head.js:141
(Diff revision 4)
> + * Handler function that responds with the interpolated
> + * static file associated to the URL specified by request.path.
> + * This replaces the %PORT% entries in the file with the actual
> + * value of the running server's port (stored in gPort).
> + */
> +function interpolateAndServeFile(request, response) {

This is some old code that doesn't follow a lot of current practices.  How about just inline in your test creating your payload (with the proper port already interpolated) and creating a little handler to emit the payload, instead of using this?

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:13
(Diff revision 4)
> +
> +// Allow for unsigned addons.
> +AddonTestUtils.overrideCertDB();
> +
> +// Ensure signature checks are enabled by default.
> +Services.prefs.setBoolPref(PREF_XPI_SIGNATURES_REQUIRED, true);

why not leave this at its default?

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:65
(Diff revision 4)
> +add_task(function* setup() {
> +  yield ExtensionTestUtils.startAddonManager();
> +});
> +
> +add_task(function* test_runtime_on_installed() {
> +  yield promiseStartupManager();

you shouldn't need both this at the startAddonManager() a few lines above

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:93
(Diff revision 4)
> +  let addon = yield promiseAddonByID(EXTENSION_ID);
> +  do_check_neq(addon, null);
> +  do_check_eq(addon.version, "1.0");
> +  do_check_eq(addon.name, "Generated extension");
> +  do_check_true(addon.isCompatible);
> +  do_check_false(addon.appDisabled);
> +  do_check_true(addon.isActive);
> +  do_check_eq(addon.type, "extension");

other than checking that addon isn't null, i don't think you need all this here

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:119
(Diff revision 4)
> +        browser.test.notifyPass("runtime.onInstalled");
> +      });
> +    }
> +  });
> +
> +  file.moveTo(addonsDir, `test_runtime_on_installed-2.0.xpi`);

Instead of creating an addons directory and cleaning it up after the test, just do
`testserver.registerFile("/addons/blah.xpi", file)` here

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:130
(Diff revision 4)
> +  yield promiseCompleteAllInstalls([install]);
> +
> +  yield extension.awaitMessage("reload");
> +
> +  let [updated_addon] = yield promiseInstalled;
> +  yield promiseWebExtensionStartup();

why is this needed?

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:133
(Diff revision 4)
> +  do_check_eq(updated_addon.version, "2.0");
> +  do_check_eq(updated_addon.name, "Test Runtime onInstalled");
> +  do_check_true(updated_addon.isCompatible);
> +  do_check_false(updated_addon.appDisabled);
> +  do_check_true(updated_addon.isActive);
> +  do_check_eq(updated_addon.type, "extension");

this can all go

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:143
(Diff revision 4)
> +  // TODO (matt): The staged directory needs to be cleaned up for the test to pass. This
> +  // might not be the Right Way to do this.

This isn't right, why does the staged dir exist here at all?
Attachment #8794411 - Flags: review?(aswan) → review-
Comment on attachment 8794949 [details]
Bug 1252871 - Move promiseFindAddonUpdates to AddonTestUtils

https://reviewboard.mozilla.org/r/81152/#review80200

> If you're going to use this, i think you need to get it from testScope, but I don't think you even need it, you can just throw an Error here (and all the other similar cases here)

You're right, I was originally getting it from testScope but I must've forgot to put it back when I was cleaning up the patch.  Regardless, I'll switch to throwing Errors instead since that's more consistent with the rest of the file.
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review80094

> Now this `startup` event is used for both `onStartup` and `onInstalled`, I suggest to move the comment about `onStartup` to the `onStartup` implementation in `ext-runtime.js`.

Thanks, moved it.

> Can you add an extra line
> 
> ```
> let listener = () => fire();
> ```
> 
> and use it for `on` and `off`? Otherwise the `onStartup` events will be invoked with "startup" as parameter (because the callback to `on` is invoked with the event name as first parameter).

Oh I see, that makes sense - fixed.

> Nit: `manifest.json` already has something called `manifest_version`, so this pref name is a bit confusing. Can you rename it to something less ambiguous and more descriptive, such as `prefs.lastInstalledVersion`?

Sounds good to me - updated.

> WHat's the purpose of this import? I thought that if you use a second argument the module will only be exported to that object. Seeing that you don't use the return value nor the second parameter, I think that this line can be removed.

You're right, thanks - removed.

> Why not
> 
> ```
> XPCOMUtils.defineLazyGetter(this, "gProfD", () => {
>   return AddonTestUtils.profileDir.clone();
> });
> ```
> 
> or even
> 
> ```
> const gProfD = AddonTestUtils.profileDir.clone();
> ```
> 
> ?
> 
> Similarly for `gTmpD`.

I think your second suggestion is reasonable here - updated.

> Nitty nit: Normal quotes work fine too.

Updated.
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review80206

> make this a lazy getter at the top of the file

Done.  Should I move the other instances of Cu.import to use lazy getters as well in this patch?

> The state for the new wrapper needs to be initialized here.  I don't think you can distinguish pending from running at this point, but just setting it to running (with a comment explaining that it may actually be pending) would be okay.

Done.

> I think this name is way too vague.
> extensions.webextensions.lastVersion?

Updated.

> The prefs entry needs to get cleaned up when the extension is uninstalled.

Good catch - fixed.

> This is some old code that doesn't follow a lot of current practices.  How about just inline in your test creating your payload (with the proper port already interpolated) and creating a little handler to emit the payload, instead of using this?

Doing this simplified the code quite a bit - let me know if this isn't what you were thinking.

> why not leave this at its default?

Removed.

> you shouldn't need both this at the startAddonManager() a few lines above

Fixed.

> other than checking that addon isn't null, i don't think you need all this here

Yeah, that's reasonable - updated.

> Instead of creating an addons directory and cleaning it up after the test, just do
> `testserver.registerFile("/addons/blah.xpi", file)` here

This is much simpler, thanks!

> why is this needed?

The extension doesn't get added to the `GlobalManager.extensionMap` until startup - see https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#1623.  On a second look, though, I noticed `promiseWebExtensionStartup` already resolves with the extension, so we can just get it from there instead of using the `GlobalManager`.

> this can all go

Removed.
No longer depends on: 1294287
Attachment #8794411 - Flags: review?(aswan)
Attachment #8794950 - Flags: review?(aswan)
Depends on: 1306492
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review80206

> This isn't right, why does the staged dir exist here at all?

This is no longer needed now that bug 1306492 landed.
Comment on attachment 8794950 [details]
Bug 1252871 - Add a utility for managing extension preferences

https://reviewboard.mozilla.org/r/81154/#review86106

This won't work as-is, you're changing the format of the UUIDMap so existing mappings will break (meaning extensions will lose their storage etc when users upgrade to a build that has this code)
Attachment #8794950 - Flags: review?(aswan) → review-
Comment on attachment 8794950 [details]
Bug 1252871 - Add a utility for managing extension preferences

https://reviewboard.mozilla.org/r/81154/#review86106

Ah you're right, that's a really good catch. My goal was to make it so only objects could be stored for each extension, but I think because of this case it's probably easiest to allow any JSON serializable type to be stored. I'm also okay with removing this since there are only these two use cases that I know of.

In any case, I updated the latest revision to allow extensions to store anything that is JSON serializable. I also renamed the utility to ExtensionPreferences because I think that is better. Let me know what you think.
Where are we at with this bug?  We discussed using bootstrap reasons instead of preferences on IRC and I'm strongly in favor of doing that (which will be necessary anyway for onStartup)
Flags: needinfo?(mwein)
(In reply to Andrew Swan [:aswan] from comment #32)
> Where are we at with this bug?  We discussed using bootstrap reasons instead
> of preferences on IRC and I'm strongly in favor of doing that (which will be
> necessary anyway for onStartup)

Using bootstrap reasons sounds good to me also. I think we'll still need to use preferences to keep track of the browser version, though.
Flags: needinfo?(mwein)
Comment on attachment 8794950 [details]
Bug 1252871 - Add a utility for managing extension preferences

clearing review flag until this is ready
Attachment #8794950 - Flags: review?(aswan)
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

clearing review flag until this is ready
Attachment #8794411 - Flags: review?(aswan)
I think it would be great if we could get this landed in 52 with follow-up bugs for adding additional tests once we have support for properly re-wrapping the extension after reloads and restarts. What are you thoughts on this? If you agree do you think you could give this another review?
Flags: needinfo?(aswan)
Blocks: 1247435
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review87752

This is heading in the right direction.  There are some line-item comments but I think the two biggest things are:
- I think the preference handling can and should be greatly simplified.  Lets get rid of the generic preference wrapper thing and just use a single pref for "last running browser version for the webextensions system".  Just update that pref and a "browserChanged" global once in ExtensionManagement.
- The tests that assume a local variable persists across restarts -- I think they're trying to be more complicated than they need to be.  All you really need to check is if the event has fired in a particular session, just keeping a boolean and having a message from the test to the extension to retrieve that boolean should be enough.

::: toolkit/components/extensions/ExtensionXPCShellUtils.jsm:78
(Diff revision 10)
> +    });
> +
> +    this.testScope.do_print(`Extension loaded`);
> +  }
> +
> +  listeners() {

Lets make the name more descriptive, attachListeners perhaps?

::: toolkit/components/extensions/ExtensionXPCShellUtils.jsm:159
(Diff revision 10)
>    sendMessage(...args) {
>      this.extension.testMessage(...args);
>    }
>  
> +  awaitEvent(event) {
> +    const {Management} = Cu.import("resource://gre/modules/Extension.jsm", {});

Make this a top-level lazy getter

::: toolkit/components/extensions/ExtensionXPCShellUtils.jsm:164
(Diff revision 10)
> +    const {Management} = Cu.import("resource://gre/modules/Extension.jsm", {});
> +    return new Promise(resolve => {
> +      let listener = (evt, ext) => {
> +        Management.off(event, listener);
> +        this.extension = ext;
> +        this.listeners();

This seems to be merging a couple of unrelated things, waiting for a specific event and re-attaching listeners...

::: toolkit/components/extensions/ext-runtime.js:28
(Diff revision 10)
> +const LAST_BROWSER_VERSION_PREF = "extensions.webextensions.browserVersion";
> +
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("startup", (type, extension) => {
> +  // Initialize the browser version pref with the current browser version if it is not set.
> +  let prefs = ExtensionPreferences.get(extension.id, LAST_BROWSER_VERSION_PREF);

This looks like overkill, ExtensionPreferences is written to do per-extension prefs but you just need a single pref here for all webextensions.

::: toolkit/components/extensions/ext-runtime.js:55
(Diff revision 10)
>        }).api(),
>  
> -      onInstalled: ignoreEvent(context, "runtime.onInstalled"),
> +      onInstalled: new EventManager(context, "runtime.onInstalled", fire => {
> +        let listener = () => {
> +          switch (extension.startupReason) {
> +            case 1: // APP_STARTUP

Please either import this from XPIProvider.jsm or define symbolic constants for the values you care about.

::: toolkit/components/extensions/schemas/runtime.json:125
(Diff revision 10)
>          "description": "Result of the update check."
>        },
>        {
>          "id": "OnInstalledReason",
>          "type": "string",
> -        "enum": ["install", "update", "chrome_update", "shared_module_update"],
> +        "enum": ["install", "update", "firefox_update"],

Can we just call this "browser_update"

::: toolkit/components/extensions/schemas/runtime.json:473
(Diff revision 10)
>                "previousVersion": {
>                  "type": "string",
>                  "optional": true,
> +                "unsupported": true,
>                  "description": "Indicates the previous version of the extension, which has just been updated. This is present only if 'reason' is 'update'."
>                },

We're not actually including this, and I don't think we have an easy way to do so, at the very least we should leave it unsupported.

::: toolkit/components/extensions/schemas/runtime.json:479
(Diff revision 10)
>                "id": {
>                  "type": "string",
>                  "optional": true,
> +                "unsupported": true,
>                  "description": "Indicates the ID of the imported shared module extension which updated. This is present only if 'reason' is 'shared_module_update'."
>                }

I'm not sure what this applies to in Chrome but since we don't support it, lets at least leave the unsupported flag.

::: toolkit/components/extensions/test/xpcshell/head.js:10
(Diff revision 10)
>  /* exported createHttpServer, promiseConsoleOutput, cleanupDir */
>  
>  Components.utils.import("resource://gre/modules/Task.jsm");
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  Components.utils.import("resource://gre/modules/Timer.jsm");
> +Components.utils.import("resource://testing-common/AddonTestUtils.jsm");

Since most other xpcshell tests don't care about this, just put it into the new test

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:95
(Diff revision 10)
> +  let promiseInstalled = promiseAddonEvent("onInstalled");
> +  yield promiseCompleteAllInstalls([install]);
> +
> +  yield extension.awaitMessage("reloading");
> +
> +  let awaitStartup = extension.awaitEvent("startup");

I don't think the actual promise here is useful for this test, but I think you need it for the side effects it currently has (re-attaching listeners in the wrapper).  Can you move the listener re-attach logic here instead?

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:105
(Diff revision 10)
> +  yield extension.unload();
> +
> +  yield updated_addon.uninstall();

Something doesn't seem right here, why do you need both of these?

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:128
(Diff revision 10)
> +          "update_url": `http://localhost:${gPort}/test_update.json`,
> +        },
> +      },
> +    },
> +    background() {
> +      let installCount = 0;

I don't get the purpose of this?

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:183
(Diff revision 10)
> +          "update_url": `http://localhost:${gPort}/test_update.json`,
> +        },
> +      },
> +    },
> +    background() {
> +      let installCount = 0;

I'm not sure what you're trying to do here, the value stored in this variable is obviously not going to persist across reloads.
Attachment #8794411 - Flags: review?(aswan) → review-
Flags: needinfo?(aswan)
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review87752

> Lets make the name more descriptive, attachListeners perhaps?

Done.

> Make this a top-level lazy getter

Done.

> This seems to be merging a couple of unrelated things, waiting for a specific event and re-attaching listeners...

Yeah, I'll remove the code to re-attach the listeners so this only waits for the event to fire..

> This looks like overkill, ExtensionPreferences is written to do per-extension prefs but you just need a single pref here for all webextensions.

Good call, I've updated this.

> Please either import this from XPIProvider.jsm or define symbolic constants for the values you care about.

Done.

> Can we just call this "browser_update"

Sgtm.

> We're not actually including this, and I don't think we have an easy way to do so, at the very least we should leave it unsupported.

It's marked as unsupported.

> I'm not sure what this applies to in Chrome but since we don't support it, lets at least leave the unsupported flag.

Sounds good.

> I don't think the actual promise here is useful for this test, but I think you need it for the side effects it currently has (re-attaching listeners in the wrapper).  Can you move the listener re-attach logic here instead?

Done.

> I don't get the purpose of this?

I updated this to use a boolean instead.

> I'm not sure what you're trying to do here, the value stored in this variable is obviously not going to persist across reloads.

Fixed.
Attachment #8794950 - Attachment is obsolete: true
Attachment #8794950 - Flags: review?(aswan)
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review88118

Getting really close but I'd like to take one more look before landing with the changes below.

::: toolkit/components/extensions/ExtensionXPCShellUtils.jsm:167
(Diff revision 11)
>  
> +  awaitEvent(event) {
> +    return new Promise(resolve => {
> +      let listener = (evt, ext) => {
> +        Management.off(event, listener);
> +        this.extension = ext;

This should not be a side effect here.  As a matter of fact, awaitEvent() shouldn't be in ExtensionWrapper at all.  And it should resolve with the given extension, let the caller replace the extension in a given wrapper if it needs to.

::: toolkit/components/extensions/ext-runtime.js:34
(Diff revision 11)
>    let {extension} = context;
>    return {
>      runtime: {
> +      // TODO(robwu): This implementation of onStartup is wrong, see
> +      // https://bugzil.la/1247435#c1
>        onStartup: new EventManager(context, "runtime.onStartup", fire => {

Did you mean to include this here?  It also needs to be marked supported in the manifest and have tests.  Or should that all be done as part of a separate bug?

::: toolkit/components/extensions/ext-runtime.js:44
(Diff revision 11)
>          return () => {
> -          extension.onStartup = null;
> +          extension.off("startup", listener);
>          };
>        }).api(),
>  
> -      onInstalled: ignoreEvent(context, "runtime.onInstalled"),
> +      onInstalled: new EventManager(context, "runtime.onInstalled", fire => {

SingletonEventManager

::: toolkit/components/extensions/ext-runtime.js:48
(Diff revision 11)
> +              let browserVersion = Preferences.get(LAST_BROWSER_VERSION_PREF, "");
> +              if (browserVersion != Services.appinfo.version) {

I don't think this will work, you already re-set the pref above.  I was thinking you would have a global called something like `browserUpdated`.  That's initialized to false and at startup you read the pref and if it doesn't match, set `browserUpdated` to true and update the pref.  Then the code here is just `if (browserUpdated) { ... }`

::: toolkit/components/extensions/ext-runtime.js:54
(Diff revision 11)
> +              if (browserVersion != Services.appinfo.version) {
> +                Preferences.set(LAST_BROWSER_VERSION_PREF, Services.appinfo.version);
> +                fire({reason: "browser_update"});
> +              }
> +              break;
> +            case "install": // ADDON_INSTALL

comment is unneeded

::: toolkit/components/extensions/test/xpcshell/head.js:28
(Diff revision 11)
>                                    "resource://gre/modules/FileUtils.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "HttpServer",
>                                    "resource://testing-common/httpd.js");
>  XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
>                                    "resource://gre/modules/NetUtil.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",

this isn't needed

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:85
(Diff revision 11)
> +  do_check_eq(details.reason, "install");
> +
> +  let addon = yield promiseAddonByID(EXTENSION_ID);
> +  do_check_eq(addon.version, "1.0");
> +
> +  let file = createTempWebExtensionFile({

Can you put all the setup of extensions, update manifests, etc at the top and then have the logical flow of the test all together below that

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:146
(Diff revision 11)
> +      let onInstalledFired = false;
> +
> +      browser.runtime.onInstalled.addListener((details) => {
> +        onInstalledFired = true;
> +        browser.test.sendMessage("installed", details);
> +      });

This could be simplified by having `onInstallDetails` which is initialized to null and having the messaging below reply with its contents, then if you expect it to have happened, you check the contents of details, if you expect it to have not happened, you check that the reply is null.

::: toolkit/mozapps/extensions/internal/WebExtensionBootstrap.js:12
(Diff revision 11)
>  Components.utils.import("resource://gre/modules/Extension.jsm");
>  
>  var extension;
>  
> +const REASON_TO_STRING_MAP = {
> +  1: "startup",

can you use the original names here to avoid confusion?  (e.g., APP_STARTUP, ADDON_INSTALL, etc)
Attachment #8794411 - Flags: review?(aswan)
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review88264

Nice!  Thanks for sticking through a bunch of iterations.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:42
(Diff revision 12)
> +
> +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "42");
> +
> +function awaitEvent(eventName) {
> +  return new Promise(resolve => {
> +    let listener = (event, extension) => {

make this `...args` (and the same 2 lines down)

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:127
(Diff revision 12)
> +  let promiseInstalled = promiseAddonEvent("onInstalled");
> +  yield promiseCompleteAllInstalls([install]);
> +
> +  yield extension.awaitMessage("reloading");
> +
> +  let awaitReady = awaitEvent("startup");

nit: change awaitReady to something like startupPromise

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled.js:156
(Diff revision 12)
> +    manifest: {
> +      "version": "1.0",
> +      "applications": {
> +        "gecko": {
> +          "id": EXTENSION_ID,
> +          "update_url": `http://localhost:${gPort}/test_update.json`,

This line doesn't appear to be needed here, it can go
(likewise for the next few occurences below)
Attachment #8794411 - Flags: review?(aswan) → review+
By the way, we should be able to support the previousVersion property for upgrades, it gets passed as another parameter to the bootstrap startup() method.  But lets not let that hold up the patches here, can you file a separate bug to add that?
(for comment 50)
Flags: needinfo?(mwein)
I filed bug 1313648 to track adding support for the previousVersion property.
Flags: needinfo?(mwein)
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review87752

> Since most other xpcshell tests don't care about this, just put it into the new test

I was able to move everything except for the import of AddonTestUtils.jsm. I found out that the import only works if it comes before we initialize ExtensionTestUtils, and I filed bug 1313644 to track fixing this.

> Something doesn't seem right here, why do you need both of these?

I filed bug 1313653 to track fixing this.
Keywords: checkin-needed
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review88264

Thanks for the reviews! I'm really happy with how much your reviews helped me improve this code.

In the latest revision I made some minor changes in addition to the changes you requested:

1. In the test:
    - Removed some unused imported functions from AddonTestUtils.
    - Moved the global variables only used by the addon update test into the test.
2. Resolved all eslint errors.

> make this `...args` (and the same 2 lines down)

I think `Promise.resolve` would need to take `args` rather than `...args` for this to work, but either way I'd rather just resolve with the extension since the first argument is only the name of the event which we already know. However, since we do get the event name here, I added a check to the listener to make sure that the name of the event is correct before resolving with the extension.

> nit: change awaitReady to something like startupPromise

Done.

> This line doesn't appear to be needed here, it can go
> (likewise for the next few occurences below)

Removed.
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review88264

> I think `Promise.resolve` would need to take `args` rather than `...args` for this to work, but either way I'd rather just resolve with the extension since the first argument is only the name of the event which we already know. However, since we do get the event name here, I added a check to the listener to make sure that the name of the event is correct before resolving with the extension.

Sorry, I meant `(event, ...args)`.  You're right there's no need to resolve with the event name.  But not all events have an `extension` parameter.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9c2c3780aa4c
Move and sort methods imported from AddonTestUtils r=aswan
https://hg.mozilla.org/integration/autoland/rev/c2a511511a72
Move promiseFindAddonUpdates to AddonTestUtils r=aswan
https://hg.mozilla.org/integration/autoland/rev/b3a08a040c8a
Add support for runtime.onInstalled r=aswan
Keywords: checkin-needed
Comment on attachment 8794411 [details]
Bug 1252871 - Add support for runtime.onInstalled

https://reviewboard.mozilla.org/r/80898/#review88264

> Sorry, I meant `(event, ...args)`.  You're right there's no need to resolve with the event name.  But not all events have an `extension` parameter.

No worries. What do you suggest we do since this already landed? The test only cares about "ready" and "startup" which both have the `extension` parameter.
(In reply to Matthew Wein [:K-9, :mattw] from comment #60)
> No worries. What do you suggest we do since this already landed? The test
> only cares about "ready" and "startup" which both have the `extension`
> parameter.

Since we moved this out of ExtensionXPCShellUtils.jsm, its not such a big deal.  If you touch that file again soon (implementing previousVersion maybe?) you could change it but I wouldn't sweat it.
It looks like I needed to update test_ext_all_apis.js, sorry about that.  This should be good to go now.
Flags: needinfo?(mwein)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/593e3e011fc7
Move and sort methods imported from AddonTestUtils r=aswan
https://hg.mozilla.org/integration/autoland/rev/7bcf61591212
Move promiseFindAddonUpdates to AddonTestUtils r=aswan
https://hg.mozilla.org/integration/autoland/rev/568c403cde43
Add support for runtime.onInstalled r=aswan
Keywords: checkin-needed
This seems to not work when using "Load Temporary Add-on" in about:debugging - it seems that the event isn't triggered.

Tested with an add-on that just contains a single background script:

function handleInstalled(details) {
  console.log(details.reason);
  chrome.tabs.create({
    url: "http://chilloutandwatchsomecatgifs.com/"
  });
}

chrome.runtime.onInstalled.addListener(handleInstalled);
Please see bug 1323938.
(In reply to Andy McKay [:andym] from comment #68)
> Please see bug 1323938.

Oh that would be it.

I updated the compat data, and noted this problem:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime#Browser_compatibility
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/onInstalled#Browser_compatibility

I'm going to mark this dev-doc-complete, but let me know if you need anything else.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.