1269889 - addon.reload() should behave more like temporary add-on loading

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Kumar McMillan [:kumar]]

The addon.reload() function should behave more like temporary add-on loading for a couple of reasons:

- it will fix metadata refresh issues such as bug 1267870 and bug 1268884
- it will make the code path simpler and more maintainable
- future features such as CSS reloading will be easier to support

The downside is that you won't be able to reload a permanently installed add-on. This trade-off seems OK to me since you only need reload for development purposes.

Comment 1

[Kumar McMillan [:kumar]]

Attachment: MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

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

Comment 2

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/50251/#review47075

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6978
(Diff revision 1)
>  
>    get type() {
>      return getExternalType(addonFor(this).type);
>    },
>  
> +  get temporarilyInstalled() {

This is used internally by addon.reload() but about:debugging also needs it to know when to provide a Reload button.

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

https://reviewboard.mozilla.org/r/50251/#review47261

Looks great, all the line-by-line suggestions below are minor things.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6978
(Diff revision 1)
>  
>    get type() {
>      return getExternalType(addonFor(this).type);
>    },
>  
> +  get temporarilyInstalled() {

Is there a subsequent patch coming to about:debugging that will do this?  I don't see it in this patch.

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:36
(Diff revision 1)
>  function* tearDownAddon(addon) {
>    addon.uninstall();
>    yield promiseShutdownManager();
>  }
>  
> -add_task(function* test_reloading_an_addon() {
> +function* assertError(test, expectedErrorMsg) {

You could also use `Assert.throws()` here?  (documented under https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Assertions_and_logging)

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:56
(Diff revision 1)
> -  const [disabledAddon] = yield onDisabled;
> +  yield addon.reload();
> -  do_check_eq(disabledAddon.id, addonId);
>  
> -  const [enabledAddon] = yield onEnabled;
> -  do_check_eq(enabledAddon.id, addonId);
> +  const [reinstalledAddon] = yield onInstalled;
> +  do_check_eq(reinstalledAddon.id, sampleAddon.id);
>  

Just for sanity, also check that we can find the new addon via promiseAddonByID() and that its isActive flag is true?

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:126
(Diff revision 1)
> +
> +  // Trigger an installation error with an empty manifest.
> +  writeInstallRDFToDir({}, tempdir, manifestSample.id);
> +
> +  assertError(() => addon.reload(), "No ID in install manifest");
>  

what's the expected behavior here?  that the old add-on is still present and active?  that seems simplest but possibly confusing to the user.  regardless, lets nail down the expected behavior and test explicitly for it.

Comment 4

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/50251/#review47261

> Is there a subsequent patch coming to about:debugging that will do this?  I don't see it in this patch.

I think the devtools folks will want to review it in a separate patch so I didn't add it here. It will need another patch (a cache flush fix) that should probably get some devtools eyes on it anyway.

Comment 5

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/50251/#review47261

> You could also use `Assert.throws()` here?  (documented under https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Assertions_and_logging)

Oh, I didn't know about that, thanks. However, it only takes a callback, not a promise. I can't seem to find anything that supports promises.

Comment 6

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

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

Comment 7

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/50251/#review47261

> what's the expected behavior here?  that the old add-on is still present and active?  that seems simplest but possibly confusing to the user.  regardless, lets nail down the expected behavior and test explicitly for it.

Good call. The old add-on should be left undisturbed by the broken reload so I added that to the test.

Comment 8

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

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

Comment 9

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

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

Comment 10

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/50251/#review47261

> Oh, I didn't know about that, thanks. However, it only takes a callback, not a promise. I can't seem to find anything that supports promises.

Its apparently missing from MDN but there is Assert.rejects:
https://dxr.mozilla.org/mozilla-central/source/testing/modules/Assert.jsm#376

Comment 11

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

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

Comment 12

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/50251/#review47577

::: devtools/server/actors/root.js
(Diff revision 5)
>      });
>    },
>  
>    onAddonListChanged: function () {
>      this.conn.send({ from: this.actorID, type: "addonListChanged" });
> -    this._parameters.addonList.onListChanged = null;

This was unregistering all AddonManager listeners which was failing the reload test because uninstalling after a reload wasn't triggering a page refresh. As far as I can tell, it's not needed for anything.

Comment 13

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

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

Comment 14

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

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

Comment 15

[Kumar McMillan [:kumar]]

(In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment #12) },
> >    onAddonListChanged: function () {
> >      this.conn.send({ from: this.actorID, type: "addonListChanged" });
> > -    this._parameters.addonList.onListChanged = null;
> 
> This was unregistering all AddonManager listeners which was failing the
> reload test because uninstalling after a reload wasn't triggering a page
> refresh. As far as I can tell, it's not needed for anything.

bah! This can't be right. I think this change will cause it to leak listeners. I'm going to dig into it deeper to find a better fix.

Comment 16

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/7-8/

Comment 17

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/8-9/

Comment 18

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/50251/#review47841

::: devtools/client/aboutdebugging/test/browser_addons_reload.js:62
(Diff revision 9)
>    info("Uninstall addon installed earlier.");
> -  yield uninstallAddon(document, ADDON_ID, ADDON_NAME);
> +  const onUninstalled = promiseAddonEvent("onUninstalled");
> +  reloadedAddon.uninstall();
> +  const [uninstalledAddon] = yield onUninstalled;
> +  ok(uninstalledAddon.id === ADDON_ID,
> +     "Add-on was uninstalled: " + uninstalledAddon.id);

The old approach tested that addon.uninstall() removed the add-on from the UI but this is already tested elsewhere.

I switched to a simpler uninstall approach here because there was a timing problem with the old approach. The old approach started the install too early, before the add-on reload had a chance to refresh the about:debugging UI; the test code would then execute at a time when the UI was not listening to uninstall events so the UI would never get updated.

Comment 19

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/50251/#review47853

This looks great, just a nitpick that you can feel free to ignore, the tests might be more readable with interpolated strings (e.g., \`uninstalled ${id}\` instead of "uninstalled " + id.

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

https://reviewboard.mozilla.org/r/50251/#review47861

Looks good to me, aside from some nits.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7351
(Diff revision 9)
>      finally {
>        zipReader.close();
>      }
>    },
>  
>    reload: function() {

Please add a doc comment, and in particular mention that this only works for temporary add-ons.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7353
(Diff revision 9)
>      }
>    },
>  
>    reload: function() {
> -    return new Promise(resolve => {
> +    return new Promise((resolve) => {
>        if (this.appDisabled) {

It would be better to check `enabled` and `PERM_CAN_DISABLE` here, I think.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7364
(Diff revision 9)
> -      if (!isReloadable) {
> -        throw new Error(
> +        logger.debug("cannot reload add-on at " + addon._sourceBundle);
> +        throw new Error("only temporary add-ons can be reloaded");
> -          "cannot reload add-on because it requires a browser restart");
>        }
>  
> -      this.userDisabled = true;
> +      logger.debug("reloading add-on " + addon.id);

Please use template strings rather than concatenation in new code.

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:45
(Diff revision 9)
> -  const onEnabled = promiseAddonEvent("onEnabled");
>  
>    yield addon.reload();
>  
> -  const [disabledAddon] = yield onDisabled;
> -  do_check_eq(disabledAddon.id, addonId);
> +  const [reinstalledAddon] = yield onInstalled;
> +  do_check_eq(reinstalledAddon.id, sampleAddon.id);

It's better to use `equal` in newer code.

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:82
(Diff revision 9)
> +      name: "Test Bootstrap 1",
> +    }), tempdir, manifestSample.id, "bootstrap.js");
> +
> +  yield AddonManager.installTemporaryAddon(unpacked_addon);
> +  const addon = yield promiseAddonByID(manifestSample.id);
> +  do_check_neq(addon, null);

It's better to use `notEqual` in newer code.

Comment 21

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/9-10/

Comment 22

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/10-11/

Comment 23

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/11-12/

Comment 24

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/12-13/

Comment 25

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/13-14/

Comment 26

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/14-15/

Comment 27

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/15-16/

Comment 28

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/50251/#review48449

Looks good to me.  I would be inclined to create two separate commits, one that fixes the bootstrap reason passed to install (and the corresponding tests) and then a second one that redoes reload, just so that each commit does a single thing and can be followed more easily if/when somebody comes back to look at the history.  But I don't feel that strongly about it.
Nice work, I know this bug has been a slog!

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3925
(Diff revision 16)
>          let newVersion = addon.version;
>          let oldVersion = oldAddon.version;
>          let uninstallReason = Services.vc.compare(oldVersion, newVersion) < 0 ?
>                                BOOTSTRAP_REASONS.ADDON_UPGRADE :
>                                BOOTSTRAP_REASONS.ADDON_DOWNGRADE;
> +        if (Services.vc.compare(newVersion, oldVersion) >= 0) {

nitpick: you could roll the setting of uninstallReason above into this if statement

Comment 29

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/50251/#review48451

wtf reviewboard, i could swear i checked "ship it" on the last review.

Comment 30

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/50251/#review48449

> nitpick: you could roll the setting of uninstallReason above into this if statement

I combined the uninstall reason with the install reason and added tests. This reverses the uninstall reason logic but I think that was a bug all along.

Comment 31

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/16-17/

Comment 32

[Kris Maglione [:kmag]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

https://reviewboard.mozilla.org/r/50251/#review48517

This looks pretty good overall, I'm mostly just unreasonably nitpicky.

Sorry I didn't get to this yesterday. Mozreview didn't add it to my review queue, I guess because it already had an r+, and I apparently wasn't CCed on the bug either.

::: devtools/client/aboutdebugging/test/browser_addons_reload.js:44
(Diff revision 17)
> -  const onEnabled = promiseAddonEvent("onEnabled");
>  
>    const onBootstrapInstallCalled = new Promise(done => {
>      Services.obs.addObserver(function listener() {
>        Services.obs.removeObserver(listener, ADDON_NAME, false);
> -      ok(true, "Add-on was installed: " + ADDON_NAME);
> +      ok(true, "Add-on was re-installed: " + ADDON_NAME);

You can use `info(` instead of `ok(true,` here.

::: devtools/client/aboutdebugging/test/browser_addons_reload.js:52
(Diff revision 17)
>    });
>  
>    reloadButton.click();
>  
> -  const [disabledAddon] = yield onDisabled;
> -  ok(disabledAddon.name === ADDON_NAME,
> +  const [reloadedAddon] = yield onInstalled;
> +  ok(reloadedAddon.name === ADDON_NAME,

Please use `is(` rather than `ok(` here.

::: devtools/client/aboutdebugging/test/browser_addons_reload.js:61
(Diff revision 17)
>  
>    info("Uninstall addon installed earlier.");
> -  yield uninstallAddon(document, ADDON_ID, ADDON_NAME);
> +  const onUninstalled = promiseAddonEvent("onUninstalled");
> +  reloadedAddon.uninstall();
> +  const [uninstalledAddon] = yield onUninstalled;
> +  ok(uninstalledAddon.id === ADDON_ID,

Please us `is(` rather than `ok(`

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3902
(Diff revision 17)
>  
>      if (!addon.bootstrap) {
>        throw new Error("Only restartless (bootstrap) add-ons"
>                      + " can be temporarily installed:", addon.id);
>      }
> +    var newInstallReason = BOOTSTRAP_REASONS.ADDON_INSTALL;

Can we just call this `installReason`?

Also, please use `let` rather than `var`.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3924
(Diff revision 17)
>          // call its uninstall method
>          let newVersion = addon.version;
>          let oldVersion = oldAddon.version;
> -        let uninstallReason = Services.vc.compare(oldVersion, newVersion) < 0 ?
> -                              BOOTSTRAP_REASONS.ADDON_UPGRADE :
> -                              BOOTSTRAP_REASONS.ADDON_DOWNGRADE;
> +        let uninstallReason;
> +        if (Services.vc.compare(newVersion, oldVersion) >= 0) {
> +          uninstallReason = newInstallReason = BOOTSTRAP_REASONS.ADDON_UPGRADE;

Assignment as a side-effect isn't really acceptable in Toolkit code.

Can you just assign to `uninstallReason` here, and then assign `uninstallReason` to `installReason` after the if-else?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6975
(Diff revision 17)
>    get type() {
>      return getExternalType(addonFor(this).type);
>    },
>  
> +  get temporarilyInstalled() {
> +    return (addonFor(this)._installLocation == TemporaryInstallLocation);

Please remove unnecessary parens.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7368
(Diff revision 17)
> -
>        const addon = addonFor(this);
> -      const isReloadable = (!XPIProvider.enableRequiresRestart(addon) &&
> -                            !XPIProvider.disableRequiresRestart(addon));
> -      if (!isReloadable) {
> -        throw new Error(
> +
> +      if (!this.temporarilyInstalled) {
> +        logger.debug(`cannot reload add-on at ${addon._sourceBundle}`);
> +        throw new Error("only temporary add-ons can be reloaded");

Please capitalize the first words of both messages.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7371
(Diff revision 17)
> -      if (!isReloadable) {
> -        throw new Error(
> +        logger.debug(`cannot reload add-on at ${addon._sourceBundle}`);
> +        throw new Error("only temporary add-ons can be reloaded");
> -          "cannot reload add-on because it requires a browser restart");
>        }
>  
> -      this.userDisabled = true;
> +      logger.debug(`reloading add-on ${addon.id}`);

Please capitalize.

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:195
(Diff revision 17)
> +  },
> +
> +  checkEventIntegrity(event, version = undefined) {
> +    notEqual(event, undefined);
> +    if (version !== undefined) {
> +      equal(event && event.data.version, version);

Can we just add `event &&` to the if, and do `equal(event.data.version, ...)` here?

Although, if it comes to it, `notEqual` will throw if `event` is undefined, so even that check isn't necessary.

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:17
(Diff revision 17)
> +
> +const manifestSample = {
> +  id: "[e-mail]",
> +  version: "1.0",
> +  bootstrap: true,
> +  unpack: true,

Is this necessary?

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:27
(Diff revision 17)
> +  }],
> +};
> +
> +function* installAddon(fixtureName, addonID) {
>    yield promiseInstallAllFiles([do_get_addon(fixtureName)]);
>    restartManager();

`yield promiseRestartManager()`

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:48
(Diff revision 17)
> +  var receivedOnInstalling = false;
> +
> +  const onReload = new Promise(resolve => {
> +    const listener = {
> +      onUninstalling: () => {
> +        receivedOnUninstalling = true;

We should check the add-on ID in all of these callbacks.

Can we make all of these counters, rather than booleans?

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:76
(Diff revision 17)
> -  const [disabledAddon] = yield onDisabled;
> -  do_check_eq(disabledAddon.id, addonId);
> +  // Make sure reload() doesn't trigger uninstall events.
> +  equal(receivedOnUninstalled, false, "reload should not trigger onUninstalled");
> +  equal(receivedOnUninstalling, false, "reload should not trigger onUninstalling");
> +
> +  // Make sure reload() triggers install events, like an upgrade.
> +  equal(receivedOnInstalling, true, "reload should trigger onUninstalling");

"trigger onInstalling"

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:77
(Diff revision 17)
> -  do_check_eq(disabledAddon.id, addonId);
> +  equal(receivedOnUninstalled, false, "reload should not trigger onUninstalled");
> +  equal(receivedOnUninstalling, false, "reload should not trigger onUninstalling");
> +
> +  // Make sure reload() triggers install events, like an upgrade.
> +  equal(receivedOnInstalling, true, "reload should trigger onUninstalling");
> +  equal(receivedOnInstalled, true, "reload should trigger onUninstalled");

"trigger onInstalled"

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:79
(Diff revision 17)
> +
> +  // Make sure reload() triggers install events, like an upgrade.
> +  equal(receivedOnInstalling, true, "reload should trigger onUninstalling");
> +  equal(receivedOnInstalled, true, "reload should trigger onUninstalled");
>  
> -  const [enabledAddon] = yield onEnabled;
> +  tearDownAddon(addon);

`yield`

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:87
(Diff revision 17)
> +add_task(function* test_cannot_reload_permanent_addon() {
> +  const addon = yield installAddon(sampleAddon.name, sampleAddon.id);
> +
> +  yield Assert.rejects(addon.reload(), /only temporary add-ons can be reloaded/);
>  
>    tearDownAddon(addon);

`yield`

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:91
(Diff revision 17)
>  
>    tearDownAddon(addon);
>  });
>  
> -add_task(function* test_cannot_reload_addon_requiring_restart() {
> -  // This is a plain install.rdf add-on without a bootstrap script.
> +add_task(function* test_disabled_addon_can_be_enabled_after_reload() {
> +  restartManager();

`yield promiseRestartManager()`

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:96
(Diff revision 17)
> -  // This is a plain install.rdf add-on without a bootstrap script.
> -  const addon = yield getTestAddon("test_install1", "[e-mail]");
> +  restartManager();
> +  let tempdir = gTmpD.clone();
> +
> +  // Create an add-on with strictCompatibility which should cause it
> +  // to be appDisabled.
> +  const unpacked_addon = writeInstallRDFToDir(

Variables in toolkit code should be camel case, rather than underscored.

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:118
(Diff revision 17)
> -    caughtError = error;
> -  }
>  
> -  do_check_eq(
> -    caughtError && caughtError.message,
> -    "cannot reload add-on because it requires a browser restart");
> +  const reloadedAddon = yield promiseAddonByID(manifestSample.id);
> +  notEqual(reloadedAddon, null);
> +  equal(reloadedAddon.appDisabled, false);

Can we also test that changing the compat range and then reloading causes the add-on to be enabled?

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:121
(Diff revision 17)
> -  do_check_eq(
> -    caughtError && caughtError.message,
> -    "cannot reload add-on because it requires a browser restart");
> +  const reloadedAddon = yield promiseAddonByID(manifestSample.id);
> +  notEqual(reloadedAddon, null);
> +  equal(reloadedAddon.appDisabled, false);
>  
> +  unpacked_addon.remove(true);
>    tearDownAddon(addon);

`yield`

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:125
(Diff revision 17)
> +  unpacked_addon.remove(true);
>    tearDownAddon(addon);
>  });
>  
> -add_task(function* test_cannot_reload_app_disabled_addon() {
> -  // This add-on will be app disabled immediately.
> +add_task(function* test_manifest_changes_are_refreshed() {
> +  restartManager();

`yield promiseRestartManager()`

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:149
(Diff revision 17)
> -  do_check_eq(
> -    caughtError && caughtError.message,
> -    "cannot reload add-on because it is disabled by the application");
> +  const reloadedAddon = yield promiseAddonByID(manifestSample.id);
> +  notEqual(reloadedAddon, null);
> +  equal(reloadedAddon.name, "Test Bootstrap 1 (reloaded)");
> +
> +  unpacked_addon.remove(true);
> +  tearDownAddon(addon);

`yield`

Also, this should probably use `reloadedAddon` rather than `addon`. Same for the other tests.

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:153
(Diff revision 17)
> +  unpacked_addon.remove(true);
> +  tearDownAddon(addon);
> +});
> +
> +add_task(function* test_reload_fails_on_installation_errors() {
> +  restartManager();

`yield promiseRestartManager()`

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:178
(Diff revision 17)
> +  notEqual(oldAddon, null);
> +  equal(oldAddon.isActive, true);
> +  equal(oldAddon.name, "Test Bootstrap 1");
>  
> +  unpacked_addon.remove(true);
>    tearDownAddon(addon);

`yield`

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:10
(Diff revision 17)
>  const ID = "[e-mail]";
> +const sampleRDFManifest = {
> +  id: ID,
> +  version: "1.0",
> +  bootstrap: true,
> +  unpack: true,

Is this necessary?

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:28
(Diff revision 17)
>  
> +// Partial list of bootstrap reasons from XPIProvider.jsm
> +const BOOTSTRAP_REASONS = {
> +  ADDON_INSTALL: 5,
> +  ADDON_UPGRADE: 7,
> +  ADDON_DOWNGRADE: 8,

Importing XPIProvider is acceptable in AOM tests. Just do something like:

    const { BOOTSTRAP_REASONS } = Components.utils.import("resource://gre/modules/addons/XPIProvider.jsm");

Actually, that probably won't work, since the constant is lexical. Oh well..

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:289
(Diff revision 17)
> +
> +  const unpackedAddon = tempdir.clone();
> +  unpackedAddon.append(ID);
> +  do_get_file("data/test_temporary/bootstrap.js")
> +    .copyTo(unpackedAddon, "bootstrap.js");
> +

`BootstrapMonitor.clear(ID)`

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:327
(Diff revision 17)
> +
> +  const unpackedAddon = tempdir.clone();
> +  unpackedAddon.append(ID);
> +  do_get_file("data/test_temporary/bootstrap.js")
> +    .copyTo(unpackedAddon, "bootstrap.js");
> +

`BootstrapMonitor.clear(ID)`

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:365
(Diff revision 17)
> +
> +  const unpackedAddon = tempdir.clone();
> +  unpackedAddon.append(ID);
> +  do_get_file("data/test_temporary/bootstrap.js")
> +    .copyTo(unpackedAddon, "bootstrap.js");
> +

`BootstrapMonitor.clear(ID)`

Comment 33

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/50251/#review48517

> Can we just call this `installReason`?
> 
> Also, please use `let` rather than `var`.

Huh. I didn't think I could use let since it's assigned in a nested block. The test seems to pass though so I'll change it.

> Can we also test that changing the compat range and then reloading causes the add-on to be enabled?

How would that add any additional coverage? There is already a test that declares strict compatibility, asserts a disabled add-on, removes the strict compatibility declaration, reloads the add-on, and asserts an enabled add-on.

Comment 34

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/50251/#review48517

> How would that add any additional coverage? There is already a test that declares strict compatibility, asserts a disabled add-on, removes the strict compatibility declaration, reloads the add-on, and asserts an enabled add-on.

Sorry, you're right.

Comment 35

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/50251/#review48517

> `BootstrapMonitor.clear(ID)`

Whoops, good catch. I'm not sure how this test ever passed. It looks like BootstrapMonitor can't handle uninstalls (because of all its built-in integrity checks) so I had to ditch it.

Comment 36

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/17-18/

Comment 37

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/18-19/

Comment 38

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/19-20/

Comment 39

[Kris Maglione [:kmag]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

https://reviewboard.mozilla.org/r/50251/#review48955

Looks good. Thanks!

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:40
(Diff revisions 17 - 20)
> +        const targetAddonId = info.data.id;
> +        if (targetAddonId === addonId && info.event === expectedEvent) {
> +          resolve(info);
> +          Services.obs.removeObserver(observer);
> +        } else {
> +          Services.console.logStringMessage(

It would probably be better to use `do_print()` for this.

Comment 40

[Kumar McMillan [:kumar]]

Comment on attachment 8748385 [details]
MozReview Request: Bug 1269889 - make addon.reload() more like temp loading. r=kmag r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50251/diff/20-21/

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3100ff68652f

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3100ff68652f