1223171 - [driver] Ability to install/manage (restartless) addons on the fly

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

[Andrew Halberstadt [:ahal]]

Due to the addon signing requirement, it will soon no longer be possible to install unsigned addons by dumping them in the profile directory.

Instead, the addons will need to be restartless, and we'll need to install them at gecko runtime using the AddonManager api [1]. Given that most harnesses already use marionette in some capacity, and marionette can execute privileged JS.. I think this code should live somewhere in marionette.

After some irc discussion with AutomatedTester and ato, I'll implement a composable "Addons" class, rather than hanging it off the main marionette instance.

[1] https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1223171 - [marionette] add ability to install addons programatically, r=ato

Bug 1223171 - [marionette] add ability to install addons programatically, r=ato

This is needed to install unsigned addons in developer mode. A special pref that
can only be set at runtime will determine whether or not unsigned addons can be
installed. So unsigned addons required for testing will need to be restartless
and installed on the fly. The normal method of dropping the addon in the profile
folder will no longer work.

Comment 2

[Andrew Halberstadt [:ahal]]

Here's a working try run (though I made a few changes since):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9c78ff34d68

Also feel free to redirect review if appropriate.

Comment 3

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8685624 [details]
MozReview Request: Bug 1223171 - [marionette] add ability to install addons programatically, r=ato

https://reviewboard.mozilla.org/r/24861/#review22445

::: testing/marionette/client/marionette/tests/unit/test_addons.py:19
(Diff revision 1)
> +        self.addon_path = os.path.join(here, 'addons', 'mn-restartless.xpi')

Skip the extra _addons/_ directory.

::: testing/marionette/client/marionette/tests/unit/test_addons.py:22
(Diff revision 1)
> +    @property
> +    def all_addon_ids(self):
> +        with self.marionette.using_context('chrome'):
> +            addons = self.marionette.execute_async_script("""
> +              Components.utils.import("resource://gre/modules/AddonManager.jsm");
> +              AddonManager.getAllAddons(function(addons){
> +                let ids = addons.map(function(x) {
> +                  return x.id;
> +                });
> +                marionetteScriptFinished(ids);
> +              });
> +            """)

Do you think this would make sense to expose in the `Addons` class?

::: testing/marionette/driver/marionette_driver/addons.py:9
(Diff revision 1)
> +ADDON_INSTALL_ERRORS = {
> +    -1: "ERROR_NETWORK_FAILURE: A network error occured.",
> +    -2: "ERROR_INCORECT_HASH: The downloaded file did not match the expected hash.",
> +    -3: "ERROR_CORRUPT_FILE: The file appears to be corrupt.",
> +    -4: "ERROR_FILE_ACCESS: There was an error accessing the filesystem.",
> +    -5: "ERROR_SIGNEDSTATE_REQUIRED: The addon must be signed and isn't.",
> +}

Exclude this from the exported set of symbols by defining `__all__ = ["Addons"]`.

::: testing/marionette/driver/marionette_driver/addons.py:36
(Diff revision 1)
> +        def on_restart():
> +            self.signature_required = self._signature_required
> +        self._mn.restart_handlers.append(on_restart)

I don't get this.  Is the signature_required getter/setter replaced by a bool value on restart?

::: testing/marionette/driver/marionette_driver/addons.py:52
(Diff revision 1)
> +    def install(self, path):
> +        """Install an addon.
> +
> +        If the addon is restartless, it can be used right away. Otherwise
> +        a restart using :func:`~marionette_driver.marionette.Marionette.restart`
> +        will be needed.
> +
> +        :param path: A file path to the extension to be installed.
> +        """

Perhaps it would be possible to return the addon ID from this function, so it coudl be reused in `uninstall`?

::: testing/marionette/driver/marionette_driver/addons.py:100
(Diff revision 1)
> +        python.

s/python/Python/

::: testing/marionette/driver/marionette_driver/addons.py:102
(Diff revision 1)
> +        :param addon_id: The addon id string to uninstall.

s/id/ID/

::: testing/marionette/driver/marionette_driver/errors.py:179
(Diff revision 1)
> +    status = "addon install error"

