Bug 1570690 - Synchronously extract system addons before Gecko startup in Fennec. r=VladBaicu,Grisha, a=RyanVM
authorNick Alexander <nalexander@mozilla.com>
Wed, 14 Aug 2019 18:10:55 +0000
changeset 541952 151bfae582afadf93c61e55e1fd4f980a716756f
parent 541951 c2aa1c31c07e4427bea5ec58432f8806dbe4d973
child 541953 3a3ff2f2324cea34a7f2db4ecf865e6ffd8ebbc5
push id11793
push userryanvm@gmail.com
push dateThu, 15 Aug 2019 16:39:25 +0000
treeherdermozilla-beta@3a3ff2f2324c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersVladBaicu, Grisha, RyanVM
bugs1570690, 1534451
milestone69.0
Bug 1570690 - Synchronously extract system addons before Gecko startup in Fennec. r=VladBaicu,Grisha, a=RyanVM Right now, there are a lot of things that race to complete before Gecko creates or first reads the profile. One of those things is extracting system addons (the `assets/features/` directory of the APK) to disk, ready for the Gecko profile code to enumerate them. Bug 1534451 added a non-trivial amount of background computation during `onCreate`. This tickled the existing race conditions such that system addon extraction frequently loses the race, making system addons unreliable. In addition, for reasons unknown, `PostUpdateHandler` did its work during `onStart`. But the Gecko profile was created/first read earlier, in `onCreate`. This widened the race window. This commit pulls the update handler into `onCreate`, which is at least early enough for it to have a chance of winning the race; and it makes the work synchronous, which is the simplest way to ensure that it is actually in place before Gecko startup (and profile creation/first read). Since system addons are our "get out of jail" card in many situations, the cost of extracting earlier seems like a good trade-off. That is, I'm sure the early disk access will appear in profiles, and it may even regress Raptor -- but it's a good trade-off. Differential Revision: https://phabricator.services.mozilla.com/D41687
mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
mobile/android/base/java/org/mozilla/gecko/updater/PostUpdateHandler.java
--- a/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
+++ b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ -324,17 +324,16 @@ public class BrowserApp extends GeckoApp
 
     private final TelemetryCorePingDelegate mTelemetryCorePingDelegate = new TelemetryCorePingDelegate();
     private final TelemetryActivationPingDelegate mTelemetryActivationPingDelegate = new TelemetryActivationPingDelegate();
 
     private final List<BrowserAppDelegate> delegates = Collections.unmodifiableList(Arrays.asList(
             new ScreenshotDelegate(),
             new BookmarkStateChangeDelegate(),
             new ReaderViewBookmarkPromotion(),
-            new PostUpdateHandler(),
             mTelemetryCorePingDelegate,
             mTelemetryActivationPingDelegate,
             new OfflineTabStatusDelegate(),
             new AdjustBrowserAppDelegate(mTelemetryCorePingDelegate)
     ));
 
     @NonNull
     private SearchEngineManager mSearchEngineManager; // Contains reference to Context - DO NOT LEAK!
@@ -645,16 +644,21 @@ public class BrowserApp extends GeckoApp
         }
 
         // This has to be prepared prior to calling GeckoApp.onCreate, because
         // widget code and BrowserToolbar need it, and they're created by the
         // layout, which GeckoApp takes care of.
         final GeckoApplication app = (GeckoApplication) getApplication();
         app.prepareLightweightTheme();
 
+        // Copying features out the APK races Gecko startup: the first time the profile is read by
+        // Gecko, it needs to find the copied features.  `super.onCreate(...)` initiates Gecko
+        // startup, so this must come first -- and be synchronous!
+        new PostUpdateHandler().onCreate(this, savedInstanceState);
+
         super.onCreate(savedInstanceState);
 
         if (isShutDownOrAbort()) {
             return;
         }
 
         mOnboardingHelper = new OnboardingHelper(this, safeStartingIntent);
         initSwitchboardAndMma(this, safeStartingIntent, isInAutomation);
--- a/mobile/android/base/java/org/mozilla/gecko/updater/PostUpdateHandler.java
+++ b/mobile/android/base/java/org/mozilla/gecko/updater/PostUpdateHandler.java
@@ -1,63 +1,66 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.updater;
 
