Revert of Changes caching logic of mojo java apps (patchset #5 id:80001 of...

Revert of Changes caching logic of mojo java apps (patchset #5 id:80001 of https://codereview.chromium.org/1149813002/)

Reason for revert:
Broke windows build.

Original issue's description:
> Changes caching logic of mojo java apps
>
> Previously we would extract all necessary files every time we ran the
> app. This is obviously unnecessary for any bundled apps. Now we
> extract only as necessary.
>
> R=ben@chromium.org, jcivelli@chromium.org
> BUG=none
> TEST=none
>
> Committed: https://crrev.com/128f7a0181634a89a35f681baab1d086d100e377
> Cr-Commit-Position: refs/heads/master@{#330824}

TBR=ben@chromium.org,jcivelli@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=none

Review URL: https://codereview.chromium.org/1147203002

Cr-Commit-Position: refs/heads/master@{#330832}

mojo/runner/android/android_handler.cc

@@ -15,8 +15,6 @@
 #include "mojo/public/c/system/main.h"
 #include "mojo/runner/android/run_android_application_function.h"
 #include "mojo/runner/native_application_support.h"
-#include "mojo/util/filename_util.h"
-#include "url/gurl.h"
 
 using base::android::AttachCurrentThread;
 using base::android::ScopedJavaLocalRef;
@@ -36,17 +34,15 @@ namespace {
 void RunAndroidApplication(JNIEnv* env,
                            jobject j_context,
                            const base::FilePath& app_path,
-                           jint j_handle,
-                           bool is_cached_app) {
+                           jint j_handle) {
   InterfaceRequest<Application> application_request =
       MakeRequest<Application>(MakeScopedHandle(MessagePipeHandle(j_handle)));
 
   // Load the library, so that we can set the application context there if
   // needed.
   // TODO(vtl): We'd use a ScopedNativeLibrary, but it doesn't have .get()!
-  base::NativeLibrary app_library = LoadNativeApplication(
-      app_path, is_cached_app ? shell::NativeApplicationCleanup::DONT_DELETE
-                              : shell::NativeApplicationCleanup::DELETE);
+  base::NativeLibrary app_library =
+      LoadNativeApplication(app_path, shell::NativeApplicationCleanup::DELETE);
   if (!app_library)
     return;
 
@@ -73,57 +69,6 @@ void RunAndroidApplication(JNIEnv* env,
   base::UnloadNativeLibrary(app_library);
 }
 
-// Returns true if |url| denotes a cached app. If true |app_dir| is set to the
-// path of the directory for the app and |path_to_mojo| the path of the app's
-// .mojo file.
-bool IsCachedApp(JNIEnv* env,
-                 const GURL& url,
-                 base::FilePath* app_dir,
-                 base::FilePath* path_to_mojo) {
-  ScopedJavaLocalRef<jstring> j_local_apps_dir =
-      Java_AndroidHandler_getLocalAppsDir(env, GetApplicationContext());
-  const base::FilePath local_apps_fp(
-      ConvertJavaStringToUTF8(env, j_local_apps_dir.obj()));
-  const std::string local_apps(util::FilePathToFileURL(local_apps_fp).spec());
-  const std::string response_url(GURL(url).spec());
-  if (response_url.size() <= local_apps.size() ||
-      local_apps.compare(0u, local_apps.size(), response_url, 0u,
-                         local_apps.size()) != 0) {
-    return false;
-  }
-
-  const std::string mojo_suffix(".mojo");
-  // app_rel_path is either something like html_viewer/html_viewer.mojo, or
-  // html_viewer.mojo, depending upon whether the app has a package.
-  const std::string app_rel_path(response_url.substr(local_apps.size() + 1));
-  const size_t slash_index = app_rel_path.find('/');
-  if (slash_index != std::string::npos) {
-    const std::string tail =
-        app_rel_path.substr(slash_index + 1, std::string::npos);
-    const std::string head = app_rel_path.substr(0, slash_index);
-    if (head.find('/') != std::string::npos ||
-        tail.size() <= mojo_suffix.size() ||
-        tail.compare(tail.size() - mojo_suffix.size(), tail.size(),
-                     mojo_suffix) != 0) {
-      return false;
-    }
-    *app_dir = local_apps_fp.Append(head);
-    *path_to_mojo = app_dir->Append(tail);
-    return true;
-  }
-  if (app_rel_path.find('/') != std::string::npos ||
-      app_rel_path.size() <= mojo_suffix.size() ||
-      app_rel_path.compare(app_rel_path.size() - mojo_suffix.size(),
-                           mojo_suffix.size(), mojo_suffix) != 0) {
-    return false;
-  }
-
-  *app_dir = local_apps_fp.Append(
-      app_rel_path.substr(0, app_rel_path.size() - mojo_suffix.size()));
-  *path_to_mojo = local_apps_fp.Append(app_rel_path);
-  return true;
-}
-
 }  // namespace
 
 AndroidHandler::AndroidHandler() : content_handler_factory_(this) {
@@ -136,30 +81,13 @@ void AndroidHandler::RunApplication(
     InterfaceRequest<Application> application_request,
     URLResponsePtr response) {
   JNIEnv* env = AttachCurrentThread();
-  RunAndroidApplicationFn run_android_application_fn = &RunAndroidApplication;
-  if (!response->url.is_null()) {
-    base::FilePath internal_app_path;
-    base::FilePath path_to_mojo;
-    if (IsCachedApp(env, GURL(response->url), &internal_app_path,
-                    &path_to_mojo)) {
-      ScopedJavaLocalRef<jstring> j_internal_app_path(
-          ConvertUTF8ToJavaString(env, internal_app_path.value()));
-      ScopedJavaLocalRef<jstring> j_path_to_mojo(
-          ConvertUTF8ToJavaString(env, path_to_mojo.value()));
-      Java_AndroidHandler_bootstrapCachedApp(
-          env, GetApplicationContext(), j_path_to_mojo.obj(),
-          j_internal_app_path.obj(),
-          application_request.PassMessagePipe().release().value(),
-          reinterpret_cast<jlong>(run_android_application_fn));
-      return;
-    }
-  }
   ScopedJavaLocalRef<jstring> j_archive_path =
       Java_AndroidHandler_getNewTempArchivePath(env, GetApplicationContext());
   base::FilePath archive_path(
       ConvertJavaStringToUTF8(env, j_archive_path.obj()));
 
   common::BlockingCopyToFile(response->body.Pass(), archive_path);
+  RunAndroidApplicationFn run_android_application_fn = &RunAndroidApplication;
   Java_AndroidHandler_bootstrap(
       env, GetApplicationContext(), j_archive_path.obj(),
       application_request.PassMessagePipe().release().value(),

mojo/runner/android/apk/src/org/chromium/mojo/shell/Bootstrap.java

@@ -22,16 +22,14 @@ public class Bootstrap implements Runnable {
     private final File mApplicationNativeLibrary;
     private final int mHandle;
     private final long mRunApplicationPtr;
-    private final boolean mIsCachedApp;
 
     public Bootstrap(Context context, File bootstrapNativeLibrary, File applicationNativeLibrary,
-            Integer handle, Long runApplicationPtr, Boolean isCachedApp) {
+            Integer handle, Long runApplicationPtr) {
         mContext = context;
         mBootstrapNativeLibrary = bootstrapNativeLibrary;
         mApplicationNativeLibrary = applicationNativeLibrary;
         mHandle = handle;
         mRunApplicationPtr = runApplicationPtr;
-        mIsCachedApp = isCachedApp;
     }
 
     @Override
@@ -39,9 +37,9 @@ public class Bootstrap implements Runnable {
         System.load(mBootstrapNativeLibrary.getAbsolutePath());
         System.load(mApplicationNativeLibrary.getAbsolutePath());
         nativeBootstrap(mContext, mApplicationNativeLibrary.getAbsolutePath(), mHandle,
-                mRunApplicationPtr, mIsCachedApp);
+                mRunApplicationPtr);
     }
 
     native void nativeBootstrap(Context context, String libraryPath, int handle,
-            long runApplicationPtr, boolean isCachedApp);
+            long runApplicationPtr);
 }

mojo/runner/android/apk/src/org/chromium/mojo/shell/FileHelper.java

@@ -7,8 +7,7 @@ package org.chromium.mojo.shell;
 import android.content.Context;
 import android.content.pm.PackageInfo;
 import android.content.pm.PackageManager;
-
-import org.chromium.base.Log;
+import android.util.Log;
 
 import java.io.BufferedInputStream;
 import java.io.BufferedOutputStream;
@@ -16,6 +15,7 @@ import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
+import java.io.FilenameFilter;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
@@ -35,88 +35,63 @@ class FileHelper {
     // Prefix used when naming timestamp files.
     private static final String TIMESTAMP_PREFIX = "asset_timestamp-";
 
-    /**
-     * Used to indicate the type of destination file that should be created.
-     */
-    public enum FileType {
-        TEMPORARY,
-        PERMANENT,
-    }
-
-    public enum ArchiveType {
-        /**
-         * The archive was created for a content handler (contains the mojo escape sequence).
-         */
-        CONTENT_HANDLER,
-        NORMAL,
-    }
-
     /**
      * Looks for a timestamp file on disk that indicates the version of the APK that the resource
      * assets were extracted from. Returns null if a timestamp was found and it indicates that the
-     * resources match the current APK. Otherwise returns the file to create.
+     * resources match the current APK. Otherwise returns a String that represents the filename of a
+     * timestamp to create.
      */
-    private static File findAssetTimestamp(Context context, File outputDir) {
+    private static String checkAssetTimestamp(Context context, File outputDir) {
         PackageManager pm = context.getPackageManager();
         PackageInfo pi = null;
 
         try {
             pi = pm.getPackageInfo(context.getPackageName(), 0);
         } catch (PackageManager.NameNotFoundException e) {
+            return TIMESTAMP_PREFIX;
         }
 
         if (pi == null) {
-            return new File(outputDir, TIMESTAMP_PREFIX);
+            return TIMESTAMP_PREFIX;
         }
 
-        final File expectedTimestamp =
-                new File(outputDir, TIMESTAMP_PREFIX + pi.versionCode + "-" + pi.lastUpdateTime);
-        return expectedTimestamp.exists() ? null : expectedTimestamp;
-    }
+        String expectedTimestamp = TIMESTAMP_PREFIX + pi.versionCode + "-" + pi.lastUpdateTime;
 
-    /**
-     * Invoke prior to extracting any assets into {@code directory}. If necessary deletes all the
-     * files in the specified directory. The return value must be supplied to {@link
-     *createTimestampIfNecessary}.
-     *
-     * @param directory directory assets will be extracted to
-     * @return non-null if a file with the specified name needs to be created after assets have
-     * been extracted.
-     */
-    public static File prepareDirectoryForAssets(Context context, File directory) {
-        final File timestamp = findAssetTimestamp(context, directory);
-        if (timestamp == null) {
-            return null;
-        }
-        for (File child : directory.listFiles()) {
-            deleteRecursively(child);
-        }
-        return timestamp;
-    }
+        String[] timestamps = outputDir.list(new FilenameFilter() {
+            @Override
+            public boolean accept(File dir, String name) {
+                return name.startsWith(TIMESTAMP_PREFIX);
+            }
+        });
 
-    /**
-     * Creates a file used as a timestamp. The supplied file comes from {@link
-     *prepareDirectoryForAssets}.
-     *
-     * @param timestamp path of file to create, or null if a file does not need to be created
-     */
-    public static void createTimestampIfNecessary(File timestamp) {
-        if (timestamp == null) {
-            return;
+        if (timestamps.length != 1) {
+            // If there's no timestamp, nuke to be safe as we can't tell the age of the files.
+            // If there's multiple timestamps, something's gone wrong so nuke.
+            return expectedTimestamp;
         }
-        try {
-            timestamp.createNewFile();
-        } catch (IOException e) {
-            // In the worst case we don't write a timestamp, so we'll re-extract the asset next
-            // time.
-            Log.w(TAG, "Failed to write asset timestamp!");
+
+        if (!expectedTimestamp.equals(timestamps[0])) {
+            return expectedTimestamp;
         }
+
+        // Timestamp file is already up-to date.
+        return null;
     }
 
     public static File extractFromAssets(Context context, String assetName, File outputDirectory,
-            FileType fileType) throws IOException, FileNotFoundException {
+            boolean useTempFile) throws IOException, FileNotFoundException {
+        String timestampToCreate = null;
+        if (!useTempFile) {
+            timestampToCreate = checkAssetTimestamp(context, outputDirectory);
+            if (timestampToCreate != null) {
+                for (File child : outputDirectory.listFiles()) {
+                    deleteRecursively(child);
+                }
+            }
+        }
+
         File outputFile;
-        if (fileType == FileType.TEMPORARY) {
+        if (useTempFile) {
             // Make the original filename part of the temp file name.
             // TODO(ppi): do we need to sanitize the suffix?
             String suffix = "-" + assetName;
@@ -136,48 +111,36 @@ class FileHelper {
         } finally {
             inputStream.close();
         }
+
+        if (timestampToCreate != null) {
+            try {
+                new File(outputDirectory, timestampToCreate).createNewFile();
+            } catch (IOException e) {
+                // In the worst case we don't write a timestamp, so we'll re-extract the asset next
+                // time.
+                Log.w(TAG, "Failed to write asset timestamp!");
+            }
+        }
+
         return outputFile;
     }
 
     /**
      * Extracts the file of the given extension from the archive. Throws FileNotFoundException if no
      * matching file is found.
-     *
-     * @return path of extracted file
      */
-    static File extractFromArchive(File archive, String suffixToMatch, File outputDirectory,
-            FileType fileType, ArchiveType archiveType) throws IOException, FileNotFoundException {
-        if (!outputDirectory.exists() && !outputDirectory.mkdirs()) {
-            Log.e(TAG, "extractFromArchive unable to create directory "
-                            + outputDirectory.getAbsolutePath());
-            throw new FileNotFoundException();
-        }
-
-        BufferedInputStream inputStream = new BufferedInputStream(new FileInputStream(archive));
-        if (archiveType == ArchiveType.CONTENT_HANDLER) {
-            int currentChar;
-            do {
-                currentChar = inputStream.read();
-            } while (currentChar != -1 && currentChar != '\n');
-            if (currentChar == -1) {
-                throw new FileNotFoundException();
-            }
-            inputStream = new BufferedInputStream(inputStream);
-        }
-        ZipInputStream zip = new ZipInputStream(inputStream);
+    static File extractFromArchive(File archive, String suffixToMatch,
+            File outputDirectory) throws IOException, FileNotFoundException {
+        ZipInputStream zip = new ZipInputStream(new BufferedInputStream(new FileInputStream(
+                archive)));
         ZipEntry entry;
         while ((entry = zip.getNextEntry()) != null) {
             if (entry.getName().endsWith(suffixToMatch)) {
-                // TODO(sky): sanitize name.
-                final String name = new File(entry.getName()).getName();
-                File extractedFile;
                 // Make the original filename part of the temp file name.
-                if (fileType == FileType.TEMPORARY) {
-                    final String suffix = "-" + name;
-                    extractedFile = File.createTempFile(TEMP_FILE_PREFIX, suffix, outputDirectory);
-                } else {
-                    extractedFile = new File(outputDirectory, name);
-                }
+                // TODO(ppi): do we need to sanitize the suffix?
+                String suffix = "-" + new File(entry.getName()).getName();
+                File extractedFile = File.createTempFile(TEMP_FILE_PREFIX, suffix,
+                        outputDirectory);
                 writeStreamToFile(zip, extractedFile);
                 zip.close();
                 return extractedFile;

mojo/runner/android/apk/src/org/chromium/mojo/shell/ShellMain.java

@@ -27,8 +27,7 @@ import java.util.List;
 public class ShellMain {
     private static final String TAG = "ShellMain";
 
-    // Directory where applications cached with the shell will be extracted.
-    // TODO(sky): rename this to CACHED_APP_DIRECTORY.
+    // Directory where applications bundled with the shell will be extracted.
     private static final String LOCAL_APP_DIRECTORY = "local_apps";
     // The mojo_shell library is also an executable run in forked processes when running
     // multi-process.
@@ -73,13 +72,9 @@ public class ShellMain {
         if (sInitialized) return;
         File localAppsDir = getLocalAppsDir(applicationContext);
         try {
-            final File timestamp =
-                    FileHelper.prepareDirectoryForAssets(applicationContext, localAppsDir);
             for (String assetPath : getAssetsList(applicationContext)) {
-                FileHelper.extractFromAssets(
-                        applicationContext, assetPath, localAppsDir, FileHelper.FileType.PERMANENT);
+                FileHelper.extractFromAssets(applicationContext, assetPath, localAppsDir, false);
             }
-            FileHelper.createTimestampIfNecessary(timestamp);
             File mojoShell = new File(applicationContext.getApplicationInfo().nativeLibraryDir,
                     MOJO_SHELL_EXECUTABLE);
 
@@ -116,7 +111,7 @@ public class ShellMain {
         nativeAddApplicationURL(url);
     }
 
-    static File getLocalAppsDir(Context context) {
+    private static File getLocalAppsDir(Context context) {
         return context.getDir(LOCAL_APP_DIRECTORY, Context.MODE_PRIVATE);
     }
 
@@ -133,7 +128,7 @@ public class ShellMain {
      * Initializes the native system. This API should be called only once per process.
      **/
     private static native void nativeInit(Context context, String mojoShellPath,
-            String[] parameters, String cachedAppsDirectory, String tmpDir);
+            String[] parameters, String bundledAppsDirectory, String tmpDir);
 
     private static native boolean nativeStart();
 

mojo/runner/android/bootstrap.cc

@@ -17,13 +17,12 @@ void Bootstrap(JNIEnv* env,
                jobject j_context,
                jstring j_native_library_path,
                jint j_handle,
-               jlong j_run_application_ptr,
-               jboolean is_cached_app) {
+               jlong j_run_application_ptr) {
   base::FilePath app_path(
       base::android::ConvertJavaStringToUTF8(env, j_native_library_path));
   RunAndroidApplicationFn run_android_application_fn =
       reinterpret_cast<RunAndroidApplicationFn>(j_run_application_ptr);
-  run_android_application_fn(env, j_context, app_path, j_handle, is_cached_app);
+  run_android_application_fn(env, j_context, app_path, j_handle);
 }
 
 bool RegisterBootstrapJni(JNIEnv* env) {

mojo/runner/android/run_android_application_function.h

@@ -19,8 +19,7 @@ namespace runner {
 typedef void (*RunAndroidApplicationFn)(JNIEnv* env,
                                         jobject j_context,
                                         const base::FilePath& app_path,
-                                        jint j_handle,
-                                        bool is_cached_app);
+                                        jint j_handle);
 
 }  // namespace runner
 }  // namespace mojo

mojo/runner/android/tests/src/org/chromium/mojo/shell/ShellTestBase.java

@@ -31,8 +31,7 @@ public class ShellTestBase {
         AssetManager manager = context.getResources().getAssets();
         for (String asset : manager.list("")) {
             if (asset.endsWith(".mojo")) {
-                FileHelper.extractFromAssets(
-                        context, asset, outputDirectory, FileHelper.FileType.PERMANENT);
+                FileHelper.extractFromAssets(context, asset, outputDirectory, false);
             }
         }
 

mojo/runner/url_resolver.cc

@@ -114,13 +114,9 @@ GURL URLResolver::ResolveMojoURL(const GURL& mojo_url) const {
   if (mojo_base_url_.SchemeIsFile()) {
     const GURL url_with_directory(
         mojo_base_url_.Resolve(base_url.host() + "/"));
-    const base::FilePath dir(util::UrlToFilePath(url_with_directory));
-    if (base::DirectoryExists(dir)) {
-      const base::FilePath mojo_path = dir.Append(base_url.host() + ".mojo");
-      // Only use the directory if the .mojo exists in the directory.
-      if (base::PathExists(mojo_path))
-        return url_with_directory.Resolve(base_url.host() + ".mojo" + query);
-    }
+    const base::FilePath file_path(util::UrlToFilePath(url_with_directory));
+    if (base::DirectoryExists(file_path))
+      return url_with_directory.Resolve(base_url.host() + ".mojo" + query);
   }
   return mojo_base_url_.Resolve(base_url.host() + ".mojo" + query);
 }

mojo/runner/url_resolver_unittest.cc

@@ -159,23 +159,13 @@ TEST_F(URLResolverTest, PreferDirectory) {
   EXPECT_EQ(util::FilePathToFileURL(tmp_dir.path()).spec() + "/foo.mojo",
             mapped_url.spec());
 
-  // With an empty directory |mojo:foo| maps to path/foo.mojo.
+  // With a directory |mojo:foo| maps to path/foo/foo.mojo.
   const base::FilePath foo_file_path(
       tmp_dir.path().Append(FILE_PATH_LITERAL("foo")));
   ASSERT_TRUE(base::CreateDirectory(foo_file_path));
   const GURL mapped_url_with_dir = resolver.ResolveMojoURL(GURL("mojo:foo"));
-  EXPECT_EQ(util::FilePathToFileURL(tmp_dir.path()).spec() + "/foo.mojo",
-            mapped_url_with_dir.spec());
-
-  // When foo.mojo exists in the directory (path/foo/foo.mojo), then it should
-  // be picked up.
-  // With an empty directory |mojo:foo| maps to path/foo/foo.mojo.
-  ASSERT_EQ(1,
-            base::WriteFile(foo_file_path.Append(FILE_PATH_LITERAL("foo.mojo")),
-                            "a", 1));
-  const GURL mapped_url_in_dir = resolver.ResolveMojoURL(GURL("mojo:foo"));
   EXPECT_EQ(util::FilePathToFileURL(tmp_dir.path()).spec() + "/foo/foo.mojo",
-            mapped_url_in_dir.spec());
+            mapped_url_with_dir.spec());
 }
 
 }  // namespace