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

build/android/incremental_install/java/org/chromium/incrementalinstall/ClassLoaderPatcher.java

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.incrementalinstall;
+
+import android.content.Context;
+import android.os.Build;
+import android.util.Log;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ * Provides the ability to add native libraries and .dex files to an existing class loader.
+ * Tested with Jellybean MR2 - Marshmellow.
+ */
+final class ClassLoaderPatcher {
+    private static final String TAG = "cr.incrementalinstall";
+    private final File mAppFilesSubDir;
+    private final ClassLoader mClassLoader;
+    private final Object mLibcoreOs;
+    private final int mProcessUid;
+    final boolean mIsPrimaryProcess;
+
+    ClassLoaderPatcher(Context context) throws ReflectiveOperationException {

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

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...

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

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).

+        mAppFilesSubDir =
+                new File(context.getApplicationInfo().dataDir, "incremental-install-files");
+        mClassLoader = context.getClassLoader();
+        mLibcoreOs = Reflect.getField(Class.forName("libcore.io.Libcore"), "os");
+        mProcessUid = (Integer) Reflect.invokeMethod(mLibcoreOs, "getuid");

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

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.

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

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

+        mIsPrimaryProcess = context.getApplicationInfo().uid == mProcessUid;
+        Log.i(TAG, "uid=" + mProcessUid + " (isPrimary=" + mIsPrimaryProcess + ")");
+    }
+
+    /**
+     * Loads all dex files within |dexDir| into the app's ClassLoader.
+     */
+    void loadDexFiles(File dexDir) throws ReflectiveOperationException, FileNotFoundException {
+        Log.i(TAG, "Installing dex files from: " + dexDir);
+        File[] dexFilesArr = dexDir.listFiles();
+        if (dexFilesArr == null) {
+            throw new FileNotFoundException("Dex dir does not exist: " + dexDir);
+        }
+        // The optimized dex files will be owned by this process' user.
+        // Store them within the app's data dir rather than on /data/local/tmp
+        // so that they are still deleted (by the OS) when we uninstall
+        // (even on a non-rooted device).
+        File incrementalDexesDir = new File(mAppFilesSubDir, "optimized-dexes");
+        File isolatedDexesDir = new File(mAppFilesSubDir, "isolated-dexes");
+        File optimizedDir;
+
+        if (mIsPrimaryProcess) {
+            ensureAppFilesSubDirExists();
+            // Allows isolated processes to access the same files.
+            incrementalDexesDir.mkdir();

[nyquist, 2015/09/16 06:30:45]

Nit: What if this fails? (same below)

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

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.

+            incrementalDexesDir.setReadable(true, false);
+            incrementalDexesDir.setExecutable(true, false);
+            // Create a directory for isolated processes to create directories in.
+            isolatedDexesDir.mkdir();
+            isolatedDexesDir.setWritable(true, false);
+            isolatedDexesDir.setExecutable(true, false);
+
+            optimizedDir = incrementalDexesDir;
+        } else {
+            // There is a UID check of the directory in dalvik.system.DexFile():
+            // https://android.googlesource.com/platform/libcore/+/45e0260/dalvik/src/main/java/dalvik/system/DexFile.java#101
+            // Rather than have each isolated process run DexOpt though, we use
+            // symlinks within the directory to point at the browser process'
+            // optimized dex files.
+            optimizedDir = new File(isolatedDexesDir, "isolated-" + mProcessUid);
+            optimizedDir.mkdir();

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

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?

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

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).

+            // Always wipe it out and re-create for simplicity.
+            Log.i(TAG, "Creating dex file symlinks for isolated process");
+            for (File f : optimizedDir.listFiles()) {
+                f.delete();

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

Nit: This operation might fail, and therefore it returns a boolean. Would we
want to bail out if the delete fails?

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

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

+            }
+            for (File f : incrementalDexesDir.listFiles()) {
+                String to = "../../" + incrementalDexesDir.getName() + "/" + f.getName();
+                File from = new File(optimizedDir, f.getName());
+                createSymlink(to, from);
+            }
+        }
+
+        Log.i(TAG, "Code cache dir: " + optimizedDir);
+        // TODO(agrieve): Might need to recored classpath ordering if we ever have duplicate

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

Nit: record*

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

Done.

+        //     class names (since then order will matter here).
+        ArrayList<File> dexFiles = new ArrayList<File>(Arrays.asList(dexFilesArr));

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

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?

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

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

+        Log.i(TAG, "Loading " + dexFiles.size() + " dex files");
+
+        Object dexPathList = Reflect.getField(mClassLoader, "pathList");
+        Object[] dexElements = (Object[]) Reflect.getField(dexPathList, "dexElements");
+        Object[] additionalElements = makeDexElements(dexFilesArr, optimizedDir);
+        Reflect.setField(
+                dexPathList, "dexElements", Reflect.concatArrays(dexElements, additionalElements));
+    }
+
+    /**
+     * Sets up all libraries within |libDir| to be loadable by System.loadLibrary().
+     */
+    void importNativeLibs(File libDir) throws ReflectiveOperationException, IOException {
+        Log.i(TAG, "Importing native libraries from: " + libDir);
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
+            File localLibsDir = new File(mAppFilesSubDir, "lib");

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

Nit: Could you extract this block to something like:
libDir = prepareLocalLibsAndroidM();
?

[agrieve, 2015/09/16 14:59:58]

Done.

+            File copyLibsLockFile = new File(mAppFilesSubDir, "libcopy.lock");
+            // Due to a new SELinux policy, all libs must be copied into the app's
+            // data directory first.
+            // https://code.google.com/p/android/issues/detail?id=79480
+            if (mIsPrimaryProcess) {
+                LockFile lockFile = LockFile.acquireRuntimeLock(copyLibsLockFile);
+                if (lockFile != null) {

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

Nit: Could you flip these if-blocks, so it reads if (lockFile == null) { ... }
else { ... } ?

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

Done.

+                    try {
+                        copyNativeLibraryFiles(libDir, localLibsDir);
+                    } finally {
+                        lockFile.release();
+                    }
+                } else {
+                    LockFile.waitForRuntimeLock(copyLibsLockFile, 10 * 1000);
+                }
+            } else {
+                throw new RuntimeException("Incremental install does not work on Android M+ "
+                        + "with isolated processes. Use the gn arg:\n"
+                        + "    disable_incremental_isolated_processes=true\n"
+                        + "and try again.");
+            }
+            libDir = localLibsDir;

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

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 :-)

