Closed Bug 1306703 Opened 4 years ago Closed 4 years ago

Mozbase issues with Android 7.0

Categories

(Testing :: Mozbase, defect)

Version 3
Unspecified
Android
defect
Not set

Tracking

(firefox51 fixed, firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: droeh, Assigned: bc)

References

Details

Attachments

(5 files, 6 obsolete files)

When trying to use mozregression on an Android 7 device, I get an "Unable to install" error. There seems to be two issues here:

First, before even trying to install, we check to make sure ls is available and functioning on the device. We do this right now by calling |/system/bin/ls /| and checking the return code. On 7, this gives a return code of 1 because of permissions; I can switch this to checking |/system/bin/ls /sdcard| successfully on my device, but I'm not 100% sure that that will work on all devices.

Second, we somehow are not getting the "Success" output when the install happens successfully -- as it does! -- and thus are printing the unable to install error and giving up. I'm not quite sure what's going on here yet.
You may be the first to use mozregression or any mozbase component against Android 7. autophone usually leads the way for test automation on new devices, but I think it is not using anything newer than Android 6.

In general, our mozdevice code works best in a shell running as root.
(In reply to Geoff Brown [:gbrown] from comment #1)
> In general, our mozdevice code works best in a shell running as root.

True, though I've put some effort into making sure it works for the non-rooted case, especially for mozregression. If the subset of mozdevice that we use for mozregression doesn't work on any particular Android platform, we should fix that.
Attached patch Update mozbase for Android N (obsolete) — Splinter Review
Here's a quick patch which is basically what I did to get mozregression working on my Android N device. It includes the fix I mentioned in comment 1, along with a fix for missing the "Success" message on install.

The "Success" message was being missed because adb now distinguishes between stdout and stderr[0]. I wouldn't be surprised if there was other stuff broken on N because of this. My fix was quick and dirty: in command_output, just get the stdout and stderr and concatenate them, instead of only getting the stdout -- this ensures we get all the output, but it potentially is in a different order than we would have gotten on prior versions of Android.

[0]: https://plus.google.com/u/0/+ElliottHughes/posts/fHa6Yg3bE1W
Attachment #8798051 - Flags: review?(gbrown)
Attachment #8798051 - Flags: review?(gbrown) → review+
Attached patch Update mozbase for Android N (obsolete) — Splinter Review
Rather than landing that patch, do you mind reviewing this one? I think I've handled things more nicely here:

To determine if /system/bin/ls (or /system/xbin/ls) exists, I added a function called shell_exists which essentially is similar to shell_bool but returns true if the error code is anything other than 127, so we don't need to worry about /sdcard not existing on some platforms.

To handle the stderr output, I updated ADBProcess.__init__ to forward stderr to subprocess.STDOUT, so we now should get the same output in the same order as we got on previous versions of Android.
Assignee: nobody → droeh
Attachment #8798051 - Attachment is obsolete: true
Attachment #8798467 - Flags: review?(gbrown)
Comment on attachment 8798467 [details] [diff] [review]
Update mozbase for Android N

Review of attachment 8798467 [details] [diff] [review]:
-----------------------------------------------------------------

I feel uneasy about shell_exists() and particularly the reliance on the 127 exit code...but I'm not sure I have a better solution.

Bob may have a better idea...
Attachment #8798467 - Flags: review?(gbrown) → review?(bob)
I saw that and wasn't thrilled. I'll have a closer look this evening.
It seems fairly safe based on http://tldp.org/LDP/abs/html/exitcodes.html -- although we might want to generalize it to returning false if we get an error code of 126 or above to deal with possible permissions issues.
Comment on attachment 8798467 [details] [diff] [review]
Update mozbase for Android N

>diff --git a/testing/mozbase/mozdevice/mozdevice/adb.py b/testing/mozbase/mozdevice/mozdevice/adb.py
>--- a/testing/mozbase/mozdevice/mozdevice/adb.py
>+++ b/testing/mozbase/mozdevice/mozdevice/adb.py
>@@ -24,17 +24,17 @@ class ADBProcess(object):
>         self.stderr_file = tempfile.TemporaryFile()
>         #: boolean indicating if the command timed out.
>         self.timedout = None
>         #: exitcode of the process.
>         self.exitcode = None
>         #: subprocess Process object used to execute the command.
>         self.proc = subprocess.Popen(args,
>                                      stdout=self.stdout_file,
>-                                     stderr=self.stderr_file)
>+                                     stderr=subprocess.STDOUT)

I'm not sure how I feel about this change as is while leaving self.stderr_file littered around the code.

