1209341 - Allow loading unsigned restartless add-ons at runtime

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

We want to allow loading extensions for testing purposes. This would take the form of an API that allows loading an extension from a directory of code. The add-ons manager doesn't support the notion of temporary add-ons so there are two ways we could do this:

1. Make the add-ons manager support temporary add-ons. This could be doable by just adding the add-on object into XPIDatabase and making sure it never makes it to the json store. On shutdown when unloading the bootstrap script wipe it from the bootstrappedAddons pref.

2. Just bypass the add-ons manager and load the add-on directly by running its bootstrap script.

The second is probably the safest and quickest path to implementation but it would mean the add-ons manager APIs wouldn't return the add-on if asked and that might cause issues for some add-ons and debugging tools.

Comment 1

[Dave Townsend [:mossop]]

Attachment: splitting bootstrap calling out wip

This is something I was playing with a few months ago that splits out the code that loads a restartless add-on from XPIProvider so it can be called from elsewhere. It applied against revision 7cab8e8734c7 but could probably be rebased to a more current changeset without too much difficulty. It would be useful for implementing the second option above.

Comment 2

[Robert Helmer [:rhelmer]]

(In reply to Dave Townsend [:mossop] from comment #0)
> We want to allow loading extensions for testing purposes. This would take
> the form of an API that allows loading an extension from a directory of
> code. The add-ons manager doesn't support the notion of temporary add-ons so
> there are two ways we could do this:
> 
> 1. Make the add-ons manager support temporary add-ons. This could be doable
> by just adding the add-on object into XPIDatabase and making sure it never
> makes it to the json store. On shutdown when unloading the bootstrap script
> wipe it from the bootstrappedAddons pref.
> 
> 2. Just bypass the add-ons manager and load the add-on directly by running
> its bootstrap script.
> 
> The second is probably the safest and quickest path to implementation but it
> would mean the add-ons manager APIs wouldn't return the add-on if asked and
> that might cause issues for some add-ons and debugging tools.

After chatting w/ billm I think we probably want #1, it would be a shame for add-on devs to have to deal with debugging and other quirks that only happen in this mode, if we can help it. I'll start investigating, let me know if you disagree.

Comment 3

[Robert Helmer [:rhelmer]]

Just a quick update - I have a patch in progress for this, the approach I have am trying is to add a parameter to AddonManager.getInstallForFile() to indicate if this is a "temporary" add-on (rather than implementing a new API).

Temporary add-ons can be loaded from an arbitrary directory (this is the part I am working on now), and don't get written to the PREF_BOOTSTRAP_ADDONS pref, so will not be loaded after restart.

Comment 4

[Robert Helmer [:rhelmer]]

Mossop and I went over my WIP patch (https://gist.github.com/rhelmer/1543ca629f1c9846de5a) which is the approach from comment 3. This turns out to be more complicated than we'd hoped. Here are some notes from IRC on the new direction:

Mossop> So I think it might actually be a mistake to involve AddonInstall here at all, I'm not sure it adds anything and it probably complicates things. Really all we want is some function that gets passed a path and then instantiates the add-on there. Doing so directly is probably going to be more straightforward
11:28 AM <rhelmer> hm yeah I think you're probably right... so is that more like your original approach, have a way of calling the bootstrap.js functions more-or-less directly, but also have it show up in the add-on manager UI and allow debugging etc?
11:29 AM <rhelmer> I was trying to share as much of the existing code as possible so this would be easier to maintain, but I think there are enough exceptions for temporary add-ons that it's just making it more complex
11:29 AM <Mossop> No I don't think so. YOu can still do this all in XPIProvider, just have a function that parses the manifest, adds it to the database in some way that isn't persisted to disk and runs the appropriate bootstrap methods
11:30 AM <Mossop> The hard bit about this is making things work right if an add-on with the same ID is already installed
11:30 AM <rhelmer> yeah makes sense.. should I add a new public API in AddonManager.jsm for the UI to call?
11:30 AM <Mossop> Yeah
11:31 AM <rhelmer> hrm yeah I will look at the "add-on with same ID already installed" problem, I think I looked over the code for that, I'll get the simplistic case working first then see how hard it would be to look for and override existing add-ons
11:31 AM <rhelmer> thanks!
11:36 AM <Mossop> Yeah, somehow the existing add-on would have to be disabled (straightforward) but left in the bootstrappedAddons pref so that it starts up automatically again next time (ugh). If the existing add-on isn't restartless I'd just throw an exception and tell the user they must uninstall it
11:39 AM <rhelmer> hrm yeah I think having a separate function that does the disable but doesn't touch the pref would make sense, requires some duplication (maybe can refactor to make it a little nicer)

Comment 5

[Robert Helmer [:rhelmer]]

I have an approach based on comment 4 in my fork on https://bitbucket.org/rhelmer/fx-team

Has a basic test and should be , still needs to deal with the case where there's an add-on already installed with this ID - I still need to test to see what happens currently.

Comment 6

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

bug 1209341 - allow loading unsigned restartless add-ons at runtime

Comment 7

[Robert Helmer [:rhelmer]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

This seems to be working, wanted to get feedback before adding more tests and asking for a real review.

This approach attempts to disable any existing add-ons with the same ID, unless they are temporary in which case it will refuse to install (all the add-on developer has to do is restart and the current temporary install will go away).

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

https://reviewboard.mozilla.org/r/23593/#review21089

Definitely along the right lines. Realising that you need an install location for these temporary add-ons made me realise how we can easily restore any existing add-on on the next startup. If we use a fixed install location as I suggest below and instead of disabling the existing add-on we mark it as hidden (that will happen naturally when you tell the database about an add-on in a higher location that is visible). On the next startup the temporary install location says it has no add-ons installed which will conflict with what the database knows and so it will rebuild the set of visible add-ons making the existing add-on visible again and re-enabling it automatically.

With that I don't know if we actually need to hide the temporary add-on from either the database or the bootstrap pref, they'll just get cleaned up automatically. I think you'll also find that the existing uninstallAddon method will pretty much work-as-is even making the existing add-on visible again. You may also not need the isTemporary flag on AddonInternal and in the database, just comparing the install location to the temporary location should be enough.

::: toolkit/mozapps/extensions/AddonManager.jsm:2308
(Diff revision 1)
> +   * @param  aBrowser

This parameter is no longer there

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:649
(Diff revision 1)
> +    return;

This should return an explicit true or false. Not sure which we want it to be really, I could see allowing this in safe mode as it would let a developer run a test add-on in a clean environment but up to product to make that call.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:672
(Diff revision 1)
> -  if (aAddon._installLocation.name != KEY_APP_SYSTEM_DEFAULTS && mustSign(aAddon.type)) {
> +  if (!aAddon._installLocation.isTemporary) {

I think you want just aAddon.isTemporary here

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3767
(Diff revision 1)
> +    this.state = AddonManager.STATE_INSTALLING;

This looks leftover from trying to do this with AddonInstall.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3771
(Diff revision 1)
> +                                             AddonManager.SCOPE_USER);

Hmm right I hadn't considered the install location problem and I bet things break if we don't have one. I'd be concerned with just using a throw-away location like this, other code may attempt to get to the add-on through its location and fail etc.

Instead let's create a real install location that always exists along with the others. Rather than using DirectoryInstallLocation just use something like this:

    const TemporaryInstallLocation = {
      locked: false,
      name: "app-temporary":
      scope: SCOPE_USER,
      getAddonLocations(),
      isLinkedAddon: () => false,
      installAddon()
    }

When adding a new temporary add-on add it to the location so getAddonLocations returns the right set.

I think this actually makes restoring the old add-on easy peasy, see my notes at the top.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3774
(Diff revision 1)
> +    this.addon = syncLoadManifestFromFile(aDirectory, location);

Shouldn't be using this.addon here, that's the main XPIProvider object.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3784
(Diff revision 1)
> +                          + "temporarily installed");

We should replace the add-on in this case. Happy for that to be a follow-up issue though.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3788
(Diff revision 1)
> +          oldAddon.userDisabled = true;

You don't want to set userDisabled here, that means you'd have to activate it manually somehow or it will stay disabled. Instead if it is active call its "shutdown" function and then regardless call its "uninstall" function, basically what we do when installing a upgrade for an existing add-on. The call to addAddonMetadata below will end up marking this existing add-on as hidden by the new temporary add-on.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3821
(Diff revision 1)
> +      AddonManagerPrivate.callAddonListeners("onEnabled", wrapper);

I don't think you need to call the enabling/enabled listeners when the add-on is installed enabled already. You also call onInstalled a second time here.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6380
(Diff revision 1)
> +  },

I don't think you need to wrap this with a getter/setter. Just name the property isTemporary.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6970
(Diff revision 1)
> -    if (aAddon.type != "experiment") {
> +    if (aAddon.type != "experiment" && !aAddon.isTemporary) {

I'm not sure why this is necessary

Comment 9

[Robert Helmer [:rhelmer]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

bug 1209341 - allow loading unsigned restartless add-ons at runtime

Comment 10

[Robert Helmer [:rhelmer]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

bug 1209341 - allow loading unsigned restartless add-ons at runtime

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

https://reviewboard.mozilla.org/r/23593/#review21403

I think the testing needs some work here. For the test add-ons please use ones that have the BootstrapMonitor code in them (e.g. https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/addons/test_bootstrap1_1/bootstrap.js). BootstrapMonitor will check that you're calling the bootstrap methods correctly and then you can use it to check that the add-on you expect to be running actually is (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js#189).

I can think of a number of tests I'd like to see. It might make sense to do these in a new test_temporary.js file rather than add to test_bootstrap. Plus then you can use add_task and nice promisey functions rather than callback hell.

1. Install a temporary add-on with no existing add-on present. Restart and make sure it has gone away.
2. Install a temporary add-on with no existing add-on present. Uninstall it.
3. Install a temporary add-on over the top of an existing add-on. Restart and make sure the existing add-on comes back.
4. Install a temporary add-on over the top of an existing add-on. Uninstall it and make sure the existing add-on comes back.
5. Installing a temporary add-on over a non-restartless add-on should fail.
6. Installing a non-restartless add-on temporarily should fail.
7. Installing a temporary add-on when there is already a temporary add-on should fail.

We also need to test that the onExternalInstall and other events are sent as part of the install.

::: toolkit/mozapps/extensions/AddonManager.jsm:2316
(Diff revision 2)
> +    if (!(aDirectory.isDirectory()))

Actually, do we care if this is a directory? Any reason not to allow installing packed XPIs temporarily? I don't think there is any extra work involved.

::: toolkit/mozapps/extensions/content/extensions.js
(Diff revision 2)
> -

You've removed a few empty lines throughout this patch, please leave them in place.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:653
(Diff revision 2)
> +  // after restart..

Hrm, would definitely like to understand why this is before landing.

I suspect this is because you aren't adding TemporaryInstallLocation to installLocations and installLocationsByName here https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2502

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3767
(Diff revision 2)
> +  installTemporaryAddon: function XPI_installTemporaryAddon(aDirectory) {

The problem with this method is that the caller has no way to see when some things fail. For example you throw a couple of exceptions from inside async callbacks, those won't make it back to the caller.

Instead I think this method should return a promise and then instead of throwing reject the promise and resolve it at the end. Then the caller can just follow the promise to see the result.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3772
(Diff revision 2)
> +      function installTemporaryAddon_getVisibleAddonForID(oldAddon) {

No need to name this, just use an arrow function and it should all fit on the previous line.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3781
(Diff revision 2)
> +          XPIProvider.callBootstrapMethod(oldAddon, oldAddon._sourceBundle,

You need to check that the old add-on is bootstrapped before attempting to call it.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3785
(Diff revision 2)
> +          // TODO not sure if this is needed, aped from the uninstall process

The better example to follow is that of an add-on upgrade. You need to call the uninstall method as well as the shutdown method.

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5762

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3817
(Diff revision 2)
> +      AddonManagerPrivate.callAddonListeners("onEnabling", wrapper, false);

This call is unnecessary

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3820
(Diff revision 2)
> +      AddonManagerPrivate.callInstallListeners("onExternalInstall", null, wrapper);

Looking at the other example in tree looks like onExternalInstall should be called before onInstalling is sent. You should also pass the old add-on with it.

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/LightweightThemeManager.jsm#668

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7212
(Diff revision 2)
> +  _temporaryAddon: false,

No longer needed

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7906
(Diff revision 2)
> +  // FIXME some callers use _name, should they be fixed to use name instead?

Yes please, looks like it is just one case.

::: toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js:1404
(Diff revision 2)
> +    AddonManager.getAddonByID(TEST_ID, function(b1) {

Anytime you call one of these functions you have to assume that it might call its callback asynchronously. Your code here will behave a bit differently depending on that so put stuff that should happen after the checks in the callback. Same later.

::: toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js:1434
(Diff revision 2)
> +    }

You would need to cause some kind of failure if you don't see an exception here, but this will be changing to checking a promise anyway.

Comment 12

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1209341 - fix whitespace and remove unused _isTemporary bool

bug 1209341 - fix whitespace and remove unused _isTemporary bool

Comment 13

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1209341 - use name instead of _name

bug 1209341 - use name instead of _name

Comment 14

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1209341 - do not require temporary add-ons to be a directory, XPI is ok too

bug 1209341 - do not require temporary add-ons to be a directory, XPI is ok too

Comment 15

[Alexandre Poirot [:ochameau]]

Note that, while trying to integrate this feature into about:debugging,
I'm having the following exception:
  JavaScript error: resource://gre/modules/addons/XPIProvider.jsm, line 6616: TypeError: this._installLocation is undefined
  JavaScript error: resource://gre/modules/addons/XPIProvider.jsm, line 673: TypeError: aAddon._installLocation is undefined

And the addon can't be uninstalled.
Otherwise, on first try, the addon is correctly installed and run. But on next Firefox runs it doesn't.
(I apply all the attached patches from this bug against last mozilla-central)

Comment 16

[Robert Helmer [:rhelmer]]

(In reply to Alexandre Poirot [:ochameau] from comment #15)
> Note that, while trying to integrate this feature into about:debugging,
> I'm having the following exception:
>   JavaScript error: resource://gre/modules/addons/XPIProvider.jsm, line
> 6616: TypeError: this._installLocation is undefined
>   JavaScript error: resource://gre/modules/addons/XPIProvider.jsm, line 673:
> TypeError: aAddon._installLocation is undefined
> 
> And the addon can't be uninstalled.


Hmm I am not seeing that w/ the latest patch (or in the test), if I add a temporary add-on e.g. using devtools console on about:addons with AddonManager.installTemporaryAddon(FileUtils.File('/path/to/addon')), which is basically what you are doing in bug 1221141 AFAICT.

Find me in IRC (rhelmer) and let's figure it out.


> Otherwise, on first try, the addon is correctly installed and run. But on
> next Firefox runs it doesn't.
> (I apply all the attached patches from this bug against last mozilla-central)


Going away after Firefox shutdown is as intended, I see that it's not as clear in this bug but the requirement from bug 1209338 comment 0 is "lasts until Firefox quits".

Comment 17

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1209341 - use fat arrow instead of old-style inline function

bug 1209341 - use fat arrow instead of old-style inline function

Comment 18

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1209341 - call uninstall just like regular upgrade

bug 1209341 - call uninstall just like regular upgrade

Comment 19

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1209341 - call onExternalInstall before onInstalling

bug 1209341 - call onExternalInstall before onInstalling

Comment 20

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1209341 - move temporary add-on test to its own file

bug 1209341 - move temporary add-on test to its own file

Comment 21

[Robert Helmer [:rhelmer]]

https://reviewboard.mozilla.org/r/23593/#review21403

> Hrm, would definitely like to understand why this is before landing.
> 
> I suspect this is because you aren't adding TemporaryInstallLocation to installLocations and installLocationsByName here https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2502

Hm so I removed the check and then noticed this started passing when I split this out to a separate `test_temporary.js` - could be some unintended side-effect from an earlier test in `test_bootstrap.js`?

> The better example to follow is that of an add-on upgrade. You need to call the uninstall method as well as the shutdown method.
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5762

Hah I meant to say that it was aped from the upgrade process, not the uninstall process.. I will call the uninstall method as well.

Comment 22

[Robert Helmer [:rhelmer]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

bug 1209341 - allow loading unsigned restartless add-ons at runtime

Comment 23

[Robert Helmer [:rhelmer]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

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

Comment 24

[Robert Helmer [:rhelmer]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

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

Comment 25

[Dave Townsend [:mossop]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

https://reviewboard.mozilla.org/r/23593/#review22223

Looks like a lot of comments but most of this is pretty straightforward tweaks so I'd expect the next review to be an r+.

Please use the BootstrapMonitor.checkAddonInstalled and other checks in more cases in the test, basically everytime the expected add-on has changed to verify everything is being called right.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:651
(Diff revision 5)
> +  if (aAddon.location == KEY_APP_TEMPORARY)

Did you figure out why \_installLocation isn't set here, or is it fixed now?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2516
(Diff revision 5)
> +                                  [], AddonManager.SCOPE_TEMPORARY, false);

This doesn't look right, it isn't a directory install location. No need to call a function here you just have to add TemporaryInstallLocation to the installLocations and installLocationsByName lists.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3766
(Diff revision 5)
> +   *        The directory containing the unpacked add-on

directory or file.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3771
(Diff revision 5)
> +  installTemporaryAddon: function XPI_installTemporaryAddon(aDirectory) {

Optional but if you wrap this function with Task.async then it automatically returns a promise and would make calling loadManifestFromFile easier.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3772
(Diff revision 5)
> +    let addon = syncLoadManifestFromFile(aDirectory, TemporaryInstallLocation);

This function is async so use loadManifestFromFile instead of the sync version.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3774
(Diff revision 5)
> +    let self = this;

Using fat arrow functions means _this_ remains bound correctly so this should be unnecessary.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3779
(Diff revision 5)
> +          return reject([AddonManager.STATE_INSTALL_FAILED,

I don't think including STATE_INSTALL_FAILED is useful here, if you reject then it is always because the install failed. I would instead just reject with a string message.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3783
(Diff revision 5)
> +        if (oldAddon) {

This should probably be _if (oldAddon && oldAddon.active)_ since you don't need to disable an add-on that is already disabled. You should add a test for that case too.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3789
(Diff revision 5)
> +          } else if (!oldAddon.bootstrap) {

Prevailing code style is to put the else on a new line.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3807
(Diff revision 5)
> +            self.unloadBootstrapScope(addon.id);

I know they are technically the same but I'd rather you passed oldAddon.id here. It makes it a little clearer when skimming that this is unloading the old add-on code.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3838
(Diff revision 5)
> +        AddonManagerPrivate.callAddonListeners("onInstalling", wrapper);

onInstalling should be called before calling the install bootstrap method.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7085
(Diff revision 5)
> +    if (aAddon.location == KEY_APP_TEMPORARY)

Ditto on \_installLocation here.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7930
(Diff revision 5)
> +  uninstallAddon: (aAddon) => true,

Looks fine, it doesn't need to return anything though.

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:10
(Diff revision 5)
> +const { GlobalManager, Management } = Components.utils.import("resource://gre/modules/Extension.jsm", {});

This is unnecessary.

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:14
(Diff revision 5)
> +function promiseAddonStartup() {

This function is never used.

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:41
(Diff revision 5)
> +  });

You aren't checking that these functions are actually called.

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:62
(Diff revision 5)
> +  BootstrapMonitor.checkAddonNotStarted(ID, "1.0");

These functions don't take a version argument

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:75
(Diff revision 5)
> +  ]);

No need for Promise.all when you're just waiting on one promise.

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:134
(Diff revision 5)
> +add_task(function*() {

Please do the onExternalInstall checks in this function to make sure the existing add-on is passed.

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:233
(Diff revision 5)
> +  AddonManager.installTemporaryAddon(FileUtils.File(unpacked_addon.path)).then(

You need to yield to wait for this promise to complete (it will throw an exception on rejection) or you're racing with the following code. Same goes for other cases in here.

Comment 26

[Robert Helmer [:rhelmer]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

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

Comment 27

[Robert Helmer [:rhelmer]]

https://reviewboard.mozilla.org/r/23593/#review22223

> Did you figure out why \_installLocation isn't set here, or is it fixed now?

It looks like this was from me not setting adding to the `installLocations` and `installLocationsByName` lists correctly.

> Optional but if you wrap this function with Task.async then it automatically returns a promise and would make calling loadManifestFromFile easier.

I went ahead and did this.

Comment 28

[Dave Townsend [:mossop]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

https://reviewboard.mozilla.org/r/23593/#review22385

Real close but aside from a few minor things there is one thing in the tests that looks wrong and needs to be corrected.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3775
(Diff revisions 5 - 6)
>      return new Promise((resolve, reject) => {

Since you're in a task already you don't need this, just throw an exception instead of rejecting.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3780
(Diff revisions 5 - 6)
> +      XPIDatabase.getVisibleAddonForID(addon.id, (oldAddon) => {

Instead of needing a callback function you can do:

    let oldAddon = yield new Promise(resolve => XPIDatabase.getVisibleAddonForID(addon.id, resolve);

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3805
(Diff revisions 5 - 6)
> +            //let oldVersion = oldBootstrap.version;

This looks leftover

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3807
(Diff revisions 5 - 6)
> +            let uninstallReason = BOOTSTRAP_REASONS.ADDON_UPGRADE;

Should probably compare versions and use ADDON_UPGRADE or ADDON_DOWNGRADE as appropriate. See https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#3284

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3812
(Diff revisions 5 - 6)
> +                                              { newVersion: addon.version });

This can just be { newVersion } (yay new JS features!)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3815
(Diff revisions 5 - 6)
> +                                     "uninstall", uninstallReason, { newVersion: newVersion });

Same here

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3849
(Diff revisions 5 - 6)
> -        AddonManagerPrivate.callAddonListeners("onInstalled", wrapper);
> +                                               false);

You don't need the false argument here.

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:299
(Diff revisions 5 - 6)
> +

Do BootstrapMonitor checks for installed and started.

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:312
(Diff revisions 5 - 6)
> +

Do BootstrapMonitor checks for installed and not started

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:315
(Diff revisions 5 - 6)
> +

Do BootstrapMonitor checks for installed and started.

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:216
(Diff revision 6)
> +

Do BootstrapMonitor checks that the new add-on is installed and started.

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:230
(Diff revision 6)
> +  BootstrapMonitor.checkAddonNotStarted(ID);

This looks wrong, at this point the original add-on should be installed and started.

Comment 29

[Robert Helmer [:rhelmer]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

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

Comment 30

[Robert Helmer [:rhelmer]]

https://reviewboard.mozilla.org/r/23593/#review22385

> Since you're in a task already you don't need this, just throw an exception instead of rejecting.

Hm ok did this, code is much nicer - only problem is that when I throw an `Error` it doesn't seem to have a `message` on the other side, any thoughts on why?

Comment 31

[Dave Townsend [:mossop]]

(In reply to Robert Helmer [:rhelmer] from comment #30)
> https://reviewboard.mozilla.org/r/23593/#review22385
> 
> > Since you're in a task already you don't need this, just throw an exception instead of rejecting.
> 
> Hm ok did this, code is much nicer - only problem is that when I throw an
> `Error` it doesn't seem to have a `message` on the other side, any thoughts
> on why?

It appears to have a message property to me. Non-enumerable though so you don't see it in Object.keys.

Comment 32

[Dave Townsend [:mossop]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

https://reviewboard.mozilla.org/r/23593/#review22863

Woot. Sorry it took so many back and forths.

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:396
(Diff revision 7)
> +  });

I find using this pattern to be more readable:

    try {
      yield AddonManager.installTemporaryAddon(unpacked_addon);
      do_throw(...);
    }
    catch (err) {
      do_check_eq(...);
    }

Same for the other case like that. Also getting err.message appears to work for me.

Comment 33

[Robert Helmer [:rhelmer]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

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

Comment 34

[Robert Helmer [:rhelmer]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

Carrying over previous r+

Comment 35

[Alexandre Poirot [:ochameau]]

Did you run a try build, I recently pushed to try with the last patch and got exception in xpcshell tests?
Given that the other patch I pushed to try shouldn't break addons xpcshell tests, I suspect your patch to make that test to fail:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed878a111cf0
18:30:55  WARNING -  TEST-UNEXPECTED-FAIL | all-test-dirs.list:toolkit/mozapps/extensions/test/xpcshell/test_shutdown.js | xpcshell return code: 0
18:30:55     INFO -  TEST-INFO took 463ms
18:30:55     INFO -  >>>>>>>
18:30:55     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
18:30:55     INFO -  "AddonManager.getInstallForURL"
18:30:55     INFO -  "AddonManager.getInstallForFile"
18:30:55     INFO -  "AddonManager.getAddonByID"
18:30:55     INFO -  "AddonManager.getAddonBySyncGUID"
18:30:55     INFO -  "AddonManager.getAddonsByIDs"
18:30:55     INFO -  "AddonManager.getAddonsWithOperationsByTypes"
18:30:55     INFO -  "AddonManager.getAddonsByTypes"
18:30:55     INFO -  "AddonManager.getAllAddons"
18:30:55     INFO -  "AddonManager.getInstallsByTypes"
18:30:55     INFO -  "AddonManager.getAllInstalls"
18:30:55     INFO -  "AddonManager.isInstallEnabled"
18:30:55     INFO -  "AddonManager.isInstallAllowed"
18:30:55     INFO -  PROCESS | 13158 | 1447900255527	addons.manager	WARN	Deprecated API call, please pass a non-null nsIPrincipal instead of an nsIURI
18:30:55     INFO -  "AddonManager.installAddonsFromWebpage"
18:30:55     INFO -  PROCESS | 13158 | 1447900255528	addons.manager	WARN	Deprecated API call, please pass a non-null nsIPrincipal instead of an nsIURI
18:30:55     INFO -  "AddonManager.installTemporaryAddon"
18:30:55     INFO -  installTemporaryAddon threw an unexpected exception: [Exception... "aFile must be a nsIFile"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/AddonManager.jsm :: AMI_installTemporaryAddon :: line 2322"  data: no]

Comment 36

[Alexandre Poirot [:ochameau]]

It looks like we should at least put:
2187     if (!gStarted)
2188       throw Components.Exception("AddonManager is not initialized",
2189                                  Cr.NS_ERROR_NOT_INITIALIZED);
Early, in instalAddonsFromWebpage, or blacklist this function in test_shutdown.js


Note that another test fails: toolkit/mozapps/extensions/test/xpcshell/test_system_reset.js
I haven't tried to figure that out:
 0:01.83 TEST_STATUS: Thread-1 safe_mode_disabled FAIL [safe_mode_disabled : 44] false == true
    /mnt/desktop/gecko/obj-firefox-opt/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/test_system_reset.js:check_installed:44
    _run_next_test@/mnt/desktop/gecko/testing/xpcshell/head.js:1468:9
    do_execute_soon/<.run@/mnt/desktop/gecko/testing/xpcshell/head.js:660:9
    _do_main@/mnt/desktop/gecko/testing/xpcshell/head.js:208:5
    _execute_test@/mnt/desktop/gecko/testing/xpcshell/head.js:520:5
    @-e:1:1
 0:01.83 LOG: Thread-1 INFO exiting test
 0:01.83 LOG: Thread-1 ERROR Unexpected exception 2147500036
undefined
 0:01.84 LOG: Thread-1 INFO exiting test

Comment 37

[Alexandre Poirot [:ochameau]]

And about comment 15, where I reported issues. I tested again with latest patch and it works now. The addon no longer appears in about:addons/about:debugging when I restart firefox.
The only thing to report is this message:
  1447933835323	addons.xpi	WARN	Bootstrap state is invalid (missing add-ons: /mnt/desktop/gecko/devtools)
That happens on subsequent run of Firefox.

I installed a simple addon that is like this:
  /mnt/desktop/gecko/devtools/install.rdf
                             /chrome.manifest
                             /bootstrap.js
                             /folders registered by chrome.manifest file

Comment 38

[Robert Helmer [:rhelmer]]

(In reply to Alexandre Poirot [:ochameau] from comment #35)
> Did you run a try build, I recently pushed to try with the last patch and
> got exception in xpcshell tests?
> Given that the other patch I pushed to try shouldn't break addons xpcshell
> tests, I suspect your patch to make that test to fail:
>   https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed878a111cf0
(...)
> 18:30:55     INFO -  installTemporaryAddon threw an unexpected exception:
> [Exception... "aFile must be a nsIFile"  nsresult: "0x80070057
> (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
> resource://gre/modules/AddonManager.jsm :: AMI_installTemporaryAddon :: line
> 2322"  data: no]

Hm thanks there might be something different about the environment try runs in since this works locally.. I'll do my own try run right now with only my patch.


(In reply to Alexandre Poirot [:ochameau] from comment #37)
> And about comment 15, where I reported issues. I tested again with latest
> patch and it works now. The addon no longer appears in
> about:addons/about:debugging when I restart firefox.
> The only thing to report is this message:
>   1447933835323	addons.xpi	WARN	Bootstrap state is invalid (missing add-ons:
> /mnt/desktop/gecko/devtools)
> That happens on subsequent run of Firefox.
> 
> I installed a simple addon that is like this:
>   /mnt/desktop/gecko/devtools/install.rdf
>                              /chrome.manifest
>                              /bootstrap.js
>                              /folders registered by chrome.manifest file

Thanks for testing this!

I see this warning as well, but only if I call AddonManager.installTemporaryAddon() on a directory and not on a XPI file. I'll look into this.

Comment 39

[Robert Helmer [:rhelmer]]

(In reply to Alexandre Poirot [:ochameau] from comment #36)
> It looks like we should at least put:
> 2187     if (!gStarted)
> 2188       throw Components.Exception("AddonManager is not initialized",
> 2189                                  Cr.NS_ERROR_NOT_INITIALIZED);
> Early, in instalAddonsFromWebpage, or blacklist this function in
> test_shutdown.js


Ah ok now I see, I had mistaken this for a failure in test_temporary.js ... actually contrary to what I said in IRC I think blacklisting installTemporaryAddon() from test_shutdown.js is probably the right thing.

 
> Note that another test fails:
> toolkit/mozapps/extensions/test/xpcshell/test_system_reset.js
> I haven't tried to figure that out:
>  0:01.83 TEST_STATUS: Thread-1 safe_mode_disabled FAIL [safe_mode_disabled :
> 44] false == true
>    
> /mnt/desktop/gecko/obj-firefox-opt/_tests/xpcshell/toolkit/mozapps/
> extensions/test/xpcshell/test_system_reset.js:check_installed:44
>     _run_next_test@/mnt/desktop/gecko/testing/xpcshell/head.js:1468:9
>     do_execute_soon/<.run@/mnt/desktop/gecko/testing/xpcshell/head.js:660:9
>     _do_main@/mnt/desktop/gecko/testing/xpcshell/head.js:208:5
>     _execute_test@/mnt/desktop/gecko/testing/xpcshell/head.js:520:5
>     @-e:1:1
>  0:01.83 LOG: Thread-1 INFO exiting test
>  0:01.83 LOG: Thread-1 ERROR Unexpected exception 2147500036
> undefined
>  0:01.84 LOG: Thread-1 INFO exiting test

Comment 40

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #39)
> (In reply to Alexandre Poirot [:ochameau] from comment #36)
> >  
> > Note that another test fails:
> > toolkit/mozapps/extensions/test/xpcshell/test_system_reset.js
> > I haven't tried to figure that out:
> >  0:01.83 TEST_STATUS: Thread-1 safe_mode_disabled FAIL [safe_mode_disabled :
> > 44] false == true
> >    
> > /mnt/desktop/gecko/obj-firefox-opt/_tests/xpcshell/toolkit/mozapps/
> > extensions/test/xpcshell/test_system_reset.js:check_installed:44
> >     _run_next_test@/mnt/desktop/gecko/testing/xpcshell/head.js:1468:9
> >     do_execute_soon/<.run@/mnt/desktop/gecko/testing/xpcshell/head.js:660:9
> >     _do_main@/mnt/desktop/gecko/testing/xpcshell/head.js:208:5
> >     _execute_test@/mnt/desktop/gecko/testing/xpcshell/head.js:520:5
> >     @-e:1:1
> >  0:01.83 LOG: Thread-1 INFO exiting test
> >  0:01.83 LOG: Thread-1 ERROR Unexpected exception 2147500036
> > undefined
> >  0:01.84 LOG: Thread-1 INFO exiting test

This was a mistake in my patch (changed || to && in canRunInSafeMode() inadvertently) - fixing that makes all tests pass.

I'll push this to mozreview and also get a good try run.

I'd like to understand the warning that you pointed out.

Comment 41

[Robert Helmer [:rhelmer]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

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

Comment 42

[Dave Townsend [:mossop]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

https://reviewboard.mozilla.org/r/23593/#review23089

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:654
(Diff revisions 8 - 9)
> -  return aAddon._installLocation.name == KEY_APP_SYSTEM_DEFAULTS &&
> +  return aAddon._installLocation.name == KEY_APP_SYSTEM_DEFAULTS ||

*facepalm*

::: toolkit/mozapps/extensions/AddonManager.jsm:2314
(Diff revision 9)
> +    if (!(aFile instanceof Ci.nsIFile))

I think Alex is right and we should add a gStarted check here like https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#1479

::: toolkit/mozapps/extensions/test/xpcshell/test_shutdown.js:13
(Diff revision 9)
> -                "mapURIToAddonID", "shutdown"];
> +                "mapURIToAddonID", "shutdown", "installTemporaryAddon"];

This change shouldn't be necessary with the gStarted check.

Comment 43

[Robert Helmer [:rhelmer]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

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

Comment 44

[Dave Townsend [:mossop]]

Comment on attachment 8680193 [details]
MozReview Request: bug 1209341 - allow loading unsigned restartless add-ons at runtime r=mossop

https://reviewboard.mozilla.org/r/23593/#review23139

Comment 45

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #38)
> (In reply to Alexandre Poirot [:ochameau] from comment #37)
> >   1447933835323	addons.xpi	WARN	Bootstrap state is invalid (missing add-ons:
> I see this warning as well, but only if I call
> AddonManager.installTemporaryAddon() on a directory and not on a XPI file.
> I'll look into this.

It looks like the problem here is that the add-on gets into the pref that bootstrappedAddons gets loaded from, but not into XPIStates. It looks like this goes away on another restart, and there are comments in the relevant code about folding bootstrappedAddons into XPIStates which is probably the right way to fix this instead of a one-off exception.

I think this code is ready to land.

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/01d196350d24

Comment 47

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

https://hg.mozilla.org/mozilla-central/rev/01d196350d24

Comment 48

[Wladimir Palant]

Looks like the inline documentation in AddonManager.jsm is wrong now:

>+   * @param  aDirectory
>+   *         The directory of the add-on to be temporarily installed

The parameter is called aFile and the comment in XPIProvider.jsm says that it can be an XPI file as well, not just a directory (there the parameter is actually called aDirectory, for additional confusion points).

Comment 49

[Robert Helmer [:rhelmer]]

(In reply to Wladimir Palant from comment #48)
> Looks like the inline documentation in AddonManager.jsm is wrong now:
> 
> >+   * @param  aDirectory
> >+   *         The directory of the add-on to be temporarily installed
> 
> The parameter is called aFile and the comment in XPIProvider.jsm says that
> it can be an XPI file as well, not just a directory (there the parameter is
> actually called aDirectory, for additional confusion points).

Thanks! I filed bug 1231003 to fix that up.