Expose installing temporary extensions as an extension command in Marionette

RESOLVED FIXED in Firefox 52

Status

Testing
Marionette
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: ato)

Tracking

Version 3
mozilla53
Points:
---

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
(Assignee)

Updated

2 years ago
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Comment 1

2 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.
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)
(Assignee)

Comment 3

2 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.
(Assignee)

Comment 4

2 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
(Assignee)

Updated

2 years ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
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 7

2 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.
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)
(Assignee)

Comment 10

2 years ago
I suppose it would be best to rewrite this using the AddonManager.installTemporaryAddon promise rhelmer refers to.
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

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

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/260e64c44531
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
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.
status-firefox52: --- → affected
Whiteboard: [checkin-needed-aurora]

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/40bbf975f409
status-firefox52: affected → fixed
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.