> 
>     @property
>     def stdout(self):
>         """Return the contents of stdout."""
>         if not self.stdout_file or self.stdout_file.closed:
>             content = ""
>         else:
>             self.stdout_file.seek(0, os.SEEK_SET)
>@@ -588,19 +588,19 @@ class ADBDevice(ADBCommand):
>         except ADBError:
>             self._logger.debug("Check for su 0 failed")
> 
>         self._mkdir_p = None
>         # Force the use of /system/bin/ls or /system/xbin/ls in case
>         # there is /sbin/ls which embeds ansi escape codes to colorize
>         # the output.  Detect if we are using busybox ls. We want each
>         # entry on a single line and we don't want . or ..
>-        if self.shell_bool("/system/bin/ls /", timeout=timeout):
>+        if self.shell_exists("/system/bin/ls /", timeout=timeout):
>             self._ls = "/system/bin/ls"
>-        elif self.shell_bool("/system/xbin/ls /", timeout=timeout):
>+        elif self.shell_exists("/system/xbin/ls /", timeout=timeout):
>             self._ls = "/system/xbin/ls"
>         else:
>             raise ADBError("ADBDevice.__init__: ls not found")
>         try:
>             self.shell_output("%s -1A /" % self._ls, timeout=timeout)
>             self._ls += " -1A"

Would checking /data/local/tmp be sufficient instead of checking the root / ? If so, we would not need the shell_exists method at all would we?

We would also want to update version_codes with N / Nougat.

To keep the changes minimal, could you try a version of your patch where:

do not use shell_exists but instead try ls /data/local/tmp instead of ls / ?

update version_codes.py with N

in command_output/shell_output test the version of android and if it is N or later, return the concatenation of stdout/stderr as the output?

Nougat makes a number of changes to adb and the shell that look very interesting but I don't have a device available. I have ordered a device that I can install N and will hopefully have it relatively soon. I'd definitely like to see what we can take advantage of in N and later but would like to do it as a coherent update rather than an ad hoc approach.
Attachment #8798467 - Flags: review?(bob)
Checking /data/local/tmp works for me regarding the ls issue.

I'm not so sure about your suggestions for changing command_output, though. My understanding (maybe incorrect) is that adb squashed all stderr output into stdout prior to N; so we do use stderr_file, but I think prior to N it will always be empty. Do you know of cases where it's not?

