Bug 1570690 - Synchronously extract system addons before Gecko startup in Fennec. r=VladBaicu,Grisha
authorNick Alexander <nalexander@mozilla.com>
Wed, 14 Aug 2019 18:10:55 +0000
changeset 488091 d374e582afbc0c9b8b7a1916a08440dff94a9049
parent 488090 91ae5f60b8c081767422e8bf9bb88d666ff204b3
child 488092 725ed668541437310ddcc3fb5aa18e23cfc922b2
push id113900
push usercbrindusan@mozilla.com
push dateThu, 15 Aug 2019 09:53:50 +0000
treeherdermozilla-inbound@0db07ff50ab5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersVladBaicu, Grisha
bugs1570690, 1534451
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1570690 - Synchronously extract system addons before Gecko startup in Fennec. r=VladBaicu,Grisha 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");