Force root mode when clearing application data.

Force root mode when clearing application data.

The old code was assuming that adb was already running in root, so could
delete application files without asking for extra privilages. This isn't
true on production builds. Fix by explicitly running each command as
root (using su).

BUG=169011
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179915

[Isaac (away), 2013-01-29 21:06:33]

Hold up here!  What is the context of this change?  We're trying to avoid
introducing new dependencies on root.

[Isaac (away), 2013-01-29 21:07:56]

Alright, I read bug...  Any way to avoid this?

[aberent, 2013-01-30 10:33:31]

On 2013/01/29 21:07:56, Isaac wrote:
> Alright, I read bug...  Any way to avoid this?

None I can think of in the short term. We need to be able to reset Chrome and
the device to a known state before running the tests. For security only Chrome
and root can get at these files. In the longer term we could (as discussed in
the bug) possibly create a dummy application that pretends to be Chrome and does
everything we need to Chrome owned files (both these files and the command line
options), but this still won't get rid of our need for root to set up the state
of the device (e.g. to set the performance governors).

[John Knottenbelt, 2013-01-30 18:11:11]

lgtm

[frankf, 2013-01-30 18:15:20]

https://codereview.chromium.org/12094041/diff/1/build/android/pylib/android_c...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/12094041/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:601: self.RunShellCommand('su -c rm -r
/data/data/%s/shared_prefs/*' % package)
can you create a wrapper around these, something like:

self.RunShellCommandRoot('rm -r...')

[Sami, 2013-01-30 18:15:21]

lgtm #2, sorry for letting this sit for a while.

[commit-bot: I haz the power, 2013-01-30 19:36:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/12094041/1

[Isaac (away), 2013-01-30 21:43:20]

(unchecked CQ box).

Can we use adb shell pm clear <package> instead?

[aberent, 2013-01-31 10:00:44]

On 2013/01/30 21:43:20, Isaac wrote:
> (unchecked CQ box).
> 
> Can we use adb shell pm clear <package> instead?

Actually yes. Good find.

[aberent, 2013-01-31 16:00:44]

On further investigation, no.

It seems to work ok on Galaxy S3, but on Galaxy Nexus it seems to clear out too
much data. If I do:

adb shell pm clear com.google.android.apps.chrome

and then try to start Chrome it crashes with:

F/chromium( 7018): [FATAL:chrome_browser_main.cc(716)] Check failed:
PathService::Get(chrome::DIR_USER_DATA, &user_data_dir_). Must be able to get
user data directory!

It looks as if adb shell pm clear is deleting the complete data directory.

The only way to get Chrome working again is to un-install and re-install it.

It is, I think, a Chrome bug that it can't start without a data directory, but I
don't think it is easy to fix since the installer currently the c++ library in
the data directory.

It works on S3 because adb shell pm clear isn't running as root, so can't delete
the library.

Since the original patch is the only known reasonably quick fix for a release
blocker please urgently confirm that you are ok for me land it.




On 2013/01/31 10:00:44, aberent wrote:
> On 2013/01/30 21:43:20, Isaac wrote:
> > (unchecked CQ box).
> > 
> > Can we use adb shell pm clear <package> instead?
> 
> Actually yes. Good find.

[Isaac (away), 2013-01-31 18:54:54]

lgtm

[commit-bot: I haz the power, 2013-01-31 18:57:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/12094041/1

[commit-bot: I haz the power, 2013-01-31 19:00:40]

Change committed as 179915

[Isaac (away), 2013-01-31 19:31:03]

Is this function specific to perf bots?  This is fine as a quick fix but would
be preferable to use pm clear as part of our current goal:
"Tests run without root, buildbots use root for automatation"

Please file a bug about what you think needs to be done to switch to pm clear. 
Part of this might be that you tested running adb shell as root, which most of
the upstream bots do not do (exception being perf bots).

[Isaac (away), 2013-01-31 19:31:03]

Is this function specific to perf bots?  This is fine as a quick fix but would
be preferable to use pm clear as part of our current goal:
"Tests run without root, buildbots use root for automatation"

Please file a bug about what you think needs to be done to switch to pm clear. 
Part of this might be that you tested running adb shell as root, which most of
the upstream bots do not do (exception being perf bots).

[Anthony Berent, 2013-01-31 20:22:28]

No, the function is also used in a couple of other places (run_html5.py and
CrashReportTest.py), I don't know how these are used, or if they are still
being used at all, but they are both downstream (on the clank repository).
There are no references to ClearApplicationState in the upstream tests. If
the function is being used in tests that don't use adb root then I would be
very surprised if the old version actually did anything at all.

I have asked about better ways to do this on Android Chatty, and the answer
I am getting is that it should work, and may be an Android bug. I will see
how this develops over the next day or so before I file a Chrome bug, since
that may clarify things a bit.

On Thu, Jan 31, 2013 at 7:31 PM, <ilevy@chromium.org> wrote:

> Is this function specific to perf bots?  This is fine as a quick fix but
> would
> be preferable to use pm clear as part of our current goal:
> "Tests run without root, buildbots use root for automatation"
>
> Please file a bug about what you think needs to be done to switch to pm
> clear.
> Part of this might be that you tested running adb shell as root, which
> most of
> the upstream bots do not do (exception being perf bots).
>
>
https://chromiumcodereview.**appspot.com/12094041/<https://chromiumcodereview...
>

[aberent, 2013-02-01 12:13:54]

The problems with 'pm clear' turn out to be specific to particular slightly old
builds of Android (JOP22 has the problem, JOP40D is apparently fine). Once I am
not a sheriff (I am currently Chrome Perf Sheriff) I will do some investigation
to find out if we can avoid testing on the problematic Android builds.

On 2013/01/31 20:22:28, Anthony Berent wrote:
> No, the function is also used in a couple of other places (run_html5.py and
> CrashReportTest.py), I don't know how these are used, or if they are still
> being used at all, but they are both downstream (on the clank repository).
> There are no references to ClearApplicationState in the upstream tests. If
> the function is being used in tests that don't use adb root then I would be
> very surprised if the old version actually did anything at all.
> 
> I have asked about better ways to do this on Android Chatty, and the answer
> I am getting is that it should work, and may be an Android bug. I will see
> how this develops over the next day or so before I file a Chrome bug, since
> that may clarify things a bit.
> 
> On Thu, Jan 31, 2013 at 7:31 PM, <mailto:ilevy@chromium.org> wrote:
> 
> > Is this function specific to perf bots?  This is fine as a quick fix but
> > would
> > be preferable to use pm clear as part of our current goal:
> > "Tests run without root, buildbots use root for automatation"
> >
> > Please file a bug about what you think needs to be done to switch to pm
> > clear.
> > Part of this might be that you tested running adb shell as root, which
> > most of
> > the upstream bots do not do (exception being perf bots).
> >
> >
>
https://chromiumcodereview.**appspot.com/12094041/%3Chttps://chromiumcoderevi...>
> >

[Isaac (away), 2013-02-02 05:32:34]

Sounds like the issue only occurs on your local test phone, not the bots.  Are
there any blockers to switching to shell pm clear cmd?  If not, let's switch
this. Thanks.

[aberent, 2013-02-06 16:02:17]

See https://codereview.chromium.org/12210036/ for the change to using "pm clear"