[agrieve, 2015/09/16 14:59:58]

Looks a bit better now with method extracted :)

+        }
+        addNativeLibrarySearchPath(libDir);
+    }
+
+    @SuppressWarnings("unchecked")
+    private void addNativeLibrarySearchPath(File nativeLibDir) throws ReflectiveOperationException {
+        Object dexPathList = Reflect.getField(mClassLoader, "pathList");
+        Object currentDirs = Reflect.getField(dexPathList, "nativeLibraryDirectories");
+        File[] newDirs = new File[] { nativeLibDir };
+        // Switched from an array to an ArrayList in Lollipop.
+        if (currentDirs instanceof List) {
+            List<File> dirsAsList = (List<File>) currentDirs;
+            dirsAsList.add(nativeLibDir);
+        } else {
+            File[] dirsAsArray = (File[]) currentDirs;
+            Reflect.setField(dexPathList, "nativeLibraryDirectories",
+                    Reflect.concatArrays(dirsAsArray, newDirs));
+        }
+
+        Object[] nativeLibraryPathElements = null;

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

Nit: This null-assignment is unnecessary. It's always set (or exception is
thrown, and you return), in the try-block below.

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

Done.

+        try {
+            nativeLibraryPathElements =
+                    (Object[]) Reflect.getField(dexPathList, "nativeLibraryPathElements");
+        } catch (NoSuchFieldException e) {
+            // This field doesn't exist pre-M.
+            return;
+        }
+        Object[] additionalElements = makeNativePathElements(newDirs);
+        Reflect.setField(
+                dexPathList, "nativeLibraryPathElements",
+                Reflect.concatArrays(nativeLibraryPathElements, additionalElements));
+    }
+
+    private void copyNativeLibraryFiles(File srcDir, File dstDir) throws IOException {
+        ensureAppFilesSubDirExists();
+        dstDir.mkdir();

[nyquist, 2015/09/16 06:30:45]

Nit: These three operations can also fail. What would we do if they did?

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

Acknowledged.

+        dstDir.setReadable(true, false);
+        dstDir.setExecutable(true, false);
+
+        // No need to delete stale libs since libraries are loaded explicitly.
+        for (File f : srcDir.listFiles()) {
+            // Note: Tried using hardlinks, but results in EACCES exception.
+            File dest = new File(dstDir, f.getName());
+            copyIfModified(f, dest);
+        }
+    }
+
+    private static void copyIfModified(File src, File dest) throws IOException {
+        long lastModified = src.lastModified();
+        if (!dest.exists() || dest.lastModified() != lastModified) {
+            Log.i(TAG, "Copying " + src + " -> " + dest);
+            FileInputStream istream = new FileInputStream(src);
+            FileOutputStream ostream = new FileOutputStream(dest);
+            ostream.getChannel().transferFrom(istream.getChannel(), 0, istream.getChannel().size());
+            istream.close();
+            ostream.close();
+            dest.setReadable(true, false);

[nyquist, 2015/09/16 06:30:45]

Nit: Same as above for these three.

[agrieve, 2015/09/16 14:59:58]

Acknowledged.

+            dest.setExecutable(true,  false);
+            dest.setLastModified(lastModified);
+        }
+    }
+
+    private void ensureAppFilesSubDirExists() {
+        mAppFilesSubDir.mkdir();

[nyquist, 2015/09/16 06:30:45]

Nit: Same as above for these two.

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

Acknowledged.

+        mAppFilesSubDir.setExecutable(true, false);
+    }
+
+    private void createSymlink(String to, File from) throws ReflectiveOperationException {
+        Reflect.invokeMethod(mLibcoreOs, "symlink", to, from.getAbsolutePath());
+    }
+
+    private static Object[] makeNativePathElements(File[] paths)
+            throws ReflectiveOperationException {
+        Class<?> entryClazz = Class.forName("dalvik.system.DexPathList$Element");
+        Object[] entries = new Object[paths.length];
+        for (int i = 0; i < paths.length; ++i) {
+            entries[i] = Reflect.newInstance(entryClazz, paths[i], true, null, null);
+        }
+        return entries;
+    }
+
+    private static Object[] makeDexElements(File[] files, File optimizedDirectory)
+            throws ReflectiveOperationException {
+        Class<?> entryClazz = Class.forName("dalvik.system.DexPathList$Element");
+        Class<?> clazz = Class.forName("dalvik.system.DexPathList");
+        Object[] entries = new Object[files.length];
+        File emptyDir = new File("");
+        for (int i = 0; i < files.length; ++i) {
+            File file = files[i];
+            Object dexFile = Reflect.invokeMethod(clazz, "loadDexFile", file, optimizedDirectory);
+            entries[i] = Reflect.newInstance(entryClazz, emptyDir, false, file, dexFile);
+        }
+        return entries;
+    }
+}