Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian a=ritu
authorMichael Comella <michael.l.comella@gmail.com>
Thu, 24 Mar 2016 11:18:58 -0700
changeset 323983 98b3c3a9c2732dd58a6e74c6d19deb53060dd5bf
parent 323982 a5801fb8ba648ee9a98a75479c7743d408cc43b9
child 323984 bceb57212824761be930e2a753df202ce14c5c5d
push id5913
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 16:57:49 +0000
treeherdermozilla-beta@dcaf0a6fa115 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssebastian, ritu
bugs1249288
milestone47.0a2
Bug 1249288 - review: Correct concurrency issues with searchEngineManager. r=sebastian a=ritu Additionally, added WeakReferences to the SEM in its callbacks so we can GC ASAP if the Activity (and thus the SEM) gets GC'd. This is important since we hold a reference to Context which can be a rather large object. Furthermore, I added some related thread annotations where I felt they were useful. MozReview-Commit-ID: KaWlw14uOoN
mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java
mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java
mobile/android/search/java/org/mozilla/search/SearchActivity.java
--- a/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
+++ b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ -4,16 +4,18 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko;
 
 import android.Manifest;
 import android.app.DownloadManager;
 import android.os.Environment;
 import android.support.annotation.NonNull;
+import android.support.annotation.Nullable;
+import android.support.annotation.WorkerThread;
 import org.json.JSONArray;
 import org.mozilla.gecko.adjust.AdjustHelperInterface;
 import org.mozilla.gecko.annotation.RobocopTarget;
 import org.mozilla.gecko.AppConstants.Versions;
 import org.mozilla.gecko.DynamicToolbar.PinReason;
 import org.mozilla.gecko.DynamicToolbar.VisibilityTransition;
 import org.mozilla.gecko.GeckoProfileDirectories.NoMozillaDirectoryException;
 import org.mozilla.gecko.Tabs.TabEvents;
@@ -282,17 +284,18 @@ public class BrowserApp extends GeckoApp
     // (starting the animation), the HomePager is hidden, and the HomePager animation completes,
     // both the web content and the HomePager will be hidden. This flag is used to prevent the
     // race by determining if the web content should be hidden at the animation's end.
     private boolean mHideWebContentOnAnimationEnd;
 
     private final DynamicToolbar mDynamicToolbar = new DynamicToolbar();
     private final ScreenshotObserver mScreenshotObserver = new ScreenshotObserver();
 
-    private SearchEngineManager searchEngineManager;
+    @NonNull
+    private SearchEngineManager searchEngineManager; // Contains reference to Context - DO NOT LEAK!
 
     @Override
     public View onCreateView(final String name, final Context context, final AttributeSet attrs) {
         final View view;
         if (BrowserToolbar.class.getName().equals(name)) {
             view = BrowserToolbar.create(context, attrs);
         } else if (TabsPanel.TabsLayout.class.getName().equals(name)) {
             view = TabsPanel.createTabsLayout(context, attrs);
@@ -1051,27 +1054,27 @@ public class BrowserApp extends GeckoApp
                 if (profile.inGuestMode()) {
                     GuestSession.showNotification(BrowserApp.this);
                 } else {
                     // If we're restarting, we won't destroy the activity.
                     // Make sure we remove any guest notifications that might
                     // have been shown.
                     GuestSession.hideNotification(BrowserApp.this);
                 }
-
-                // We don't upload in onCreate because that's only called when the Activity needs to be instantiated
-                // and it's possible the system will never free the Activity from memory.
-                //
-                // We don't upload in onResume/onPause because that will be called each time the Activity is obscured,
-                // including by our own Activities/dialogs, and there is no reason to upload each time we're unobscured.
-                //
-                // So we're left with onStart/onStop.
-                uploadTelemetry(profile);
             }
         });
+
+        // We don't upload in onCreate because that's only called when the Activity needs to be instantiated
+        // and it's possible the system will never free the Activity from memory.
+        //
+        // We don't upload in onResume/onPause because that will be called each time the Activity is obscured,
+        // including by our own Activities/dialogs, and there is no reason to upload each time we're unobscured.
+        //
+        // So we're left with onStart/onStop.
+        searchEngineManager.getEngine(new UploadTelemetryCallback(BrowserApp.this));
     }
 
     @Override
     public void onStop() {
         super.onStop();
 
         // We only show the guest mode notification when our activity is in the foreground.
         GuestSession.hideNotification(this);
@@ -1423,18 +1426,17 @@ public class BrowserApp extends GeckoApp
             mAccountsHelper.uninit();
             mAccountsHelper = null;
         }
 
         if (mZoomedView != null) {
             mZoomedView.destroy();
         }
 