+import android.content.SharedPreferences;
 import android.content.res.AssetManager;
-import android.content.SharedPreferences;
+import android.os.Bundle;
 import android.util.Log;
 
 import org.mozilla.gecko.AppConstants;
 import org.mozilla.gecko.BrowserApp;
-import org.mozilla.gecko.BuildConfig;
-import org.mozilla.gecko.delegates.BrowserAppDelegateWithReference;
 import org.mozilla.gecko.GeckoSharedPrefs;
 import org.mozilla.gecko.preferences.GeckoPreferences;
 import org.mozilla.gecko.util.IOUtils;
-import org.mozilla.gecko.util.ThreadUtils;
+import org.mozilla.gecko.util.StrictModeContext;
 
 import java.io.File;
 import java.io.FileOutputStream;
+import java.io.IOException;
 import java.io.InputStream;
-import java.io.IOException;
 import java.io.OutputStream;
 
 /**
- * Perform tasks in the background after the app has been installed/updated.
+ * Perform <b>synchronous</b> tasks after the app has been installed/updated.
+ *
+ * This is <b>only</b> intended for things that race profile creation and/or first profile read.
+ * Use (or introduce) an asynchronous vehicle for things that don't race one of those two
+ * operations!
  */
-public class PostUpdateHandler extends BrowserAppDelegateWithReference {
+public class PostUpdateHandler {
     private static final String LOGTAG = "GeckoPostUpdateHandler";
     private static final boolean DEBUG = false;
 
-    @Override
-    public void onStart(final BrowserApp browserApp) {
-        ThreadUtils.postToBackgroundThread(new Runnable() {
-            @Override
-            public void run() {
-                final SharedPreferences prefs = GeckoSharedPrefs.forApp(browserApp);
+    @SuppressWarnings("try")
+    public void onCreate(final BrowserApp browserApp, final Bundle savedInstanceState) {
+        // Copying features out the APK races Gecko startup: the first time the profile is read by
+        // Gecko, it needs to find the copied features.  Rather than do non-trivial synchronization
+        // to avoid the race, just do the work synchronously.
+        try (StrictModeContext unused = StrictModeContext.allowDiskWrites()) {
+            final SharedPreferences prefs = GeckoSharedPrefs.forApp(browserApp);
 
-                // Check if this is a new installation or if the app has been updated since the last start.
-                if (!AppConstants.MOZ_APP_BUILDID.equals(prefs.getString(GeckoPreferences.PREFS_APP_UPDATE_LAST_BUILD_ID, null))) {
-                    if (DEBUG) {
-                        Log.d(LOGTAG, "Build ID changed since last start: '" +
-                                AppConstants.MOZ_APP_BUILDID +
-                                "', '" +
-                                prefs.getString(GeckoPreferences.PREFS_APP_UPDATE_LAST_BUILD_ID, null)
-                                + "'");
-                    }
+            // Check if this is a new installation or if the app has been updated since the last start.
+            if (!AppConstants.MOZ_APP_BUILDID.equals(prefs.getString(GeckoPreferences.PREFS_APP_UPDATE_LAST_BUILD_ID, null))) {
+                if (DEBUG) {
+                    Log.d(LOGTAG, "Build ID changed since last start: '" +
+                            AppConstants.MOZ_APP_BUILDID +
+                            "', '" +
+                            prefs.getString(GeckoPreferences.PREFS_APP_UPDATE_LAST_BUILD_ID, null)
+                            + "'");
+                }
 
-                    // Copy the bundled system add-ons from the APK to the data directory.
-                    copyFeaturesFromAPK(browserApp);
-                }
+                // Copy the bundled system add-ons from the APK to the data directory.
+                copyFeaturesFromAPK(browserApp);
             }
-        });
+        }
     }
 
     /**
      * Copies the /assets/features folder out of the APK and into the app's data directory.
      */
     private void copyFeaturesFromAPK(BrowserApp browserApp) {
         if (DEBUG) {
             Log.d(LOGTAG, "Copying system add-ons from APK to dataDir");