GN: Side-load dex files as well as native code in incremental installs

GN: Side-load dex files as well as native code in incremental installs

This also re-organizes a bit by moving all incremental install related
files into //build/android/incremental_install.

BUG=520082
Committed: https://crrev.com/933f2593199c4f95678ddda884aa476ff1fe0c23
Cr-Commit-Position: refs/heads/master@{#349200}

[agrieve, 2015-09-12 02:33:30]

Note: Got presubmit warnings (not sure how to resolve or if it's a concern):

Pylint (241 files using ['--disable=cyclic-import'] on 40 cores) (6.80s) failed
************* Module create_install_script
F: 18, 0: Unable to import 'util' (import-error)
************* Module generate_android_manifest
F: 18, 0: Unable to import 'util' (import-error)
************* Module installer
F: 27, 0: Unable to import 'util' (import-error)

[jbudorick, 2015-09-12 02:35:13]

On 2015/09/12 at 02:33:30, agrieve wrote:
> Note: Got presubmit warnings (not sure how to resolve or if it's a concern):
> 
> Pylint (241 files using ['--disable=cyclic-import'] on 40 cores) (6.80s)
failed
> ************* Module create_install_script
> F: 18, 0: Unable to import 'util' (import-error)
> ************* Module generate_android_manifest
> F: 18, 0: Unable to import 'util' (import-error)
> ************* Module installer
> F: 27, 0: Unable to import 'util' (import-error)

yeah, the linter doesn't seem to be a big fan of importing things from
directories appended to sys.path.

# pylint: disable=import-error

[agrieve, 2015-09-15 18:00:54]

agrieve@chromium.org changed reviewers:
+ jbudorick@chromium.org, nyquist@chromium.org, thestig@chromium.org

[agrieve, 2015-09-15 18:00:55]

jbudorick@chromium.org: Please review Python changes

nyquist@chromium.org: Please review Java changes

thestig@chromium.org: Please review changes in base/*

[jbudorick, 2015-09-15 18:36:42]

https://codereview.chromium.org/1338813003/diff/40001/build/android/devil/and...
File build/android/devil/android/apk_helper.py (right):

https://codereview.chromium.org/1338813003/diff/40001/build/android/devil/and...
build/android/devil/android/apk_helper.py:20: _TRUE = '0xffffffff'
I worry about this being fragile if Android decides to represent true as
anything else. What about just zero or nonzero?

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
File build/android/incremental_install/create_install_script.py (right):

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/create_install_script.py:75: )
nit: pull this up onto the previous line

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
File build/android/incremental_install/generate_android_manifest.py (right):

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/generate_android_manifest.py:40:
parser.add_argument('--disable-isolated-processes',
When would we use this?

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/generate_android_manifest.py:47:
disable_isolated_processes):
nit: indentation

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
File build/android/incremental_install/installer.py (right):

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/installer.py:18:
sys.path.append(os.path.join(os.path.dirname(__file__), os.pardir, 'gyp'))
nit: I'd prefer this one down above the build_utils import s.t. it's easy to
tell what's coming from where.

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/installer.py:104: is_installed =
device.GetApplicationPaths(apk_package)
Is this faster than just a raw Uninstall call? That'd be surprising, but if it's
the case, we should change the implementation of DeviceUtils.Uninstall
accordingly.

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/installer.py:118:
'disable_incremental_isolated_processes=true to do so.')
o_o

yikes

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/installer.py:151: shutil.copyfile(src_path,
os.path.join(temp_dir, dest_name))
cross your fingers and hope for copy-on-write, I suppose.

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/installer.py:165: device.RunShellCommand(cmd %
device_incremental_dir, check_return=True)
I think I'm missing something... how is this creating install.lock?

https://codereview.chromium.org/1338813003/diff/40001/build/android/pylib/uti...
File build/android/pylib/utils/time_profile.py (right):

https://codereview.chromium.org/1338813003/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/time_profile.py:30: delta = round(delta, 2) if delta <
1 else round(delta, 1)
nit: I'd prefer this rounding threshold at 10s instead of 1s

[Lei Zhang, 2015-09-15 19:27:25]

I defer to base/android/OWNERS

[agrieve, 2015-09-15 19:30:05]

https://codereview.chromium.org/1338813003/diff/40001/build/android/devil/and...
File build/android/devil/android/apk_helper.py (right):

https://codereview.chromium.org/1338813003/diff/40001/build/android/devil/and...
build/android/devil/android/apk_helper.py:20: _TRUE = '0xffffffff'
On 2015/09/15 18:36:41, jbudorick wrote:
> I worry about this being fragile if Android decides to represent true as
> anything else. What about just zero or nonzero?

Changed it to parse the number and look for non-zero.

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
File build/android/incremental_install/create_install_script.py (right):

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/create_install_script.py:75: )
On 2015/09/15 18:36:41, jbudorick wrote:
> nit: pull this up onto the previous line

Done.

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
File build/android/incremental_install/generate_android_manifest.py (right):

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/generate_android_manifest.py:40:
parser.add_argument('--disable-isolated-processes',
On 2015/09/15 18:36:41, jbudorick wrote:
> When would we use this?

Required for Android M (added to help text).

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/generate_android_manifest.py:47:
disable_isolated_processes):
On 2015/09/15 18:36:41, jbudorick wrote:
> nit: indentation

Done.

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
File build/android/incremental_install/installer.py (right):

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/installer.py:18:
sys.path.append(os.path.join(os.path.dirname(__file__), os.pardir, 'gyp'))
On 2015/09/15 18:36:41, jbudorick wrote:
> nit: I'd prefer this one down above the build_utils import s.t. it's easy to
> tell what's coming from where.

Done.

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/installer.py:104: is_installed =
device.GetApplicationPaths(apk_package)
On 2015/09/15 18:36:41, jbudorick wrote:
> Is this faster than just a raw Uninstall call? That'd be surprising, but if
it's
> the case, we should change the implementation of DeviceUtils.Uninstall
> accordingly.

DeviceUtils.Uninstall() fails if the app isn't already installed. I've moved the
logic to device_utils so that it doesn't anymore.

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/installer.py:118:
'disable_incremental_isolated_processes=true to do so.')
On 2015/09/15 18:36:41, jbudorick wrote:
> o_o
> 
> yikes

yeah... :( maybe someone will figure it out, but for now SELinux has foiled all
of my ideas.

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/installer.py:151: shutil.copyfile(src_path,
os.path.join(temp_dir, dest_name))
On 2015/09/15 18:36:41, jbudorick wrote:
> cross your fingers and hope for copy-on-write, I suppose.

Yeah, might come back to this, but it's sub-second for me.

https://codereview.chromium.org/1338813003/diff/40001/build/android/increment...
build/android/incremental_install/installer.py:165: device.RunShellCommand(cmd %
device_incremental_dir, check_return=True)
On 2015/09/15 18:36:41, jbudorick wrote:
> I think I'm missing something... how is this creating install.lock?

You're missing the invisible > that I changed too late on to notice! (nice
catch!)

https://codereview.chromium.org/1338813003/diff/40001/build/android/pylib/uti...
File build/android/pylib/utils/time_profile.py (right):

https://codereview.chromium.org/1338813003/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/time_profile.py:30: delta = round(delta, 2) if delta <
1 else round(delta, 1)
On 2015/09/15 18:36:41, jbudorick wrote:
> nit: I'd prefer this rounding threshold at 10s instead of 1s

Done.

[jbudorick, 2015-09-15 22:36:49]

python generally looks good.

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

https://codereview.chromium.org/1338813003/diff/60001/build/android/devil/and...
build/android/devil/android/device_utils.py:644: installed =
self._GetApplicationPathsInternal(package_name)
er... hm. Reconsidering this one, as there's now no way to distinguish between
an app being uninstalled and an app not being present. Perhaps it'd be better to
handle this in the client after all with a try/except CommandFailedError.

[jbudorick, 2015-09-15 22:37:28]

On 2015/09/15 at 22:36:49, jbudorick wrote:
> python generally looks good.
> 
>
https://codereview.chromium.org/1338813003/diff/60001/build/android/devil/and...
> File build/android/devil/android/device_utils.py (right):
> 
>
https://codereview.chromium.org/1338813003/diff/60001/build/android/devil/and...
> build/android/devil/android/device_utils.py:644: installed =
self._GetApplicationPathsInternal(package_name)
> er... hm. Reconsidering this one, as there's now no way to distinguish between
an app being uninstalled and an app not being present. Perhaps it'd be better to
handle this in the client after all with a try/except CommandFailedError.

(that is, no way to make that distinction from the call to Uninstall)

[agrieve, 2015-09-16 01:08:57]

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

https://codereview.chromium.org/1338813003/diff/60001/build/android/devil/and...
build/android/devil/android/device_utils.py:644: installed =
self._GetApplicationPathsInternal(package_name)
On 2015/09/15 22:36:48, jbudorick wrote:
> er... hm. Reconsidering this one, as there's now no way to distinguish between
> an app being uninstalled and an app not being present. Perhaps it'd be better
to
> handle this in the client after all with a try/except CommandFailedError.

GetApplicationPaths is a public method, so if the caller really cared, they
could check first whether the app is already uninstalled. I think just making
sure it's gone is the 99% use-case though.

[nyquist, 2015-09-16 06:30:46]

sorry for all the nits :-/
mostly looks good, I just have a few questions.

https://codereview.chromium.org/1338813003/diff/60001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
(right):

https://codereview.chromium.org/1338813003/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/BaseChromiumApplication.java:22:
super.attachBaseContext(base);
Nit: Override now only calls super, and does not increase visibility.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
File
build/android/incremental_install/java/org/chromium/incrementalinstall/BootstrapApplication.java
(right):

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/BootstrapApplication.java:5:
package org.chromium.incrementalinstall;
Optional nit: How do you feel about org.chromium.build.incrementalinstall? I
don't feel strongly though, since it's kind of a standalone thing.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/BootstrapApplication.java:51:
if (isFirstRun && mClassLoaderPatcher.mIsPrimaryProcess) {
Nit: I know it would be an additional indent, but I find it easier to read:
if (isFirstRun) {
  if (...isPrimary...) {
    ..install..
  } else {
    ..firstRun..
  }
}

I think that should yield the same result?

Or where you trying to keep the same format for the if-statement as below where
you clear the installer lock for first run?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/BootstrapApplication.java:70:
String mRealApplicationName = getRealApplicationName();
Nit: Just |realApplicationName|

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/BootstrapApplication.java:97:
throw new RuntimeException(e);
Nit: I know we log that we're trying above here, but could we add a short
message to the exception here saying that this magically cool thing failed?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
File
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java
(right):

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:32:
ClassLoaderPatcher(Context context) throws ReflectiveOperationException {
ReflectiveOperationException was added in API level 19. I believe that is the
first version of KitKat. Does this work with JELLY_BEAN_MR2 (API level 18)? I
see that the comment above says so, it's just odd to me. See
http://developer.android.com/reference/android/os/Build.VERSION_CODES.html and
https://developer.android.com/reference/java/lang/ReflectiveOperationExceptio...

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:37:
mProcessUid = (Integer) Reflect.invokeMethod(mLibcoreOs, "getuid");
I wish we didn't have to do this hack to call getuid, because then we could use
it in legit prod code to separate browser process from the others early on.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:62:
incrementalDexesDir.mkdir();
Nit: What if this fails? (same below)

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:78:
optimizedDir.mkdir();
Nit: This might fail and therefore returns a boolean. Could we check the return
value of this operation and either fail, or gracefully ignore / log?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:82:
f.delete();
Nit: This operation might fail, and therefore it returns a boolean. Would we
want to bail out if the delete fails?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:92:
// TODO(agrieve): Might need to recored classpath ordering if we ever have
duplicate
Nit: record*

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:94:
ArrayList<File> dexFiles = new ArrayList<File>(Arrays.asList(dexFilesArr));
This seems to only be used to print dexFiles.size(), since you use dexFilesArr
below. Could you just use dexFilesArr.length in the log message instead? Or did
you mean to use dexFiles in the call to makeDexElements(...) below?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:110:
File localLibsDir = new File(mAppFilesSubDir, "lib");
Nit: Could you extract this block to something like:
libDir = prepareLocalLibsAndroidM();
?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:117:
if (lockFile != null) {
Nit: Could you flip these if-blocks, so it reads if (lockFile == null) { ... }
else { ... } ?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:132:
libDir = localLibsDir;
Nit: This just looks odd to me (reassignment of parameter), but I don't feel
strongly about adding another variable to rectify it either.

I see this has been done many places in this CL, so at least it's consistent :-)

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:152:
Object[] nativeLibraryPathElements = null;
Nit: This null-assignment is unnecessary. It's always set (or exception is
thrown, and you return), in the try-block below.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:168:
dstDir.mkdir();
Nit: These three operations can also fail. What would we do if they did?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:189:
dest.setReadable(true, false);
Nit: Same as above for these three.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:196:
mAppFilesSubDir.mkdir();
Nit: Same as above for these two.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
File
build/android/incremental_install/java/org/chromium/incrementalinstall/LockFile.java
(right):

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/LockFile.java:36:
// On Android M, we can't delete files in /data/local/tmp, so we write to it
instead.
Nit: Should this be "Android M+"?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/LockFile.java:57:
final long pollIntervalMs = 200;
Nit: Unnecessary final

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/LockFile.java:91:
static LockFile acquireRuntimeLock(final File file) {
Nit: Unnecessary final

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/LockFile.java:124:
mFile.delete();
Nit: Do we need to do anything special if this operation fails? Log something?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
File
build/android/incremental_install/java/org/chromium/incrementalinstall/Reflect.java
(right):

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/Reflect.java:56:
* type.
Nit: How do you feel about adding an assert (or just throw) that the types are
the same?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/Reflect.java:59:
Object[] result = (Object[]) (Array.newInstance(
Nit: paranthesis around Array.newInstance(...) call is unnecessary.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/Reflect.java:71:
instance = null;
Nit: This reassigns a parameter value to null which was at first read of this
method odd to me. I wonder if this could be easier to read if we did something
like:
boolean isStatic;
if ( is class ) {
  isStatic = true;
  ..
} else {
  isStatic = false;
  ...
}
...
if (isStatic) return method.invoke(null, params);
return method.invoke(instance, params);

or something along those lines?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/Reflect.java:85:
static Object newInstance(Class<?> clazz, Object... params)
Nit: Could you add a comment about whether this works as expected in no-arg
constructors?

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/Reflect.java:107:
} catch (NoSuchFieldException e) {
Nit: Could you add the boilerplate-like '// This shouldn't happen.' here to keep
me sane? :-p

[agrieve, 2015-09-16 14:59:59]

https://codereview.chromium.org/1338813003/diff/60001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
(right):

https://codereview.chromium.org/1338813003/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/BaseChromiumApplication.java:22:
super.attachBaseContext(base);
On 2015/09/16 06:30:45, nyquist (OOO - back 9-18) wrote:
> Nit: Override now only calls super, and does not increase visibility.

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
File
build/android/incremental_install/java/org/chromium/incrementalinstall/BootstrapApplication.java
(right):

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/BootstrapApplication.java:5:
package org.chromium.incrementalinstall;
On 2015/09/16 06:30:45, nyquist (OOO - back 9-18) wrote:
> Optional nit: How do you feel about org.chromium.build.incrementalinstall? I
> don't feel strongly though, since it's kind of a standalone thing.

All the same to me, so will leave it if you don't really care.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/BootstrapApplication.java:51:
if (isFirstRun && mClassLoaderPatcher.mIsPrimaryProcess) {
On 2015/09/16 06:30:45, nyquist (OOO - back 9-18) wrote:
> Nit: I know it would be an additional indent, but I find it easier to read:
> if (isFirstRun) {
>   if (...isPrimary...) {
>     ..install..
>   } else {
>     ..firstRun..
>   }
> }
> 
> I think that should yield the same result?
> 
> Or where you trying to keep the same format for the if-statement as below
where
> you clear the installer lock for first run?

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/BootstrapApplication.java:70:
String mRealApplicationName = getRealApplicationName();
On 2015/09/16 06:30:45, nyquist (OOO - back 9-18) wrote:
> Nit: Just |realApplicationName|

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/BootstrapApplication.java:97:
throw new RuntimeException(e);
On 2015/09/16 06:30:45, nyquist (OOO - back 9-18) wrote:
> Nit: I know we log that we're trying above here, but could we add a short
> message to the exception here saying that this magically cool thing failed?

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
File
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java
(right):

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:32:
ClassLoaderPatcher(Context context) throws ReflectiveOperationException {
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> ReflectiveOperationException was added in API level 19. I believe that is the
> first version of KitKat. Does this work with JELLY_BEAN_MR2 (API level 18)? I
> see that the comment above says so, it's just odd to me. See
> http://developer.android.com/reference/android/os/Build.VERSION_CODES.html and
>
https://developer.android.com/reference/java/lang/ReflectiveOperationExceptio...

You are correct!
http://stackoverflow.com/questions/24992670/verifyerror-only-on-some-android-...

Just re-tried it and it worked fine on my device, so I suspect it'll only come
up if an exception is actually thrown. All the code just re-throws as a runtime
exception anyways, so I'm just going to leave it (listing out each exception
hurts - there are up to 4, and throwing Exception is overbroad).

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:37:
mProcessUid = (Integer) Reflect.invokeMethod(mLibcoreOs, "getuid");
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> I wish we didn't have to do this hack to call getuid, because then we could
use
> it in legit prod code to separate browser process from the others early on.

There's also:
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:62:
incrementalDexesDir.mkdir();
On 2015/09/16 06:30:45, nyquist (OOO - back 9-18) wrote:
> Nit: What if this fails? (same below)

Then things won't work :P. 

It fails silently if the directory already exists, so that's good. If for some
reason it actually just can't create it, then the following line will throw a
FileNotFound I think.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:78:
optimizedDir.mkdir();
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: This might fail and therefore returns a boolean. Could we check the
return
> value of this operation and either fail, or gracefully ignore / log?

I'm a bit afraid that adding a lot of explicit checks will make the code harder
to read. In the end there is a lot that can go wrong with all of this code, and
there's very little in an exception messages that will help you debug it (need
to adb shell in and look to see what looks wrong).

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:82:
f.delete();
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: This operation might fail, and therefore it returns a boolean. Would we
> want to bail out if the delete fails?

Having stale symlinks around is harmless. If one fails to delete that we then
try and recreate then we'll get an exception anyways.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:92:
// TODO(agrieve): Might need to recored classpath ordering if we ever have
duplicate
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: record*

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:94:
ArrayList<File> dexFiles = new ArrayList<File>(Arrays.asList(dexFilesArr));
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> This seems to only be used to print dexFiles.size(), since you use dexFilesArr
> below. Could you just use dexFilesArr.length in the log message instead? Or
did
> you mean to use dexFiles in the call to makeDexElements(...) below?

One iteration did use a list, but this certainly doesn't need to be here now.
Done!

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:110:
File localLibsDir = new File(mAppFilesSubDir, "lib");
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: Could you extract this block to something like:
> libDir = prepareLocalLibsAndroidM();
> ?

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:117:
if (lockFile != null) {
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: Could you flip these if-blocks, so it reads if (lockFile == null) { ... }
> else { ... } ?

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:132:
libDir = localLibsDir;
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: This just looks odd to me (reassignment of parameter), but I don't feel
> strongly about adding another variable to rectify it either.
> 
> I see this has been done many places in this CL, so at least it's consistent
:-)

Looks a bit better now with method extracted :)

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:152:
Object[] nativeLibraryPathElements = null;
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: This null-assignment is unnecessary. It's always set (or exception is
> thrown, and you return), in the try-block below.

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:168:
dstDir.mkdir();
On 2015/09/16 06:30:45, nyquist (OOO - back 9-18) wrote:
> Nit: These three operations can also fail. What would we do if they did?

Acknowledged.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:189:
dest.setReadable(true, false);
On 2015/09/16 06:30:45, nyquist (OOO - back 9-18) wrote:
> Nit: Same as above for these three.

Acknowledged.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java:196:
mAppFilesSubDir.mkdir();
On 2015/09/16 06:30:45, nyquist (OOO - back 9-18) wrote:
> Nit: Same as above for these two.

Acknowledged.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
File
build/android/incremental_install/java/org/chromium/incrementalinstall/LockFile.java
(right):

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/LockFile.java:36:
// On Android M, we can't delete files in /data/local/tmp, so we write to it
instead.
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: Should this be "Android M+"?

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/LockFile.java:57:
final long pollIntervalMs = 200;
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: Unnecessary final

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/LockFile.java:91:
static LockFile acquireRuntimeLock(final File file) {
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: Unnecessary final

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/LockFile.java:124:
mFile.delete();
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: Do we need to do anything special if this operation fails? Log something?

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
File
build/android/incremental_install/java/org/chromium/incrementalinstall/Reflect.java
(right):

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/Reflect.java:56:
* type.
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: How do you feel about adding an assert (or just throw) that the types are
> the same?

It will already throw if the types are incompatible (the arraycopy throws)

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/Reflect.java:59:
Object[] result = (Object[]) (Array.newInstance(
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: paranthesis around Array.newInstance(...) call is unnecessary.

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/Reflect.java:71:
instance = null;
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: This reassigns a parameter value to null which was at first read of this
> method odd to me. I wonder if this could be easier to read if we did something
> like:
> boolean isStatic;
> if ( is class ) {
>   isStatic = true;
>   ..
> } else {
>   isStatic = false;
>   ...
> }
> ...
> if (isStatic) return method.invoke(null, params);
> return method.invoke(instance, params);
> 
> or something along those lines?

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/Reflect.java:85:
static Object newInstance(Class<?> clazz, Object... params)
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: Could you add a comment about whether this works as expected in no-arg
> constructors?

Done.

https://codereview.chromium.org/1338813003/diff/60001/build/android/increment...
build/android/incremental_install/java/org/chromium/incrementalinstall/Reflect.java:107:
} catch (NoSuchFieldException e) {
On 2015/09/16 06:30:46, nyquist (OOO - back 9-18) wrote:
> Nit: Could you add the boilerplate-like '// This shouldn't happen.' here to
keep
> me sane? :-p

Done.

[nyquist, 2015-09-16 15:36:22]

lgtm

[jbudorick, 2015-09-16 15:38:00]

https://codereview.chromium.org/1338813003/diff/60001/build/config/android/ru...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1338813003/diff/60001/build/config/android/ru...
build/config/android/rules.gni:1153: _jar_path = "$base_path.jar"
possible nit: leave as jar_path and use forward_variables_from where used below?

(and similarly below)

https://codereview.chromium.org/1338813003/diff/100001/build/config/android/i...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1338813003/diff/100001/build/config/android/i...
build/config/android/internal_rules.gni:679: deps = invoker.deps
forward_variables_from?

[agrieve, 2015-09-16 15:50:47]

https://codereview.chromium.org/1338813003/diff/60001/build/config/android/ru...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1338813003/diff/60001/build/config/android/ru...
build/config/android/rules.gni:1153: _jar_path = "$base_path.jar"
On 2015/09/16 15:38:00, jbudorick wrote:
> possible nit: leave as jar_path and use forward_variables_from where used
below?
> 
> (and similarly below)

It's a "feature" of GN (crbug/469338) that variables in the outer scope get
pulled into inner scopes automatically. Good for setting "testonly=true" at the
top of a template, but in this case having jar_path here cause it to be
implicitly picked up by write_build_config and java_library_impl, which tripped
me up when making these changes.

https://codereview.chromium.org/1338813003/diff/100001/build/config/android/i...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1338813003/diff/100001/build/config/android/i...
build/config/android/internal_rules.gni:679: deps = invoker.deps
On 2015/09/16 15:38:00, jbudorick wrote:
> forward_variables_from?

In this case I want it to be an error if deps isn't set in the invoker.

[jbudorick, 2015-09-16 15:52:10]

lgtm

On 2015/09/16 at 15:50:47, agrieve wrote:
>
https://codereview.chromium.org/1338813003/diff/60001/build/config/android/ru...
> File build/config/android/rules.gni (right):
> 
>
https://codereview.chromium.org/1338813003/diff/60001/build/config/android/ru...
> build/config/android/rules.gni:1153: _jar_path = "$base_path.jar"
> On 2015/09/16 15:38:00, jbudorick wrote:
> > possible nit: leave as jar_path and use forward_variables_from where used
below?
> > 
> > (and similarly below)
> 
> It's a "feature" of GN (crbug/469338) that variables in the outer scope get
pulled into inner scopes automatically. Good for setting "testonly=true" at the
top of a template, but in this case having jar_path here cause it to be
implicitly picked up by write_build_config and java_library_impl, which tripped
me up when making these changes.

oh goody, I love "features"

> 
>
https://codereview.chromium.org/1338813003/diff/100001/build/config/android/i...
> File build/config/android/internal_rules.gni (right):
> 
>
https://codereview.chromium.org/1338813003/diff/100001/build/config/android/i...
> build/config/android/internal_rules.gni:679: deps = invoker.deps
> On 2015/09/16 15:38:00, jbudorick wrote:
> > forward_variables_from?
> 
> In this case I want it to be an error if deps isn't set in the invoker.

[jbudorick, 2015-09-16 15:52:10]

lgtm

On 2015/09/16 at 15:50:47, agrieve wrote:
>
https://codereview.chromium.org/1338813003/diff/60001/build/config/android/ru...
> File build/config/android/rules.gni (right):
> 
>
https://codereview.chromium.org/1338813003/diff/60001/build/config/android/ru...
> build/config/android/rules.gni:1153: _jar_path = "$base_path.jar"
> On 2015/09/16 15:38:00, jbudorick wrote:
> > possible nit: leave as jar_path and use forward_variables_from where used
below?
> > 
> > (and similarly below)
> 
> It's a "feature" of GN (crbug/469338) that variables in the outer scope get
pulled into inner scopes automatically. Good for setting "testonly=true" at the
top of a template, but in this case having jar_path here cause it to be
implicitly picked up by write_build_config and java_library_impl, which tripped
me up when making these changes.

oh goody, I love "features"

> 
>
https://codereview.chromium.org/1338813003/diff/100001/build/config/android/i...
> File build/config/android/internal_rules.gni (right):
> 
>
https://codereview.chromium.org/1338813003/diff/100001/build/config/android/i...
> build/config/android/internal_rules.gni:679: deps = invoker.deps
> On 2015/09/16 15:38:00, jbudorick wrote:
> > forward_variables_from?
> 
> In this case I want it to be an error if deps isn't set in the invoker.

[agrieve, 2015-09-16 17:23:38]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-16 17:24:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1338813003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1338813003/100001

[commit-bot: I haz the power, 2015-09-16 19:02:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-16 19:02:40]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Lei Zhang, 2015-09-16 19:19:37]

rs lgtm

[Lei Zhang, 2015-09-16 19:19:43]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-09-16 19:20:47]

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

[commit-bot: I haz the power, 2015-09-16 19:56:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-16 19:57:50]

Patchset 6 (id:??) landed as
https://crrev.com/933f2593199c4f95678ddda884aa476ff1fe0c23
Cr-Commit-Position: refs/heads/master@{#349200}

[commit-bot: I haz the power, 2015-09-23 12:59:43]

Patchset 6 (id:??) landed as
https://crrev.com/933f2593199c4f95678ddda884aa476ff1fe0c23
Cr-Commit-Position: refs/heads/master@{#349200}