1334613 - Autophone - Pixels raise DMError: Error killing process 'org.mozilla.fennec': ['/system/bin/sh: kill: 5765: Operation not permitted']

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

While testing rca locally I've noticed that on my Pixel device I get

0 ERROR Automation Error: Exception caught while running tests
Traceback (most recent call last):
  File "mochitest/runrobocop.py", line 460, in runSingleTest
    timeout=timeout, symbolsPath=self.options.symbolsPath)
  File ".../tests/mochitest/automation.py", line 
    outputHandler=outputHandler)
  File ".../tests/mochitest/remoteautomation.py",
    proc.kill(True)
  File ".../tests/mochitest/remoteautomation.py",
    self.dm.killProcess(self.procName, 3)
  File ".../tests/mozbase/mozdevice/mozdevice/dev
    "'%s': %s" % (appname, p.output))
DMError: Error killing process 'org.mozilla.fennec': ['/system/bin/sh: kill: 5765: Operation not permitted']
INFO | runtests.py | Test summary: start.
0 INFO TEST-START | Shutdown
1 INFO Passed: 0
2 INFO Failed: 0
3 INFO Todo: 0
4 INFO SimpleTest FINISHED
INFO | runtests.py | Test summary: end.

which certainly looks like a cross process permissions problem on Android 7+. I haven't noticed this before during my Pixel testing.

Two approaches:

1. Quick Autophone local fix. Just use adb.py ADBAndroid.kill with root=True to kill fennec before starting Unittests. This may not work if the test runner runs the test more than once.

2. Fix devicemanagerADB.py in mozdevice so that killProcess uses root when the sdk is >= 24.

Thoughts?

Comment 1

[Geoff Brown [:gbrown]]

I like the sound of #2 -- should save us effort in the future.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: bug-1334613-pixel-perms.patch

I did a try run against my local devices overnight:

https://treeherder.allizom.org/#/jobs?repo=try&revision=3f94cb4ac4f772df6c53b9c5970f6264fe320a7b

It worked pretty well but led me to add the check in killProcess to not raise DMError if "No such process" was found. I retested locally without submitting to Treeherder and the updated patch seems ok.

I've submitted a new try run against production:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48d7a0d878e7bb67a722b6593d558ec743f46fc8&exclusion_profile=false&group_state=expanded

The whole shell, _runCmd, _checkCmd is a pita so maybe this isn't the best way to go. I think it will work though.

Comment 3

[Geoff Brown [:gbrown]]

Comment on attachment 8831764 [details] [diff] [review]
bug-1334613-pixel-perms.patch

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

::: testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py
@@ +467,4 @@
>          procs = self.getProcessList()
>          for (pid, name, user) in procs:
>              if name == appname:
> +                args = list(shell_args)

It doesn't hurt, but I think you don't need list() here.

@@ +473,5 @@
>                      args.append("-%d" % sig)
>                  args.append(str(pid))
>                  p = self._runCmd(args, timeout=self.short_timeout)
> +                if p.returncode != 0 and len(p.output) > 0 and \
> +                   'No such process' not in p.output[0]:

Probably 'No such process' just means the process died on its own while we were thinking of killing it? Seems like a good thing to ignore.

@@ +761,3 @@
>              self._adb_version = re_version.match(proc.output[0]).group(1)
>              self._logger.info("Detected adb %s", self._adb_version)
> +            proc = self._runCmd(["shell", "getprop", "ro.build.version.sdk"],

I have a vague feeling I have seen android images without ro.build.version.sdk defined...but that was probably a long time ago / maybe nothing to worry about.

Comment 4

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #3)
> Comment on attachment 8831764 [details] [diff] [review]
> bug-1334613-pixel-perms.patch
> 
> Review of attachment 8831764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py
> @@ +467,4 @@
> >          procs = self.getProcessList()
> >          for (pid, name, user) in procs:
> >              if name == appname:
> > +                args = list(shell_args)
> 
> It doesn't hurt, but I think you don't need list() here.
> 

I did that since I add the su args to shell_args prior to the loop and didn't want the loop to modify shell_args. 

Just assigning would have meant args was just a reference to shell_args, right?

> @@ +473,5 @@
> >                      args.append("-%d" % sig)
> >                  args.append(str(pid))
> >                  p = self._runCmd(args, timeout=self.short_timeout)
> > +                if p.returncode != 0 and len(p.output) > 0 and \
> > +                   'No such process' not in p.output[0]:
> 
> Probably 'No such process' just means the process died on its own while we
> were thinking of killing it? Seems like a good thing to ignore.
> 

Yes. Just a race condition that isn't an error and which shouldn't be raised.

> @@ +761,3 @@
> >              self._adb_version = re_version.match(proc.output[0]).group(1)
> >              self._logger.info("Detected adb %s", self._adb_version)
> > +            proc = self._runCmd(["shell", "getprop", "ro.build.version.sdk"],
> 
> I have a vague feeling I have seen android images without
> ro.build.version.sdk defined...but that was probably a long time ago / maybe
> nothing to worry about.

Thank goodness we've dropped support for the really old versions.

Comment 5

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #4)
> (In reply to Geoff Brown [:gbrown] from comment #3)
> > It doesn't hurt, but I think you don't need list() here.
> 
> I did that since I add the su args to shell_args prior to the loop and
> didn't want the loop to modify shell_args. 

I see. Makes sense now.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6703e57f3778
[mozdevice] - use root to kill processes on Android 7+, r=gbrown.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6703e57f3778

Comment 8

[Bob Clary [:bc] (inactive)]

a=testonly for aurora, beta, release.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2678b49156fb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f271c045f7ab

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/cda9860414d4