Closed Bug 1383931 Opened 7 years ago Closed 7 years ago

Accept addons as base64 in /moz/addon/install

Categories

(Testing :: geckodriver, defect)

defect
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: juangj, Assigned: juangj)

References

()

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36

Steps to reproduce:

Originally filed at https://github.com/mozilla/geckodriver/issues/829.

The /moz/addon/install command accepts a path to the addon, e.g.,
{
  "path": "/tmp/my-addon.xpi",
  "temporary": true
}

For use cases where Geckodriver is running on a remote machine, it would be more convenient to be able to give a base64 blob, which Geckodriver would presumably decode and write to a temporary file, like it does with base64-encoded profiles.
Attachment #8889666 - Flags: review?(ato)
Assignee: nobody → juangj
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8889666 [details]
Bug 1383931 - Accept base64-encoded addons in the addon install command.

https://reviewboard.mozilla.org/r/160732/#review166178

Sorry for the delay in reviewing this, but I have been on holiday.  This is a great change, and thanks for making it!  It’s also great that you’ve written tests for it.

I have a few minor comments it would be great if you could address.  I’ve also triggered a try run.

::: commit-message-bf463:1
(Diff revision 1)
> +Bug 1383931 - Accept base64-encoded addons in the addon install command.

Can you also add an entry in testing/geckodriver/CHANGES.md describing the change?

::: commit-message-bf463:3
(Diff revision 1)
> +Bug 1383931 - Accept base64-encoded addons in the addon install command.
> +
> +This allows tests that use Geckodriver remotely to more easily install addons. The base64 blob is written to a temporary file before being passed on to Marionette.

Wrap this at ~72 characters.

::: testing/geckodriver/src/marionette.rs:276
(Diff revision 1)
> +                let addon_path = env::temp_dir().as_path()
> +                    .join(format!("addon-{}.xpi", Uuid::new_v4()));
> +                let mut addon_file = try!(File::create(&addon_path));
> +                let addon_buf = try!(s.from_base64());
> +                try!(addon_file.write(addon_buf.as_slice()));

I think the right thing to do here is to pull in tempdir, which is already vendored under third_party/rust/tempdir: https://doc.rust-lang.org/tempdir/tempdir/index.html

::: testing/geckodriver/src/marionette.rs:294
(Diff revision 1)
> +            Some(x) => Some(try_opt!(x.as_string(),
> +                                     ErrorStatus::InvalidArgument,
> +                                     "'path' is not a string").to_string()),
> +            None => None,
> +        };
> +        if (base64.is_none() && path.is_none()) || (base64.is_some() && path.is_some()) {

if-conditions don’t have to be wrapped in parenthesis.

::: testing/geckodriver/src/marionette.rs:297
(Diff revision 1)
> +            None => None,
> +        };
> +        if (base64.is_none() && path.is_none()) || (base64.is_some() && path.is_some()) {
> +            return Err(WebDriverError::new(
> -            ErrorStatus::InvalidArgument,
> +                ErrorStatus::InvalidArgument,
> -            "'path' is not a string").to_string();
> +                "must specify exactly one of 'path' and 'addon'"));

s/must/Must/
Attachment #8889666 - Flags: review?(ato) → review-
Comment on attachment 8889666 [details]
Bug 1383931 - Accept base64-encoded addons in the addon install command.

https://reviewboard.mozilla.org/r/160732/#review166178

> I think the right thing to do here is to pull in tempdir, which is already vendored under third_party/rust/tempdir: https://doc.rust-lang.org/tempdir/tempdir/index.html

I mentioned this in the Github PR but neglected to move the comments over here: The problem I'm having with using tempdir is that the directory is deleted when the `TempDir` reference is dropped. We would need to stash a reference to it somewhere until we receive the response from Marionette, and I'm not sure where to do that -- it's not appropriate to just stick it in the `AddonInstallParameters`, because the file reference isn't cloneable.

From experimentaion, I *think* it's safe to delete the temporary file after we receive the Marionette response. But we could also keep the file around for the lifetime of the session, if there's an easier way to accomplish that.
Comment on attachment 8889666 [details]
Bug 1383931 - Accept base64-encoded addons in the addon install command.

https://reviewboard.mozilla.org/r/160732/#review166178

> if-conditions don’t have to be wrapped in parenthesis.

I made a mistake with regards to the parenthesis here in misreading how you used them.  You are grouping to things together and ending the block before ||: I misread it as the whole block was wrapped in one group.

Can you revert this to how you wrote it originally?  It now fails the build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=10e0538fb251&selectedJob=117999148
Comment on attachment 8889666 [details]
Bug 1383931 - Accept base64-encoded addons in the addon install command.

https://reviewboard.mozilla.org/r/160732/#review166178

> I mentioned this in the Github PR but neglected to move the comments over here: The problem I'm having with using tempdir is that the directory is deleted when the `TempDir` reference is dropped. We would need to stash a reference to it somewhere until we receive the response from Marionette, and I'm not sure where to do that -- it's not appropriate to just stick it in the `AddonInstallParameters`, because the file reference isn't cloneable.
> 
> From experimentaion, I *think* it's safe to delete the temporary file after we receive the Marionette response. But we could also keep the file around for the lifetime of the session, if there's an easier way to accomplish that.

Right, sorry for missing that.  I see your point regarding the lifetime of TempDir: when it is dropped from the local scope the temporary directory is deleted before the .xpi file is installed by Marionette.

I think the correct solution is to keep the reference around until the response arrives, but it’s fine to fix this later.  I’ll resolve this issue.
Comment on attachment 8889666 [details]
Bug 1383931 - Accept base64-encoded addons in the addon install command.

https://reviewboard.mozilla.org/r/160732/#review166802
Attachment #8889666 - Flags: review?(ato) → review-
Comment on attachment 8889666 [details]
Bug 1383931 - Accept base64-encoded addons in the addon install command.

https://reviewboard.mozilla.org/r/160732/#review166178

> Right, sorry for missing that.  I see your point regarding the lifetime of TempDir: when it is dropped from the local scope the temporary directory is deleted before the .xpi file is installed by Marionette.
> 
> I think the correct solution is to keep the reference around until the response arrives, but it’s fine to fix this later.  I’ll resolve this issue.

No need to apologize, I should have written it here, in the context of the review. (Actually I didn't read carefully, so I didn't realize it was Henrik who had replied on that PR, and not you.)

OK, I'm happy for us to work out a correct way of doing this in a follow-up change. It feels like it may require some non-trivial refactoring.

> I made a mistake with regards to the parenthesis here in misreading how you used them.  You are grouping to things together and ending the block before ||: I misread it as the whole block was wrapped in one group.
> 
> Can you revert this to how you wrote it originally?  It now fails the build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=10e0538fb251&selectedJob=117999148

Oh no, I apparently didn't read it either. And I obviously didn't run the tests after making the last change... oops! Sorry about that.
Comment on attachment 8889666 [details]
Bug 1383931 - Accept base64-encoded addons in the addon install command.

https://reviewboard.mozilla.org/r/160732/#review167066
Attachment #8889666 - Flags: review?(ato) → review+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/efb8a580888a
Accept base64-encoded addons in the addon install command. r=ato
https://hg.mozilla.org/mozilla-central/rev/efb8a580888a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.