[WebLayer] Fix CrashReporterController crashing if used before WL init

This adds minimal initialization needed for CrashReporterController, as
well as delaying the call to processNewMinidumps() until after native
initialization has happened. Also cleans up the creation of
CrashReporterController to use AIDL instead of reflection.

Bug: 1027076
Change-Id: Ib1969c82eb93b88965de8ee793fa3d6eb46c7bf0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1951551
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Tobias Sargeant <tobiasjs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721747}

weblayer/browser/android/javatests/src/org/chromium/weblayer/test/CrashReporterTest.java

@@ -79,7 +79,9 @@ public class CrashReporterTest {
         BundleCallbackHelper callbackHelper = new BundleCallbackHelper();
         CallbackHelper deleteHelper = new CallbackHelper();
 
-        InstrumentationActivity activity = mActivityTestRule.launchShell(new Bundle());
+        Bundle extras = new Bundle();
+        extras.putBoolean(InstrumentationActivity.EXTRA_CREATE_WEBLAYER, false);
+        InstrumentationActivity activity = mActivityTestRule.launchShell(extras);
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             CrashReporterController crashReporterController =
                     CrashReporterController.getInstance(activity);

weblayer/browser/java/org/chromium/weblayer_private/CrashReporterControllerImpl.java

@@ -20,6 +20,7 @@ import org.chromium.components.crash.browser.ChildProcessCrashObserver;
 import org.chromium.components.minidump_uploader.CrashFileManager;
 import org.chromium.weblayer_private.interfaces.ICrashReporterController;
 import org.chromium.weblayer_private.interfaces.ICrashReporterControllerClient;
+import org.chromium.weblayer_private.interfaces.IObjectWrapper;
 import org.chromium.weblayer_private.interfaces.StrictModeWorkaround;
 
 import java.io.File;
@@ -41,15 +42,14 @@ public final class CrashReporterControllerImpl extends ICrashReporterController.
 
     @Nullable
     private ICrashReporterControllerClient mClient;
-    private final CrashFileManager mCrashFileManager;
+    private CrashFileManager mCrashFileManager;
+    private boolean mIsNativeInitialized;
 
     private static class Holder {
         static CrashReporterControllerImpl sInstance = new CrashReporterControllerImpl();
     }
 
     private CrashReporterControllerImpl() {
-        mCrashFileManager = new CrashFileManager(new File(PathUtils.getCacheDirectory()));
-
         ChildProcessCrashObserver.registerCrashCallback(
                 new ChildProcessCrashObserver.ChildCrashedCallback() {
                     @Override
@@ -59,10 +59,19 @@ public final class CrashReporterControllerImpl extends ICrashReporterController.
                 });
     }
 
-    public static CrashReporterControllerImpl getInstance() {
+    public static CrashReporterControllerImpl getInstance(IObjectWrapper appContextWrapper) {
+        // This is a no-op if init has already happened.
+        WebLayerImpl.minimalInitForContext(appContextWrapper);
         return Holder.sInstance;
     }
 
+    public void notifyNativeInitialized() {
+        mIsNativeInitialized = true;
+        if (mClient != null) {
+            processNewMinidumps();
+        }
+    }
+
     @Override
     public void deleteCrash(String localId) {
         StrictModeWorkaround.apply();
@@ -80,7 +89,7 @@ public final class CrashReporterControllerImpl extends ICrashReporterController.
     public void uploadCrash(String localId) {
         StrictModeWorkaround.apply();
         AsyncTask.THREAD_POOL_EXECUTOR.execute(() -> {
-            File minidumpFile = mCrashFileManager.getCrashFileWithLocalId(localId);
+            File minidumpFile = getCrashFileManager().getCrashFileWithLocalId(localId);
             MinidumpUploader.Result result = new MinidumpUploader().upload(minidumpFile);
             if (result.mSuccess) {
                 CrashFileManager.markUploadSuccess(minidumpFile);
@@ -135,7 +144,9 @@ public final class CrashReporterControllerImpl extends ICrashReporterController.
     public void setClient(ICrashReporterControllerClient client) {
         StrictModeWorkaround.apply();
         mClient = client;
-        processNewMinidumps();
+        if (mIsNativeInitialized) {
+            processNewMinidumps();
+        }
     }
 
     /** Start an async task to import crashes, and notify if any are found. */
@@ -154,7 +165,7 @@ public final class CrashReporterControllerImpl extends ICrashReporterController.
 
     /** Delete a crash report (and any sidecar file) given its local ID. */
     private void deleteCrashOnBackgroundThread(String localId) {
-        File minidumpFile = mCrashFileManager.getCrashFileWithLocalId(localId);
+        File minidumpFile = getCrashFileManager().getCrashFileWithLocalId(localId);
         File sidecarFile = sidecarFile(localId);
         if (minidumpFile != null) {
             CrashFileManager.deleteFile(minidumpFile);
@@ -173,8 +184,9 @@ public final class CrashReporterControllerImpl extends ICrashReporterController.
      * @return An array of local IDs for crashes that are ready to be uploaded.
      */
     private String[] getPendingMinidumpsOnBackgroundThread() {
-        mCrashFileManager.cleanOutAllNonFreshMinidumpFiles();
-        File[] pendingMinidumps = mCrashFileManager.getMinidumpsReadyForUpload(MAX_UPLOAD_RETRIES);
+        getCrashFileManager().cleanOutAllNonFreshMinidumpFiles();
+        File[] pendingMinidumps =
+                getCrashFileManager().getMinidumpsReadyForUpload(MAX_UPLOAD_RETRIES);
         ArrayList<String> localIds = new ArrayList<>(pendingMinidumps.length);
         for (File minidump : pendingMinidumps) {
             localIds.add(CrashFileManager.getCrashLocalIdFromFileName(minidump.getName()));
@@ -192,7 +204,7 @@ public final class CrashReporterControllerImpl extends ICrashReporterController.
      */
     private String[] processNewMinidumpsOnBackgroundThread() {
         Map<String, Map<String, String>> crashesInfoMap =
-                mCrashFileManager.importMinidumpsCrashKeys();
+                getCrashFileManager().importMinidumpsCrashKeys();
         ArrayList<String> localIds = new ArrayList<>(crashesInfoMap.size());
         for (Map.Entry<String, Map<String, String>> entry : crashesInfoMap.entrySet()) {
             JSONObject crashKeysJson = new JSONObject(entry.getValue());
@@ -202,7 +214,7 @@ public final class CrashReporterControllerImpl extends ICrashReporterController.
             localIds.add(CrashFileManager.getCrashLocalIdFromFileName(uuid + ".dmp"));
             writeSidecar(uuid, crashKeysJson);
         }
-        for (File minidump : mCrashFileManager.getMinidumpsSansLogcat()) {
+        for (File minidump : getCrashFileManager().getMinidumpsSansLogcat()) {
             CrashFileManager.trySetReadyForUpload(minidump);
         }
         return localIds.toArray(new String[0]);
@@ -215,7 +227,7 @@ public final class CrashReporterControllerImpl extends ICrashReporterController.
      * with the crash. All crash keys and values are strings.
      */
     private @Nullable File sidecarFile(String localId) {
-        File minidumpFile = mCrashFileManager.getCrashFileWithLocalId(localId);
+        File minidumpFile = getCrashFileManager().getCrashFileWithLocalId(localId);
         if (minidumpFile == null) {
             return null;
         }
@@ -256,4 +268,15 @@ public final class CrashReporterControllerImpl extends ICrashReporterController.
             return null;
         }
     }
+
+    private CrashFileManager getCrashFileManager() {
+        if (mCrashFileManager == null) {
+            File cacheDir = new File(PathUtils.getCacheDirectory());
+            // Make sure the cache dir has been created, since this may be called before WebLayer
+            // has been initialized.
+            cacheDir.mkdir();
+            mCrashFileManager = new CrashFileManager(cacheDir);
+        }
+        return mCrashFileManager;
+    }
 }

weblayer/browser/java/org/chromium/weblayer_private/WebLayerImpl.java

@@ -35,6 +35,7 @@ import org.chromium.content_public.browser.ChildProcessCreationParams;
 import org.chromium.content_public.browser.DeviceUtils;
 import org.chromium.ui.base.ResourceBundle;
 import org.chromium.weblayer_private.interfaces.IBrowserFragment;
+import org.chromium.weblayer_private.interfaces.ICrashReporterController;
 import org.chromium.weblayer_private.interfaces.IObjectWrapper;
 import org.chromium.weblayer_private.interfaces.IProfile;
 import org.chromium.weblayer_private.interfaces.IRemoteFragmentClient;
@@ -77,6 +78,22 @@ public final class WebLayerImpl extends IWebLayer.Stub {
 
     WebLayerImpl() {}
 
+    /**
+     * Performs the minimal initialization needed for a context. This is used for example in
+     * CrashReporterControllerImpl, so it can be used before full WebLayer initialization.
+     */
+    public static Context minimalInitForContext(IObjectWrapper appContextWrapper) {
+        if (ContextUtils.getApplicationContext() != null) {
+            return ContextUtils.getApplicationContext();
+        }
+        // Wrap the app context so that it can be used to load WebLayer implementation classes.
+        Context appContext = ClassLoaderContextWrapperFactory.get(
+                ObjectWrapper.unwrap(appContextWrapper, Context.class));
+        ContextUtils.initApplicationContext(appContext);
+        PathUtils.setPrivateDataDirectorySuffix(PRIVATE_DIRECTORY_SUFFIX, PRIVATE_DIRECTORY_SUFFIX);
+        return appContext;
+    }
+
     @Override
     public void loadAsync(IObjectWrapper appContextWrapper, IObjectWrapper loadedCallbackWrapper) {
         StrictModeWorkaround.apply();
@@ -90,6 +107,8 @@ public final class WebLayerImpl extends IWebLayer.Stub {
                         new BrowserStartupController.StartupCallback() {
                             @Override
                             public void onSuccess() {
+                                CrashReporterControllerImpl.getInstance(appContextWrapper)
+                                        .notifyNativeInitialized();
                                 loadedCallback.onReceiveValue(true);
                             }
                             @Override
@@ -107,6 +126,7 @@ public final class WebLayerImpl extends IWebLayer.Stub {
         BrowserStartupController.get(LibraryProcessType.PROCESS_WEBLAYER)
                 .startBrowserProcessesSync(
                         /* singleProcess*/ false);
+        CrashReporterControllerImpl.getInstance(appContextWrapper).notifyNativeInitialized();
     }
 
     private void init(IObjectWrapper appContextWrapper) {
@@ -117,9 +137,7 @@ public final class WebLayerImpl extends IWebLayer.Stub {
 
         UmaUtils.recordMainEntryPointTime();
 
-        // Wrap the app context so that it can be used to load WebLayer implementation classes.
-        Context appContext = ClassLoaderContextWrapperFactory.get(
-                ObjectWrapper.unwrap(appContextWrapper, Context.class));
+        Context appContext = minimalInitForContext(appContextWrapper);
         PackageInfo packageInfo = WebViewFactory.getLoadedPackageInfo();
 
         // TODO: This can break some functionality of apps that are doing interesting things with
@@ -127,7 +145,6 @@ public final class WebLayerImpl extends IWebLayer.Stub {
         addWebViewAssetPath(appContext, packageInfo);
 
         applySplitApkWorkaround(packageInfo.applicationInfo, appContext.getResources().getAssets());
-        ContextUtils.initApplicationContext(appContext);
         BuildInfo.setBrowserPackageInfo(packageInfo);
         int resourcesPackageId = getPackageId(appContext, packageInfo.packageName);
         // TODO: The call to onResourcesLoaded() can be slow, we may need to parallelize this with
@@ -135,7 +152,6 @@ public final class WebLayerImpl extends IWebLayer.Stub {
         R.onResourcesLoaded(resourcesPackageId);
 
         ResourceBundle.setAvailablePakLocales(new String[] {}, ProductConfig.UNCOMPRESSED_LOCALES);
-        PathUtils.setPrivateDataDirectorySuffix(PRIVATE_DIRECTORY_SUFFIX, PRIVATE_DIRECTORY_SUFFIX);
 
         ChildProcessCreationParams.set(appContext.getPackageName(), false /* isExternalService */,
                 LibraryProcessType.PROCESS_WEBLAYER_CHILD, true /* bindToCaller */,
@@ -188,6 +204,11 @@ public final class WebLayerImpl extends IWebLayer.Stub {
         return WebLayerImplJni.get().isRemoteDebuggingEnabled();
     }
 
+    @Override
+    public ICrashReporterController getCrashReporterController(IObjectWrapper appContext) {
+        return CrashReporterControllerImpl.getInstance(appContext);
+    }
+
     private static void addWebViewAssetPath(Context appContext, PackageInfo packageInfo) {
         try {
             if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP_MR1) {

weblayer/browser/java/org/chromium/weblayer_private/interfaces/IWebLayer.aidl

@@ -7,6 +7,7 @@ package org.chromium.weblayer_private.interfaces;
 import android.os.Bundle;
 
 import org.chromium.weblayer_private.interfaces.IBrowserFragment;
+import org.chromium.weblayer_private.interfaces.ICrashReporterController;
 import org.chromium.weblayer_private.interfaces.IObjectWrapper;
 import org.chromium.weblayer_private.interfaces.IProfile;
 import org.chromium.weblayer_private.interfaces.IRemoteFragmentClient;
@@ -45,4 +46,9 @@ interface IWebLayer {
 
   // Returns whether or not the DevTools remote debugging server is enabled.
   boolean isRemoteDebuggingEnabled() = 6;
+
+  // Returns the singleton crash reporter controller. If WebLayer has not been
+  // initialized, does the minimum initialization needed for the crash reporter.
+  ICrashReporterController getCrashReporterController(
+      in IObjectWrapper appContext) = 7;
 }

weblayer/browser/java/org/chromium/weblayer_private/interfaces/WebLayerVersion.java

@@ -9,4 +9,4 @@ package org.chromium.weblayer_private.interfaces;
  *
  * Whenever any AIDL file is changed, sVersionNumber must be incremented.
  * */
-public final class WebLayerVersion { public static final int sVersionNumber = 19; }
+public final class WebLayerVersion { public static final int sVersionNumber = 20; }

weblayer/public/java/org/chromium/weblayer/CrashReporterController.java

@@ -6,9 +6,7 @@ package org.chromium.weblayer;
 
 import android.content.Context;
 import android.os.Bundle;
-import android.os.IBinder;
 import android.os.RemoteException;
-import android.util.AndroidRuntimeException;
 
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
@@ -16,6 +14,7 @@ import androidx.annotation.Nullable;
 import org.chromium.weblayer_private.interfaces.APICallException;
 import org.chromium.weblayer_private.interfaces.ICrashReporterController;
 import org.chromium.weblayer_private.interfaces.ICrashReporterControllerClient;
+import org.chromium.weblayer_private.interfaces.ObjectWrapper;
 import org.chromium.weblayer_private.interfaces.StrictModeWorkaround;
 
 /**
@@ -134,18 +133,13 @@ public final class CrashReporterController {
         if (mImpl != null) {
             return this;
         }
-        ClassLoader remoteClassLoader;
         try {
-            remoteClassLoader = WebLayer.getOrCreateRemoteClassLoader(appContext);
-            Class crashReporterControllerClass = remoteClassLoader.loadClass(
-                    "org.chromium.weblayer_private.CrashReporterControllerImpl");
-            mImpl = ICrashReporterController.Stub.asInterface(
-                    (IBinder) crashReporterControllerClass.getMethod("getInstance").invoke(null));
+            mImpl = WebLayer.getIWebLayer(appContext)
+                            .getCrashReporterController(ObjectWrapper.wrap(appContext));
             mImpl.setClient(new CrashReporterControllerClientImpl());
-        } catch (Exception e) {
-            throw new AndroidRuntimeException(e);
+        } catch (RemoteException e) {
+            throw new APICallException(e);
         }
-
         return this;
     }
 

weblayer/public/java/org/chromium/weblayer/WebLayer.java

@@ -158,6 +158,7 @@ public final class WebLayer {
         private final boolean mAvailable;
         private final int mMajorVersion;
         private final String mVersion;
+        private boolean mIsLoadingAsync;
 
         /**
          * Creates WebLayerLoader. This does a minimal amount of loading
@@ -207,9 +208,10 @@ public final class WebLayer {
                 return;
             }
             mCallbacks.add(callback);
-            if (mIWebLayer != null) {
+            if (mIsLoadingAsync) {
                 return; // Already loading.
             }
+            mIsLoadingAsync = true;
             if (getIWebLayer(appContext) == null) {
                 // Unable to create WebLayer. This generally shouldn't happen.
                 onWebLayerReady();
@@ -343,6 +345,10 @@ public final class WebLayer {
         }
     }
 
+    /* package */ static IWebLayer getIWebLayer(Context appContext) {
+        return getWebLayerLoader(appContext).getIWebLayer(appContext);
+    }
+
     @SuppressWarnings("NewApi")
     static ClassLoader getOrCreateRemoteClassLoaderForChildProcess(Context appContext)
             throws PackageManager.NameNotFoundException, ReflectiveOperationException {