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

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.

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

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.

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

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?

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 on attachment 8685624 [details] MozReview Request: Bug 1223171 - [marionette] add ability to install addons programatically, r=ato https://reviewboard.mozilla.org/r/24861/#review22469

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