This should only be necessary for looking up errors that come from the Marionette server.

Although I do see you might have to change `by_string` below…

This is, by the way, very good. (-:

Comment 4

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/24861/#review22445

> Do you think this would make sense to expose in the `Addons` class?

Yeah, I think it could potentially be useful. My aim was to keep the Addons class as minimal as possible to begin with (as I don't need this particular functionality), and then if stuff like this was needed in the future it could be added in follow-up bugs. But I can add it now if you like, what do you think?

One thing I discovered, is that marionette is not capable of sending the actual addon objects over the wire. It results in an infinite recursion error.

> I don't get this.  Is the signature_required getter/setter replaced by a bool value on restart?

self.signature_required is an @property decorator that actually calls self.marionette.set_pref(...) when you set it.

The problem is that when we restart the browser (i.e delete the marionette session, and create a new one), all of a sudden all of the changes that people have made using that old session are lost. This includes prefs and non-sandboxed executed scripts. In otherwords, the state held in various python tools using marionette (like the Addons class, or test harnesses) is out of sync from the state on the server. I figured these callbacks could be a decent enough way for users of marionette client to ensure any necessary state is synced back up on restart.

To see what I mean, remove this callback and add a self.marionette.restart() call to the test, right after the self.signature_required = True line. The test will fail as the pref is no longer set.

Comment 5

[Andrew Halberstadt [:ahal]]

(In reply to Andrew Halberstadt [:ahal] from comment #4)
> To see what I mean, remove this callback and add a self.marionette.restart()
> call to the test, right after the self.signature_required = True line. The
> test will fail as the pref is no longer set.

Er, "right after the self.signature_required = False line"

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8685624 [details]
MozReview Request: Bug 1223171 - [marionette] add ability to install addons programatically, r=ato

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

Comment 7

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/24861/#review22445

> This should only be necessary for looking up errors that come from the Marionette server.
> 
> Although I do see you might have to change `by_string` below…

If "fixed" this by moving the exception into addons.py. If errors.py contains only server generated errors, maybe it isn't worth having that class live there. Is that ok?

Comment 8

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/24861/#review22445

> Yeah, I think it could potentially be useful. My aim was to keep the Addons class as minimal as possible to begin with (as I don't need this particular functionality), and then if stuff like this was needed in the future it could be added in follow-up bugs. But I can add it now if you like, what do you think?
> 
> One thing I discovered, is that marionette is not capable of sending the actual addon objects over the wire. It results in an infinite recursion error.

Makes sense.

The `executeScript` return value marshaling has some serious, known problems.  It's one of the things we aim to fix by writing a test suite for the WebDriver specification.

I think the specific case you're running into is that Marionette tries to serialise all the properties in the prototype chain of an object, and not just the own properties.

> self.signature_required is an @property decorator that actually calls self.marionette.set_pref(...) when you set it.
> 
> The problem is that when we restart the browser (i.e delete the marionette session, and create a new one), all of a sudden all of the changes that people have made using that old session are lost. This includes prefs and non-sandboxed executed scripts. In otherwords, the state held in various python tools using marionette (like the Addons class, or test harnesses) is out of sync from the state on the server. I figured these callbacks could be a decent enough way for users of marionette client to ensure any necessary state is synced back up on restart.
> 
> To see what I mean, remove this callback and add a self.marionette.restart() call to the test, right after the self.signature_required = True line. The test will fail as the pref is no longer set.

If the signature is the only preference needed, maybe making this a mandatory argument on installation is an option?

> If "fixed" this by moving the exception into addons.py. If errors.py contains only server generated errors, maybe it isn't worth having that class live there. Is that ok?

WFM

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8685624 [details]
MozReview Request: Bug 1223171 - [marionette] add ability to install addons programatically, r=ato

https://reviewboard.mozilla.org/r/24861/#review22469

Comment 10

[Andrew Halberstadt [:ahal]]

New try run with latest changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27946e2b1cda

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62968e4afd68

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/62968e4afd68

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Moving bugs for Marionette client due to component changes.