Mark Banner (:standard8) Reporter
	Description • 8 years ago

Marionette has a function whereby you can install a temporary add-on in a profile. 

https://dxr.mozilla.org/mozilla-central/rev/01748a2b1a463f24efd9cd8abad9ccfd76b037b8/testing/marionette/client/marionette_driver/addons.py#41

This is very useful for testing unsigned add-ons in official release builds. Hence it would be good if we could make it available to selenium/geckodriver setups so that users don't have to duplicate the temporary install scripts manually (and a specific API is likely to be better for visibility of the possibility).

:ato mentioned on irc this could be done by making the function an extension command within Marionette.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 1 • 8 years ago

Cc’ing ahal as he did the original implementation of the chrome scripts that currently live within addons.py.

ahal, do you think this is a reasonable thing to do?  I suspect we can remove the internal implementation in addons.py as a result of this so that it can be shared across different Marionette clients.

	Andrew Halberstadt [:ahal]
	Comment 2 • 8 years ago

Just to make sure I'm on the same page, as I'm not sure what an "extension command" is.. the proposal here is to move the JS logic to the server (as some sort of extension?) and provide a built-in API for clients to call? That sounds like a good idea to me. I think it was just done with execute_script originally because it was easier and could be proven out first.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 8 years ago

I should’ve explained what an ‘extension command’ is more carefully.  It is correct that it is just another command in the server, except that it will be exposed by geckodriver under a vendor prefixed namespace such as /session/{session id}/moz/addon.  It is described in more detail in the WebDriver specification:

    http://w3c.github.io/webdriver/webdriver-spec.html#protocol-extensions

In terms of implementation, it is no different to any other Marionette command.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 4 • 8 years ago

My current proposal for API is:

POST /session/{session id}/moz/addon
body:       {addon: <Base64 encoded addon file>, temporary: <boolean>}
return:     {value: <addon id string>}
Marionette: Marionette:installAddon

DELETE /session/{session id}/moz/addon/{addon id}
body:       {}
return:     {}
Marionette: Marionette:uninstallAddon

	David Burns :automatedtester
	Comment 6 • 8 years ago
mozreview-review

Comment on attachment 8785277 [details]
Bug 1298025 - Move addon installation internally to Marionette

https://reviewboard.mozilla.org/r/74546/#review72506

	Andrew Halberstadt [:ahal]
	Comment 7 • 8 years ago
mozreview-review

Comment on attachment 8785277 [details]
Bug 1298025 - Move addon installation internally to Marionette

https://reviewboard.mozilla.org/r/74546/#review73542

Sorry for the delay. Meant to do this on Monday but forgot.

This looks good to me! I skimmed the driver.js changes though, as I'm not very familiar with that area.

::: testing/marionette/addon.js:79
(Diff revision 1)
> +        }
> +        aInstall.addListener(listener);
> +        aInstall.install();
> +      });
> +    } else {
> +      AddonManager.addAddonListener(listener);

Just an fyi.. The addAddonListener object here (only used by temporary addons) will only call "onInstalled". In other words, if installation of a temporary addon fails for some reason, this promise won't get resolved. I tried fairly hard to find a way to detect if installation failed, but I couldn't figure out how to do it.

I only mention it because now that this code lives server side, you might want higher standards. I assume the marionette call will eventually timeout, but you might want a smaller timeout here with a customized error message. Fwiw, I've never seen a temporary addon fail to install.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 8 • 8 years ago

(In reply to Andrew Halberstadt [:ahal] from comment #7)
> Just an fyi.. The addAddonListener object here (only used by temporary
> addons) will only call "onInstalled". In other words, if installation of a
> temporary addon fails for some reason, this promise won't get resolved. I
> tried fairly hard to find a way to detect if installation failed, but I
> couldn't figure out how to do it.
> 
> I only mention it because now that this code lives server side, you might
> want higher standards. I assume the marionette call will eventually timeout,
> but you might want a smaller timeout here with a customized error message.
> Fwiw, I've never seen a temporary addon fail to install.

Dave, can you give an advice maybe?

	Robert Helmer [:rhelmer]
	Comment 9 • 8 years ago

AddonManager.installTemporaryAddon() returns a promise, you shouldn't need to install a listener. The promise rejects if installation fails: http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/toolkit/mozapps/extensions/AddonManager.jsm#2313-2320

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 10 • 8 years ago

I suppose it would be best to rewrite this using the AddonManager.installTemporaryAddon promise rhelmer refers to.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 12 • 8 years ago

After closer examination I think the current approach is better as we need to resolve the `addon.install` promise with the ID of the installed addon, which the install listener gives us.  In the case of an error we need to convert it and it’s better to avoid code duplication by doing this once in the install listener.

	Pulsebot
	Comment 15 • 8 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/260e64c44531
Move addon installation internally to Marionette; r=ahal,automatedtester

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/260e64c44531

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 18 • 7 years ago

It would be great to have this test-only change in the upcoming ESR. Requesting uplift for Aurora.

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/40bbf975f409