If you don't want to squash stderr into stdout, how do you feel about having command_output return both stdout and stderr regardless of version? I think that might be a bit nicer than having command_output behave differently based on the android version.
Flags: needinfo?(bob)
(In reply to Dylan Roeh (:droeh) from comment #9)
> Checking /data/local/tmp works for me regarding the ls issue.
> 

Great.

> I'm not so sure about your suggestions for changing command_output, though.
> My understanding (maybe incorrect) is that adb squashed all stderr output
> into stdout prior to N; so we do use stderr_file, but I think prior to N it
> will always be empty. Do you know of cases where it's not?
> 

It is my understanding that unless adb failed on the host, stderr would be empty prior to N.

> If you don't want to squash stderr into stdout, how do you feel about having
> command_output return both stdout and stderr regardless of version? I think
> that might be a bit nicer than having command_output behave differently
> based on the android version.

I do want to make N behave just like prior versions in adb. What I am suggesting is that for Android N, we emulate the behavior of prior versions by returning the concatenation of stdout and stderr as the output. I believe this is a minimal change to support Android N that preserves backwards compatibility.

I'm just not happy with doing this in Popen while leaving the stderr_file stuff scattered everywhere. I'd rather not make more extensive changes, particularly changing the return signatures, until I have an Android N device on hand and can look into it in more detail.
Flags: needinfo?(bob)
Alright, that makes sense to me. How does this look?
Attachment #8798467 - Attachment is obsolete: true
Attachment #8800784 - Flags: review?(bob)
Comment on attachment 8800784 [details] [diff] [review]
Update mozbase for Android N

droeh: the change to use /data/local/tmp instead of / and to add N to version_codes look good, but the additions of the sdk version detection and the modification of the output in ADBCommand bother me a bit.

Admittedly, the distinction between ADBAndroid in adb_android.py and ADBB2G in adb_b2g.py are kind of irrelevant today due to B2G's obsolescence but it seems to me the proper location for the sdk/output changes is in adb_android.py. We already have sdk detection there and this change is android specific.

Unfortunately, the way I originally wrote those methods makes it more difficult than it should be to make these changes there. Rather than several rounds of me finding new issues due to my own biases, would you be ok if I took a stab at writing a patch that fulfills your needs?
Flags: needinfo?(droeh)
Yeah, if you'd like to take over the bug, that's fine with me.
Flags: needinfo?(droeh)
Attachment #8800784 - Flags: review?(bob)
Assignee: droeh → bob
Status: NEW → ASSIGNED
Depends on: 1310937
ls /data/local/tmp and version_codes.N for autophone
Attachment #8802692 - Flags: review+
I ended up using the subprocess.PIPE but also removed all references to stderr file handles etc.
Attachment #8802693 - Flags: review?(gbrown)
Attachment #8802693 - Flags: review?(droeh)
Attachment #8802695 - Flags: review+
Attached patch bug-1306703-02-v1.patch (obsolete) — Splinter Review
identical to the autophone version.

droeh: Do these work for you?
Attachment #8802698 - Flags: review?(gbrown)
Attachment #8802698 - Flags: review?(droeh)
Attachment #8802693 - Flags: review?(gbrown) → review+
Attachment #8802698 - Flags: review?(gbrown) → review+
Comment on attachment 8802698 [details] [diff] [review]
bug-1306703-02-v1.patch

This fails on my device but is mostly correct. You didn't include changing the ls test directory to /data/local/tmp, and you should be using subprocess.STDOUT rather than subprocess.PIPE, I believe.
Attachment #8802698 - Flags: review?(droeh) → feedback+
Comment on attachment 8802693 [details] [diff] [review]
autophone bug-1306703-02-v1.patch

Same as above.
Attachment #8802693 - Flags: review?(droeh) → feedback+
(In reply to Dylan Roeh (:droeh) from comment #18)
> Comment on attachment 8802698 [details] [diff] [review]
> bug-1306703-02-v1.patch
> 
> This fails on my device but is mostly correct. You didn't include changing
> the ls test directory to /data/local/tmp, and you should be using

the /data/local/tmp should have been in the 01 patch but it looks like I missed the one in self.shell_output("%s -1A /" % self._ls, timeout=timeout)

> subprocess.STDOUT rather than subprocess.PIPE, I believe.

You are correct. Thanks!
(In reply to Bob Clary [:bc:] from comment #20)
> the /data/local/tmp should have been in the 01 patch but it looks like I
> missed the one in self.shell_output("%s -1A /" % self._ls, timeout=timeout)

Ah, my bad, I misread the 01 as being part of a version number and didn't apply it. That looks good to me, then.
No problem. I've updated my patches locally and am testing them on Android 4.4 and 6.0. I'll have updated versions for you later this morning. Thanks for looking at them and finding the problems.
Attachment #8802695 - Attachment is obsolete: true
Attachment #8803458 - Flags: review?(droeh)
Attachment #8802698 - Attachment is obsolete: true
Attachment #8803461 - Flags: review?(gbrown)
Attachment #8803461 - Flags: review?(droeh)
Attachment #8802692 - Attachment is obsolete: true
Attachment #8803463 - Flags: review?(droeh)
Attachment #8802693 - Attachment is obsolete: true
Attachment #8803464 - Flags: review?(gbrown)
Attachment #8803464 - Flags: review?(droeh)
Attachment #8803464 - Flags: review?(gbrown) → review+
Attachment #8803461 - Flags: review?(gbrown) → review+
Comment on attachment 8803458 [details] [diff] [review]
mozdevice bug-1306703-01-v2.patch

Everything looks good to me and works on my device. Thanks for working on this!
Attachment #8803458 - Flags: review?(droeh) → review+
Attachment #8803461 - Flags: review?(droeh) → review+
Attachment #8803463 - Flags: review?(droeh) → review+
Attachment #8803464 - Flags: review?(droeh) → review+
Pushed by bclary@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eba1ac43a15
Autophone - mozdevice - minimal Android 7 support in adb*.py, r=droeh
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a3b906728b2
Autophone - mozdevice - combine stdout, stderr, r=droeh,gbrown
https://hg.mozilla.org/mozilla-central/rev/4eba1ac43a15
https://hg.mozilla.org/mozilla-central/rev/9a3b906728b2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
bc, I dunno if this should be re-opened, but I'm getting "Unable to install" errors with mozregression and Android 7. I filed Bug 1353483 with more details. Any ideas?
Flags: needinfo?(bob)
Should be handled by the release of a new version of adb. We'll have to wait for bug 1353537 to land and merge with m-c.
Flags: needinfo?(bob)
You need to log in before you can comment on or make changes to this bug.