-        searchEngineManager.destroy();
-        searchEngineManager = null;
+        searchEngineManager.unregisterListeners();
 
         EventDispatcher.getInstance().unregisterGeckoThreadListener((GeckoEventListener) this,
             "Gecko:DelayedStartup",
             "Menu:Open",
             "Menu:Update",
             "LightweightTheme:Update",
             "Search:Keyword",
             "Prompt:ShowTop");
@@ -4042,58 +4044,69 @@ public class BrowserApp extends GeckoApp
 
         mActionBarFlipper.showPrevious();
 
         // Only slide the urlbar out if it was hidden when the action mode started
         // Don't animate hiding it so that there's no flash as we switch back to url mode
         mDynamicToolbar.setTemporarilyVisible(false, VisibilityTransition.IMMEDIATE);
     }
 
-    private void uploadTelemetry(final GeckoProfile profile) {
-        if (!TelemetryUploadService.isUploadEnabledByProfileConfig(this, profile)) {
+    @WorkerThread // synchronous SharedPrefs write.
+    private static void uploadTelemetry(final Context context, final GeckoProfile profile,
+            final org.mozilla.gecko.search.SearchEngine defaultEngine) {
+        if (!TelemetryUploadService.isUploadEnabledByProfileConfig(context, profile)) {
             return;
         }
 
-        final SharedPreferences sharedPrefs = GeckoSharedPrefs.forProfileName(this, profile.getName());
+        final SharedPreferences sharedPrefs = GeckoSharedPrefs.forProfileName(context, profile.getName());
         final int seq = sharedPrefs.getInt(TelemetryConstants.PREF_SEQ_COUNT, 1);
 
         // We store synchronously before sending the Intent to ensure this sequence number will not be re-used.
         sharedPrefs.edit().putInt(TelemetryConstants.PREF_SEQ_COUNT, seq + 1).commit();
 
-        searchEngineManager.getEngine(new UploadTelemetryCallback(getContext(), seq, profile.getName(), profile.getDir()));
+        final Intent i = new Intent(TelemetryConstants.ACTION_UPLOAD_CORE);
+        i.setClass(context, TelemetryUploadService.class);
+        i.putExtra(TelemetryConstants.EXTRA_DEFAULT_SEARCH_ENGINE, defaultEngine.getIdentifier());
+        i.putExtra(TelemetryConstants.EXTRA_DOC_ID, UUID.randomUUID().toString());
+        i.putExtra(TelemetryConstants.EXTRA_PROFILE_NAME, profile.getName());
+        i.putExtra(TelemetryConstants.EXTRA_PROFILE_PATH, profile.getDir().getAbsolutePath());
+        i.putExtra(TelemetryConstants.EXTRA_SEQ, seq);
+        context.startService(i);
     }
 
     private static class UploadTelemetryCallback implements SearchEngineManager.SearchEngineCallback {
-        private final WeakReference<Context> contextWeakReference;
-        private final int seq;
-        private final String profileName;
-        private final String profileDirPath;
-
-        public UploadTelemetryCallback(final Context context, final int seq, final String profileName, final File profileDir) {
-            this.contextWeakReference = new WeakReference<>(context);
-            this.seq = seq;
-            this.profileName = profileName;
-            this.profileDirPath = profileDir.toString();
+        private final WeakReference<BrowserApp> activityWeakReference;
+
+        public UploadTelemetryCallback(final BrowserApp activity) {
+            this.activityWeakReference = new WeakReference<>(activity);
         }
 
+        // May be called from any thread.
         @Override
         public void execute(final org.mozilla.gecko.search.SearchEngine engine) {
-            final Context context = this.contextWeakReference.get();
-            if (context == null) {
+            // Don't waste resources queueing to the background thread if we don't have a reference.
+            if (this.activityWeakReference.get() == null) {
                 return;
             }
 
-            final Intent i = new Intent(TelemetryConstants.ACTION_UPLOAD_CORE);
-            i.setClass(context, TelemetryUploadService.class);
-            i.putExtra(TelemetryConstants.EXTRA_DEFAULT_SEARCH_ENGINE, engine.getIdentifier());
-            i.putExtra(TelemetryConstants.EXTRA_DOC_ID, UUID.randomUUID().toString());
-            i.putExtra(TelemetryConstants.EXTRA_PROFILE_NAME, this.profileName);
-            i.putExtra(TelemetryConstants.EXTRA_PROFILE_PATH, this.profileDirPath);
-            i.putExtra(TelemetryConstants.EXTRA_SEQ, this.seq);
-            context.startService(i);
+            // The containing method can be called from onStart: queue this work so that
+            // the first launch of the activity doesn't trigger profile init too early.
+            //
+            // Additionally, uploadTelemetry must be called from a worker thread.
+            ThreadUtils.postToBackgroundThread(new Runnable() {
+                @WorkerThread
+                @Override
+                public void run() {
+                    final BrowserApp activity = activityWeakReference.get();
+                    if (activity == null) {
+                        return;
+                    }
+                    uploadTelemetry(activity, activity.getProfile(), engine);
+                }
+            });
         }
     }
 
     public static interface TabStripInterface {
         public void refresh();
         void setOnTabChangedListener(OnTabAddedOrRemovedListener listener);
         interface OnTabAddedOrRemovedListener {
             void onTabChanged();
--- a/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java
+++ b/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java
@@ -46,16 +46,17 @@ import org.mozilla.gecko.annotation.Wrap
 import org.mozilla.gecko.util.FileUtils;
 import org.mozilla.gecko.util.HardwareUtils;
 import org.mozilla.gecko.util.ThreadUtils;
 
 import android.app.Activity;
 import android.content.Context;
 import android.content.SharedPreferences;
 import android.os.SystemClock;
+import android.support.annotation.WorkerThread;
 import android.telephony.TelephonyManager;
 import android.util.Log;
 
 /**
  * Handles distribution file loading and fetching,
  * and the corresponding hand-offs to Gecko.
  */
 @RobocopTarget
@@ -121,16 +122,17 @@ public class Distribution {
      * If <code>distributionFound</code> is called, it will be the only call.
      * If <code>distributionNotFound</code> is called, it might be followed by
      * a call to <code>distributionArrivedLate</code>.
      *
      * When <code>distributionNotFound</code> is called,
      * {@link org.mozilla.gecko.distribution.Distribution#exists()} will return
      * false. In the other two callbacks, it will return true.
      */
+    @WorkerThread
     public interface ReadyCallback {
         void distributionNotFound();
         void distributionFound(Distribution distribution);
         void distributionArrivedLate(Distribution distribution);
     }
 
     /**
      * Used as a drop-off point for ReferrerReceiver. Checked when we process
@@ -974,16 +976,17 @@ public class Distribution {
                 }
                 callback.distributionArrivedLate(Distribution.this);
             }
         });
     }
 
     private void invokeCallbackDelayed(final ReadyCallback callback) {
         ThreadUtils.postToBackgroundThread(new Runnable() {
+            @WorkerThread
             @Override
             public void run() {
                 switch (state) {
                     case STATE_SET:
                         callback.distributionFound(Distribution.this);
                         break;
                     case STATE_NONE:
                         callback.distributionNotFound();
--- a/mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java
+++ b/mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java
@@ -2,16 +2,18 @@
  * 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.search;
 
 import android.content.Context;
 import android.content.SharedPreferences;
 import android.support.annotation.Nullable;
+import android.support.annotation.UiThread;
+import android.support.annotation.WorkerThread;
 import android.text.TextUtils;
 import android.util.Log;
 import org.json.JSONException;
 import org.json.JSONObject;
 import org.mozilla.gecko.AppConstants;
 import org.mozilla.gecko.GeckoProfile;
 import org.mozilla.gecko.GeckoSharedPrefs;
 import org.mozilla.gecko.Locales;
@@ -28,22 +30,28 @@ import java.io.BufferedInputStream;
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.OutputStream;
 import java.io.UnsupportedEncodingException;
+import java.lang.ref.WeakReference;
 import java.net.HttpURLConnection;
 import java.net.URL;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Locale;
 
+/**
+ * This class is not thread-safe, except where otherwise noted.
+ *
+ * This class contains a reference to {@link Context} - DO NOT LEAK!
+ */
 public class SearchEngineManager implements SharedPreferences.OnSharedPreferenceChangeListener {
     private static final String LOG_TAG = "GeckoSearchEngineManager";
 
     // Gecko pref that defines the name of the default search engine.
     private static final String PREF_GECKO_DEFAULT_ENGINE = "browser.search.defaultenginename";
 
     // Gecko pref that defines the name of the default searchplugin locale.
     private static final String PREF_GECKO_DEFAULT_LOCALE = "distribution.searchplugins.defaultLocale";
@@ -57,20 +65,20 @@ public class SearchEngineManager impleme
     // URL for the geo-ip location service. Keep in sync with "browser.search.geoip.url" perference in Gecko.
     private static final String GEOIP_LOCATION_URL = "https://location.services.mozilla.com/v1/country?key=" + AppConstants.MOZ_MOZILLA_API_KEY;
 
     // This should go through GeckoInterface to get the UA, but the search activity
     // doesn't use a GeckoView yet. Until it does, get the UA directly.
     private static final String USER_AGENT = HardwareUtils.isTablet() ?
         AppConstants.USER_AGENT_FENNEC_TABLET : AppConstants.USER_AGENT_FENNEC_MOBILE;
 
-    private Context context;
-    private Distribution distribution;
-    @Nullable private SearchEngineCallback changeCallback;
-    private SearchEngine engine;
+    private final Context context;
+    private final Distribution distribution;
+    @Nullable private volatile SearchEngineCallback changeCallback;
+    @Nullable private volatile SearchEngine engine;
 
     // Cached version of default locale included in Gecko chrome manifest.
     // This should only be accessed from the background thread.
     private String fallbackLocale;
 
     // Cached version of default locale included in Distribution preferences.
     // This should only be accessed from the background thread.
     private String distributionLocale;
@@ -81,51 +89,51 @@ public class SearchEngineManager impleme
 
     public SearchEngineManager(Context context, Distribution distribution) {
         this.context = context;
         this.distribution = distribution;
         GeckoSharedPrefs.forApp(context).registerOnSharedPreferenceChangeListener(this);
     }
 
     /**
-     * Sets a callback to be called when the default engine changes.
+     * Sets a callback to be called when the default engine changes. This can be called from any thread.
      *
      * @param changeCallback SearchEngineCallback to be called after the search engine
      *                       changed. This will run on the UI thread.
      *                       Note: callback may be called with null engine.
      */
     public void setChangeCallback(SearchEngineCallback changeCallback) {
         this.changeCallback = changeCallback;
     }
 
     /**
-     * Perform an action with the user's default search engine.
+     * Perform an action with the user's default search engine. This can be called from any thread.
      *
      * @param callback The callback to be used with the user's default search engine. The call
      *                 may be sync or async; if the call is async, it will be called on the
      *                 ui thread.
      */
     public void getEngine(SearchEngineCallback callback) {
         if (engine != null) {
             callback.execute(engine);
         } else {
             getDefaultEngine(callback);
         }
     }
 
-    public void destroy() {
+    /**
+     * Should be called when the object goes out of scope.
+     */
+    public void unregisterListeners() {
         GeckoSharedPrefs.forApp(context).unregisterOnSharedPreferenceChangeListener(this);
-        context = null;
-        distribution = null;
-        changeCallback = null;
-        engine = null;
     }
 
-    private int ignorePreferenceChange = 0;
+    private volatile int ignorePreferenceChange = 0;
 
+    @UiThread // according to the docs.
     @Override
     public void onSharedPreferenceChanged(final SharedPreferences sharedPreferences, final String key) {
         if (!TextUtils.equals(PREF_DEFAULT_ENGINE_KEY, key)) {
             return;
         }
 
         if (ignorePreferenceChange > 0) {
             ignorePreferenceChange--;
@@ -134,107 +142,151 @@ public class SearchEngineManager impleme
 
         getDefaultEngine(changeCallback);
     }
 
     /**
      * Runs a SearchEngineCallback on the main thread.
      */
     private void runCallback(final SearchEngine engine, @Nullable final SearchEngineCallback callback) {
-        ThreadUtils.postToUiThread(new Runnable() {
-            @Override
-            public void run() {
-                // Cache engine for future calls to getEngine.
-                SearchEngineManager.this.engine = engine;
-                if (callback != null) {
-                    callback.execute(engine);
-                }
+        ThreadUtils.postToUiThread(new RunCallbackUiThreadRunnable(this, engine, callback));
+    }
+
+    // Static is not strictly necessary but the outer class has a reference to Context so we should GC ASAP.
+    private static class RunCallbackUiThreadRunnable implements Runnable {
+        private final WeakReference<SearchEngineManager> searchEngineManagerWeakReference;
+        private final SearchEngine searchEngine;
+        private final SearchEngineCallback callback;
+
+        public RunCallbackUiThreadRunnable(final SearchEngineManager searchEngineManager, final SearchEngine searchEngine,
+                final SearchEngineCallback callback) {
+            this.searchEngineManagerWeakReference = new WeakReference<>(searchEngineManager);
+            this.searchEngine = searchEngine;
+            this.callback = callback;
+        }
+
+        @UiThread
+        @Override
+        public void run() {
+            final SearchEngineManager searchEngineManager = searchEngineManagerWeakReference.get();
+            if (searchEngineManager == null) {
+                return;
             }
-        });
+
+            // Cache engine for future calls to getEngine.
+            searchEngineManager.engine = searchEngine;
+            if (callback != null) {
+                callback.execute(searchEngine);
+            }
+
+        }
     }
 
     /**
      * This method finds and creates the default search engine. It will first look for
      * the default engine name, then create the engine from that name.
      *
      * To find the default engine name, we first look in shared preferences, then
      * the distribution (if one exists), and finally fall back to the localized default.
      *
      * @param callback SearchEngineCallback to be called after successfully looking
      *                 up the search engine. This will run on the UI thread.
      *                 Note: callback may be called with null engine.
      */
     private void getDefaultEngine(final SearchEngineCallback callback) {
         // This runnable is posted to the background thread.
-        distribution.addOnDistributionReadyCallback(new Distribution.ReadyCallback() {
-            @Override
-            public void distributionNotFound() {
-                defaultBehavior();
+        distribution.addOnDistributionReadyCallback(new GetDefaultEngineDistributionCallbacks(this, callback));
+    }
+
+    // Static is not strictly necessary but the outer class contains a reference to Context so we should GC ASAP.
+    private static class GetDefaultEngineDistributionCallbacks implements Distribution.ReadyCallback {
+        private final WeakReference<SearchEngineManager> searchEngineManagerWeakReference;
+        private final SearchEngineCallback callback;
+
+        public GetDefaultEngineDistributionCallbacks(final SearchEngineManager searchEngineManager,
+                final SearchEngineCallback callback) {
+            this.searchEngineManagerWeakReference = new WeakReference<>(searchEngineManager);
+            this.callback = callback;
+        }
+
+        @Override
+        public void distributionNotFound() {
+            defaultBehavior();
+        }
+
+        @Override
+        public void distributionFound(Distribution distribution) {
+            defaultBehavior();
+        }
+
+        @Override
+        public void distributionArrivedLate(Distribution distribution) {
+            final SearchEngineManager searchEngineManager = searchEngineManagerWeakReference.get();
+            if (searchEngineManager == null) {
+                return;
             }
 
-            @Override
-            public void distributionFound(Distribution distribution) {
-                defaultBehavior();
+            // Let's see if there's a name in the distro.
+            // If so, just this once we'll override the saved value.
+            final String name = searchEngineManager.getDefaultEngineNameFromDistribution();
+
+            if (name == null) {
+                return;
             }
 
-            @Override
-            public void distributionArrivedLate(Distribution distribution) {
-                // Let's see if there's a name in the distro.
-                // If so, just this once we'll override the saved value.
-                final String name = getDefaultEngineNameFromDistribution();
+            // Store the default engine name for the future.
+            // Increment an 'ignore' counter so that this preference change
+            // won't cause getDefaultEngine to be called again.
+            searchEngineManager.ignorePreferenceChange++;
+            GeckoSharedPrefs.forApp(searchEngineManager.context)
+                    .edit()
+                    .putString(PREF_DEFAULT_ENGINE_KEY, name)
+                    .apply();
+
+            final SearchEngine engine = searchEngineManager.createEngineFromName(name);
+            searchEngineManager.runCallback(engine, callback);
+        }
 
+        @WorkerThread // calling methods are @WorkerThread
+        private void defaultBehavior() {
+            final SearchEngineManager searchEngineManager = searchEngineManagerWeakReference.get();
+            if (searchEngineManager == null) {
+                return;
+            }
+
+            // First look for a default name stored in shared preferences.
+            String name = GeckoSharedPrefs.forApp(searchEngineManager.context).getString(PREF_DEFAULT_ENGINE_KEY, null);
+
+            // Check for a region stored in shared preferences. If we don't have a region,
+            // we should force a recheck of the default engine.
+            String region = GeckoSharedPrefs.forApp(searchEngineManager.context).getString(PREF_REGION_KEY, null);
+
+            if (name != null && region != null) {
+                Log.d(LOG_TAG, "Found default engine name in SharedPreferences: " + name);
+            } else {
+                // First, look for the default search engine in a distribution.
+                name = searchEngineManager.getDefaultEngineNameFromDistribution();
                 if (name == null) {
-                    return;
+                    // Otherwise, get the default engine that we ship.
+                    name = searchEngineManager.getDefaultEngineNameFromLocale();
                 }
 
                 // Store the default engine name for the future.
                 // Increment an 'ignore' counter so that this preference change
                 // won't cause getDefaultEngine to be called again.
-                ignorePreferenceChange++;
-                GeckoSharedPrefs.forApp(context)
+                searchEngineManager.ignorePreferenceChange++;
+                GeckoSharedPrefs.forApp(searchEngineManager.context)
                         .edit()
                         .putString(PREF_DEFAULT_ENGINE_KEY, name)
                         .apply();
-
-                final SearchEngine engine = createEngineFromName(name);
-                runCallback(engine, callback);
             }
 
-            private void defaultBehavior() {
-                // First look for a default name stored in shared preferences.
-                String name = GeckoSharedPrefs.forApp(context).getString(PREF_DEFAULT_ENGINE_KEY, null);
-
-                // Check for a region stored in shared preferences. If we don't have a region,
-                // we should force a recheck of the default engine.
-                String region = GeckoSharedPrefs.forApp(context).getString(PREF_REGION_KEY, null);
-
-                if (name != null && region != null) {
-                    Log.d(LOG_TAG, "Found default engine name in SharedPreferences: " + name);
-                } else {
-                    // First, look for the default search engine in a distribution.
-                    name = getDefaultEngineNameFromDistribution();
-                    if (name == null) {
-                        // Otherwise, get the default engine that we ship.
-                        name = getDefaultEngineNameFromLocale();
-                    }
-
-                    // Store the default engine name for the future.
-                    // Increment an 'ignore' counter so that this preference change
-                    // won't cause getDefaultEngine to be called again.
-                    ignorePreferenceChange++;
-                    GeckoSharedPrefs.forApp(context)
-                                    .edit()
-                                    .putString(PREF_DEFAULT_ENGINE_KEY, name)
-                                    .apply();
-                }
-
-                final SearchEngine engine = createEngineFromName(name);
-                runCallback(engine, callback);
-            }
-        });
+            final SearchEngine engine = searchEngineManager.createEngineFromName(name);
+            searchEngineManager.runCallback(engine, callback);
+        }
     }
 
     /**
      * Looks for a default search engine included in a distribution.
      * This method must be called after the distribution is ready.
      *
      * @return search engine name.
      */
--- a/mobile/android/search/java/org/mozilla/search/SearchActivity.java
+++ b/mobile/android/search/java/org/mozilla/search/SearchActivity.java
@@ -1,14 +1,15 @@
 /* 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.search;
 
+import android.support.annotation.NonNull;
 import org.mozilla.gecko.GeckoAppShell;
 import org.mozilla.gecko.Locales;
 import org.mozilla.gecko.R;
 import org.mozilla.gecko.Telemetry;
 import org.mozilla.gecko.TelemetryContract;
 import org.mozilla.gecko.db.BrowserContract.SearchHistory;
 import org.mozilla.gecko.distribution.Distribution;
 import org.mozilla.gecko.search.SearchEngine;
@@ -57,17 +58,18 @@ public class SearchActivity extends Loca
         WAITING,
         EDITING
     }
 
     // Default states when activity is created.
     private SearchState searchState = SearchState.PRESEARCH;
     private EditState editState = EditState.WAITING;
 
-    private SearchEngineManager searchEngineManager;
+    @NonNull
+    private SearchEngineManager searchEngineManager; // Contains reference to Context - DO NOT LEAK!
 
     // Only accessed on the main thread.
     private SearchEngine engine;
 
     private SuggestionsFragment suggestionsFragment;
     private PostSearchFragment postSearchFragment;
 
     private AsyncQueryHandler queryHandler;
@@ -188,18 +190,17 @@ public class SearchActivity extends Loca
             // and we should enter editing mode to bring up the keyboard.
             setEditState(EditState.EDITING);
         }
     }
 
     @Override
     protected void onDestroy() {
         super.onDestroy();
-        searchEngineManager.destroy();
-        searchEngineManager = null;
+        searchEngineManager.unregisterListeners();
         engine = null;
         suggestionsFragment = null;
         postSearchFragment = null;
         queryHandler = null;
         searchBar = null;
         preSearch = null;
         postSearch = null;
         settingsButton = null;