Closed Bug 1298025 Opened 4 years ago Closed 4 years ago

Expose installing temporary extensions as an extension command in Marionette

Categories

(Testing :: Marionette, defect)

Version 3
defect
Not set
normal

Tracking

(firefox52 fixed, firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: standard8, Assigned: ato)

References

Details

Attachments

(1 file)

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.
OS: Unspecified → All
Hardware: Unspecified → All
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.
Flags: needinfo?(ahalberstadt)
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.
Flags: needinfo?(ahalberstadt)
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.
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
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment on attachment 8785277 [details]
Bug 1298025 - Move addon installation internally to Marionette

https://reviewboard.mozilla.org/r/74546/#review72506
Attachment #8785277 - Flags: review?(dburns) → 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.
Attachment #8785277 - Flags: review?(ahalberstadt) → review+
(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?
Flags: needinfo?(dtownsend)
Flags: needinfo?(dtownsend) → needinfo?(rhelmer)
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
Flags: needinfo?(rhelmer)
I suppose it would be best to rewrite this using the AddonManager.installTemporaryAddon promise rhelmer refers to.
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.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/260e64c44531
Move addon installation internally to Marionette; r=ahal,automatedtester
https://hg.mozilla.org/mozilla-central/rev/260e64c44531
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Duplicate of this bug: 1310628
It would be great to have this test-only change in the upcoming ESR. Requesting uplift for Aurora.
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.