[Android] Set permission when installing apks.

[Android] Set permission when installing apks.

To support M testing, we need to explicilty grant permissions to apps
when we install them.

BUG=

Committed: https://crrev.com/de468d72d9eeeb86b439e6a7c877de0ebb349fca
Cr-Commit-Position: refs/heads/master@{#347787}

[rnephew (Reviews Here), 2015-09-03 15:15:28]

rnephew@chromium.org changed reviewers:
+ jbudorick@chromium.org, perezju@chromium.org

[rnephew (Reviews Here), 2015-09-03 15:15:28]

Tested locally using:

Gtest, platform mode:
./test_runner.py gtest -s media_unittests --gtest-filter *MediaCodecPlayerTest*
--device $NEXUS6 --enable-platform-mode

Gtest, old mode:
./test_runner.py gtest -s media_unittests --gtest-filter *MediaCodecPlayerTest*
--device $NEXUS6

Instrumentation, platform mode:
./test_runner.py instrumentation --device $NEXUS4 --enable-platform-mode
--test-apk $CLANK_DIR/out/Debug/apks/ChromeTest.apk --isolate-file-path
$CLANK_DIR/clank/javatests/chrome_test_apk.isolate  -A Smoke --host-driven-root
$CLANK_DIR/clank/test/ --apk-under-test $CLANK_DIR/out/Debug/apks/Chrome.apk

Instrumentation, old mode:
./test_runner.py instrumentation --device $NEXUS4 --test-apk ChromeTest
--isolate-file-path $CLANK_DIR/clank/javatests/chrome_test_apk.isolate  -A Smoke
--host-driven-root $CLANK_DIR/clank/test/

[jbudorick, 2015-09-03 15:20:49]

https://codereview.chromium.org/1316803004/diff/80001/build/android/devil/and...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/80001/build/android/devil/and...
build/android/devil/android/device_utils.py:557: if set_permissions:
I'm concerned about the performance of doing this in Install. This will usually
mean that tests parse the manifest from the APK twice, as many of the test
runner code already does it.

[jbudorick, 2015-09-03 15:24:52]

https://codereview.chromium.org/1316803004/diff/80001/build/android/devil/and...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/80001/build/android/devil/and...
build/android/devil/android/device_utils.py:557: if set_permissions:
On 2015/09/03 at 15:20:49, jbudorick wrote:
> I'm concerned about the performance of doing this in Install. This will
usually mean that tests parse the manifest from the APK twice, as many of the
test runner code already does it.

(That said,  I'm also wondering when we _wouldn't_ want to grant the requested
permissions, so I'm kind of split on this one.)

[rnephew (Reviews Here), 2015-09-03 17:37:33]

https://codereview.chromium.org/1316803004/diff/80001/build/android/devil/and...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/80001/build/android/devil/and...
build/android/devil/android/device_utils.py:557: if set_permissions:
On 2015/09/03 15:24:52, jbudorick wrote:
> On 2015/09/03 at 15:20:49, jbudorick wrote:
> > I'm concerned about the performance of doing this in Install. This will
> usually mean that tests parse the manifest from the APK twice, as many of the
> test runner code already does it.
> 
> (That said,  I'm also wondering when we _wouldn't_ want to grant the requested
> permissions, so I'm kind of split on this one.)

Changed so that Install accepts permissions=[list_of_permissions].
Also made changes so that if the manifest has already been parsed, it will use
that permissions list; otherwise it will not pass a list to install and let it
figure out what to do.

[jbudorick, 2015-09-03 17:39:00]

https://codereview.chromium.org/1316803004/diff/100001/build/android/devil/an...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/100001/build/android/devil/an...
build/android/devil/android/device_utils.py:1935: with
device_temp_file.DeviceTempFile(self.adb, suffix='.sh') as script:
You should just be able to pass the script to RunShellCommand. It'll handle the
tempfile part.

https://codereview.chromium.org/1316803004/diff/100001/build/android/devil/an...
build/android/devil/android/device_utils.py:1937: logging.critical('Setting
permissions for %s', package)
This shouldn't be critical. info, probably.

[rnephew (Reviews Here), 2015-09-03 19:48:12]

https://codereview.chromium.org/1316803004/diff/100001/build/android/devil/an...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/100001/build/android/devil/an...
build/android/devil/android/device_utils.py:1935: with
device_temp_file.DeviceTempFile(self.adb, suffix='.sh') as script:
On 2015/09/03 17:39:00, jbudorick wrote:
> You should just be able to pass the script to RunShellCommand. It'll handle
the
> tempfile part.

Talked offline, but recap of situation:

Some of the permissions cant be set. This method allows it to fail gracefully
and set the ones it can set. I changed it to output the failures and to check if
permissions are in a blacklist. Eventually, this will be changed to just chain
them in one command with && but will have to wait until the blacklist is fully
populated.

https://codereview.chromium.org/1316803004/diff/100001/build/android/devil/an...
build/android/devil/android/device_utils.py:1937: logging.critical('Setting
permissions for %s', package)
On 2015/09/03 17:39:00, jbudorick wrote:
> This shouldn't be critical. info, probably.

Artifact from local testing. Done.

[jbudorick, 2015-09-03 20:55:45]

https://codereview.chromium.org/1316803004/diff/140001/build/android/devil/an...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/140001/build/android/devil/an...
build/android/devil/android/device_utils.py:69:
'com.google.android.apps.chrome.permission.C2D_MESSAGE',
sort these alphabetically.

https://codereview.chromium.org/1316803004/diff/140001/build/android/devil/an...
build/android/devil/android/device_utils.py:1953: # Permissions only set M and
higher.
nit: rephrase

https://codereview.chromium.org/1316803004/diff/140001/build/android/devil/an...
build/android/devil/android/device_utils.py:1959: #    RunShellCommand without
putting it into a file. Also add
nit: one space after #

https://codereview.chromium.org/1316803004/diff/140001/build/android/devil/an...
build/android/devil/android/device_utils.py:1969: if output:
This should do better detection of which permission caused the failure.

https://codereview.chromium.org/1316803004/diff/140001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1316803004/diff/140001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:69:
dev.Install(self._test_instance.apk_under_test)
I'm wondering if we should have the instrumentation test instance parse the APK
under test, too.

[rnephew (Reviews Here), 2015-09-03 21:05:09]

https://codereview.chromium.org/1316803004/diff/140001/build/android/devil/an...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/140001/build/android/devil/an...
build/android/devil/android/device_utils.py:69:
'com.google.android.apps.chrome.permission.C2D_MESSAGE',
On 2015/09/03 20:55:45, jbudorick wrote:
> sort these alphabetically.

Done.

https://codereview.chromium.org/1316803004/diff/140001/build/android/devil/an...
build/android/devil/android/device_utils.py:1953: # Permissions only set M and
higher.
On 2015/09/03 20:55:45, jbudorick wrote:
> nit: rephrase

Done.

https://codereview.chromium.org/1316803004/diff/140001/build/android/devil/an...
build/android/devil/android/device_utils.py:1959: #    RunShellCommand without
putting it into a file. Also add
On 2015/09/03 20:55:45, jbudorick wrote:
> nit: one space after #

Done.

https://codereview.chromium.org/1316803004/diff/140001/build/android/devil/an...
build/android/devil/android/device_utils.py:1969: if output:
On 2015/09/03 20:55:45, jbudorick wrote:
> This should do better detection of which permission caused the failure.

I plan to in future CLs; but I just wanted to dump everything now until the
blacklist gets completed. I've seen 3 different formats of error messages from
this, so I dont want to make any assumptions on how to parse it.

https://codereview.chromium.org/1316803004/diff/140001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1316803004/diff/140001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:69:
dev.Install(self._test_instance.apk_under_test)
On 2015/09/03 20:55:45, jbudorick wrote:
> I'm wondering if we should have the instrumentation test instance parse the
APK
> under test, too.

I had that a few iterations ago, but decided it added unnecessary clutter since
nowhere else here uses it.

[jbudorick, 2015-09-03 21:36:24]

https://codereview.chromium.org/1316803004/diff/160001/build/android/devil/an...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/160001/build/android/devil/an...
build/android/devil/android/device_utils.py:1970: logging.info('Possible problem
when granting permissions. Blacklist may'
This should at least be a warning.

https://codereview.chromium.org/1316803004/diff/160001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1316803004/diff/160001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:69:
dev.Install(self._test_instance.apk_under_test)
I think that's ok. We're still going to wind up building an ApkHelper -- we
might as well keep it in the instance.

[rnephew (Reviews Here), 2015-09-03 21:52:19]

https://codereview.chromium.org/1316803004/diff/160001/build/android/devil/an...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/160001/build/android/devil/an...
build/android/devil/android/device_utils.py:1970: logging.info('Possible problem
when granting permissions. Blacklist may'
On 2015/09/03 21:36:24, jbudorick wrote:
> This should at least be a warning.

Done.

https://codereview.chromium.org/1316803004/diff/160001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1316803004/diff/160001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:69:
dev.Install(self._test_instance.apk_under_test)
On 2015/09/03 21:36:24, jbudorick wrote:
> I think that's ok. We're still going to wind up building an ApkHelper -- we
> might as well keep it in the instance.

Done.

[perezju, 2015-09-04 09:11:21]

looks good, just a comment and a small nit

https://codereview.chromium.org/1316803004/diff/180001/build/android/devil/an...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/180001/build/android/devil/an...
build/android/devil/android/device_utils.py:524: def Install(self, apk_path,
permissions=None, reinstall=False, timeout=None,
nit: I would rather have arg order as: apk_path, reinstall, permissions,
timeout, retries. (more common first)

https://codereview.chromium.org/1316803004/diff/180001/build/android/devil/an...
build/android/devil/android/device_utils.py:565: if permissions is None:
Looks like on pre-M we will GetPremissions, but then will not do anything with
them. Seems a bit like a waste. Can we add a check and also skip getting
permissions pre-M?

[rnephew (Reviews Here), 2015-09-04 15:08:15]

https://codereview.chromium.org/1316803004/diff/180001/build/android/devil/an...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/180001/build/android/devil/an...
build/android/devil/android/device_utils.py:524: def Install(self, apk_path,
permissions=None, reinstall=False, timeout=None,
On 2015/09/04 09:11:21, perezju wrote:
> nit: I would rather have arg order as: apk_path, reinstall, permissions,
> timeout, retries. (more common first)

Done.

https://codereview.chromium.org/1316803004/diff/180001/build/android/devil/an...
build/android/devil/android/device_utils.py:565: if permissions is None:
On 2015/09/04 09:11:21, perezju wrote:
> Looks like on pre-M we will GetPremissions, but then will not do anything with
> them. Seems a bit like a waste. Can we add a check and also skip getting
> permissions pre-M?

Done.

[jbudorick, 2015-09-08 18:30:48]

https://codereview.chromium.org/1316803004/diff/200001/build/android/devil/an...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/200001/build/android/devil/an...
build/android/devil/android/device_utils.py:568: permissions =
helper.GetPermissions()
nit: just

  permissions = apk_helper.ApkHelper(apk_path).GetPermissions()

https://codereview.chromium.org/1316803004/diff/200001/build/android/devil/an...
build/android/devil/android/device_utils.py:1954: # Permissions only require to
be set on M or higher.
nit: rephrase

https://codereview.chromium.org/1316803004/diff/200001/build/android/devil/an...
build/android/devil/android/device_utils.py:1958: with
device_temp_file.DeviceTempFile(self.adb, suffix='.sh') as script:
try this w/ passing a string cmd to RunShellCommand instead of handrolling a
script.

https://codereview.chromium.org/1316803004/diff/200001/build/android/devil/an...
build/android/devil/android/device_utils.py:1959: # TODO(rnephew): After
blacklist is complete, switch to using
nit: after permissions blacklist

[rnephew (Reviews Here), 2015-09-08 19:36:01]

https://codereview.chromium.org/1316803004/diff/200001/build/android/devil/an...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316803004/diff/200001/build/android/devil/an...
build/android/devil/android/device_utils.py:568: permissions =
helper.GetPermissions()
On 2015/09/08 18:30:48, jbudorick wrote:
> nit: just
> 
>   permissions = apk_helper.ApkHelper(apk_path).GetPermissions()

Done.

https://codereview.chromium.org/1316803004/diff/200001/build/android/devil/an...
build/android/devil/android/device_utils.py:1954: # Permissions only require to
be set on M or higher.
On 2015/09/08 18:30:47, jbudorick wrote:
> nit: rephrase

Done.

https://codereview.chromium.org/1316803004/diff/200001/build/android/devil/an...
build/android/devil/android/device_utils.py:1958: with
device_temp_file.DeviceTempFile(self.adb, suffix='.sh') as script:
On 2015/09/08 18:30:48, jbudorick wrote:
> try this w/ passing a string cmd to RunShellCommand instead of handrolling a
> script.

Done.

https://codereview.chromium.org/1316803004/diff/200001/build/android/devil/an...
build/android/devil/android/device_utils.py:1959: # TODO(rnephew): After
blacklist is complete, switch to using
On 2015/09/08 18:30:48, jbudorick wrote:
> nit: after permissions blacklist

Done.

[jbudorick, 2015-09-08 19:39:22]

lgtm

[rnephew (Reviews Here), 2015-09-08 19:39:41]

The CQ bit was checked by rnephew@chromium.org

[commit-bot: I haz the power, 2015-09-08 19:40:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1316803004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1316803004/220001

[commit-bot: I haz the power, 2015-09-08 21:03:11]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-09-08 21:03:48]

Patchset 12 (id:??) landed as
https://crrev.com/de468d72d9eeeb86b439e6a7c877de0ebb349fca
Cr-Commit-Position: refs/heads/master@{#347787}