Bug 1106584 - Part 2: For safety, make explicit prefs with context getter. r=vng, a=lsblakk
authorGarvan Keeley <gkeeley@mozilla.com>
Thu, 11 Dec 2014 09:06:00 -0800
changeset 242391 a4222a91e3bb1a32df9d5bfc4a8c429f91e1923c
parent 242390 586519c20ccd407a7b7ec612a1b0a9aadd8a410a
child 242392 36a276929062532808d2de941a892baab7a2f29d
push id4311
push userraliiev@mozilla.com
push dateMon, 12 Jan 2015 19:37:41 +0000
treeherdermozilla-beta@150c9fed433b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvng, lsblakk
bugs1106584
milestone36.0a2
Bug 1106584 - Part 2: For safety, make explicit prefs with context getter. r=vng, a=lsblakk Any class that has access to a context should call Prefs.getInstance(Context) which is guaranteed to return a Prefs. With no context, classes can call Prefs.getInstanceWithoutContext() but they must null-check the return, and handle accordingly. Fortunately, the latter case happens in very few places, all of which require no special handling of this case. This change: - maintains context-less access to the Prefs - classes internal to the service can safely call getInstanceWithoutContext(), as the service (or the MainApp in the stumbler case) will have instantiated a Prefs so that getInstanceWithoutContext() will return a prefs - protects against null Prefs if we have failed to consider a particular entry point to the code will require that Prefs was instatiated with a context.
mobile/android/stumbler/java/org/mozilla/mozstumbler/service/Prefs.java
mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/StumblerService.java
mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/GPSScanner.java
mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/LocationBlockList.java
mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/WifiScanner.java
mobile/android/stumbler/java/org/mozilla/mozstumbler/service/uploadthread/AsyncUploader.java
mobile/android/stumbler/java/org/mozilla/mozstumbler/service/uploadthread/UploadAlarmReceiver.java
mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/AbstractCommunicator.java
--- a/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/Prefs.java
+++ b/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/Prefs.java
@@ -43,28 +43,25 @@ public  final class Prefs {
                     .remove("power_saving_mode")
                     .commit();
 
             getPrefs().edit().putInt(VALUES_VERSION_PREF, AppGlobals.appVersionCode).commit();
             getPrefs().edit().commit();
         }
     }
 
-    /* Prefs must be created on application startup or service startup.
-     * TODO: turn into regular singleton if Context dependency can be removed. */
-    public static void createGlobalInstance(Context c) {
-        if (sInstance != null) {
-            return;
+    public static Prefs getInstance(Context c) {
+        if (sInstance == null) {
+            sInstance = new Prefs(c);
         }
-        sInstance = new Prefs(c);
+        return sInstance;
     }
 
-    /* Only access after CreatePrefsInstance(Context) has been called at startup. */
-    public static Prefs getInstance() {
-        assert(sInstance != null);
+    // Allows code without a context handle to grab the prefs. The caller must null check the return value.
+    public static Prefs getInstanceWithoutContext() {
         return sInstance;
     }
 
     ///
     /// Setters
     ///
     public synchronized void setUserAgent(String userAgent) {
         setStringPref(USER_AGENT_PREF, userAgent);
--- a/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/StumblerService.java
+++ b/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/StumblerService.java
@@ -1,27 +1,27 @@
 /* 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.mozstumbler.service.stumblerthread;
 
+import android.content.Context;
 import android.content.Intent;
 import android.location.Location;
 import android.os.AsyncTask;
 import android.util.Log;
 import java.io.IOException;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 import org.mozilla.mozstumbler.service.AppGlobals;
 import org.mozilla.mozstumbler.service.Prefs;
 import org.mozilla.mozstumbler.service.stumblerthread.blocklist.WifiBlockListInterface;
 import org.mozilla.mozstumbler.service.stumblerthread.datahandling.DataStorageManager;
 import org.mozilla.mozstumbler.service.stumblerthread.scanners.ScanManager;
-import org.mozilla.mozstumbler.service.stumblerthread.scanners.cellscanner.CellScanner;
 import org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver;
 import org.mozilla.mozstumbler.service.utils.NetworkUtils;
 import org.mozilla.mozstumbler.service.utils.PersistentIntentService;
 
 // In stand-alone service mode (a.k.a passive scanning mode), this is created from PassiveServiceReceiver (by calling startService).
 // The StumblerService is a sticky unbound service in this usage.
 //
 public class StumblerService extends PersistentIntentService
@@ -63,18 +63,18 @@ public class StumblerService extends Per
     }
 
     // This is optional, not used in Fennec, and is for clients to specify a (potentially long) list
     // of blocklisted SSIDs/BSSIDs
     public void setWifiBlockList(WifiBlockListInterface list) {
         mScanManager.setWifiBlockList(list);
     }
 
-    public Prefs getPrefs() {
-        return Prefs.getInstance();
+    public Prefs getPrefs(Context c) {
+        return Prefs.getInstance(c);
     }
 
     public void checkPrefs() {
         mScanManager.checkPrefs();
     }
 
     public int getLocationCount() {
         return mScanManager.getLocationCount();
@@ -111,17 +111,18 @@ public class StumblerService extends Per
     public boolean isGeofenced () {
         return mScanManager.isGeofenced();
     }
 
     // Previously this was done in onCreate(). Moved out of that so that in the passive standalone service
     // use (i.e. Fennec), init() can be called from this class's dedicated thread.
     // Safe to call more than once, ensure added code complies with that intent.
     protected void init() {
-        Prefs.createGlobalInstance(this);
+        // Ensure Prefs is created, so internal utility code can use getInstanceWithoutContext
+        Prefs.getInstance(this);
         NetworkUtils.createGlobalInstance(this);
         DataStorageManager.createGlobalInstance(this, this);
 
         mReporter.startup(this);
     }
 
     // Called from the main thread.
     @Override
@@ -145,17 +146,17 @@ public class StumblerService extends Per
         new AsyncTask<Void, Void, Void>() {
             @Override
             protected Void doInBackground(Void... params) {
                 if (AppGlobals.isDebug) {
                     Log.d(LOG_TAG, "onDestroy");
                 }
 
                 if (!sFirefoxStumblingEnabled.get()) {
-                    Prefs.getInstance().setFirefoxScanEnabled(false);
+                    Prefs.getInstance(StumblerService.this).setFirefoxScanEnabled(false);
                 }
 
                 if (DataStorageManager.getInstance() != null) {
                     try {
                         DataStorageManager.getInstance().saveCurrentReportsToDisk();
                     } catch (IOException ex) {
                         AppGlobals.guiLogInfo(ex.toString());
                         Log.e(LOG_TAG, "Exception in onDestroy saving reports" + ex.toString());
@@ -177,58 +178,58 @@ public class StumblerService extends Per
 
         // Post-init(), set the mode to passive.
         mScanManager.setPassiveMode(true);
 
         if (intent == null) {
             return;
         }
 
-        final boolean isScanEnabledInPrefs = Prefs.getInstance().getFirefoxScanEnabled();
+        final boolean isScanEnabledInPrefs = Prefs.getInstance(this).getFirefoxScanEnabled();
 
         if (!isScanEnabledInPrefs && intent.getBooleanExtra(ACTION_NOT_FROM_HOST_APP, false)) {
             stopSelf();
             return;
         }
 
         boolean hasFilesWaiting = !DataStorageManager.getInstance().isDirEmpty();
         if (AppGlobals.isDebug) {
             Log.d(LOG_TAG, "Files waiting:" + hasFilesWaiting);
         }
         if (hasFilesWaiting) {
             // non-empty on startup, schedule an upload
             // This is the only upload trigger in Firefox mode
             // Firefox triggers this ~4 seconds after startup (after Gecko is loaded), add a small delay to avoid
             // clustering with other operations that are triggered at this time.
-            final long lastAttemptedTime = Prefs.getInstance().getLastAttemptedUploadTime();
+            final long lastAttemptedTime = Prefs.getInstance(this).getLastAttemptedUploadTime();
             final long timeNow = System.currentTimeMillis();
 
             if (timeNow - lastAttemptedTime < PASSIVE_UPLOAD_FREQ_GUARD_MSEC) {
                 // TODO Consider telemetry to track this.
                 if (AppGlobals.isDebug) {
                     Log.d(LOG_TAG, "Upload attempt too frequent.");
                 }
             } else {
-                Prefs.getInstance().setLastAttemptedUploadTime(timeNow);
+                Prefs.getInstance(this).setLastAttemptedUploadTime(timeNow);
                 UploadAlarmReceiver.scheduleAlarm(this, DELAY_IN_SEC_BEFORE_STARTING_UPLOAD_IN_PASSIVE_MODE, false /* no repeat*/);
             }
         }
 
         if (!isScanEnabledInPrefs) {
-            Prefs.getInstance().setFirefoxScanEnabled(true);
+            Prefs.getInstance(this).setFirefoxScanEnabled(true);
         }
 
         String apiKey = intent.getStringExtra(ACTION_EXTRA_MOZ_API_KEY);
-        if (apiKey != null && !apiKey.equals(Prefs.getInstance().getMozApiKey())) {
-            Prefs.getInstance().setMozApiKey(apiKey);
+        if (apiKey != null && !apiKey.equals(Prefs.getInstance(this).getMozApiKey())) {
+            Prefs.getInstance(this).setMozApiKey(apiKey);
         }
 
         String userAgent = intent.getStringExtra(ACTION_EXTRA_USER_AGENT);
-        if (userAgent != null && !userAgent.equals(Prefs.getInstance().getUserAgent())) {
-            Prefs.getInstance().setUserAgent(userAgent);
+        if (userAgent != null && !userAgent.equals(Prefs.getInstance(this).getUserAgent())) {
+            Prefs.getInstance(this).setUserAgent(userAgent);
         }
 
         if (!mScanManager.isScanning()) {
             startScanning();
         }
     }
 
     // Note that in passive mode, having data isn't an upload trigger, it is triggered by the start intent
--- a/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/GPSScanner.java
+++ b/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/GPSScanner.java
@@ -159,17 +159,21 @@ public class GPSScanner implements Locat
         return mLocation;
     }
 
     public void checkPrefs() {
         if (mBlockList != null) {
             mBlockList.updateBlocks();
         }
 
-        mAutoGeofencing = Prefs.getInstance().getGeofenceHere();
+        Prefs prefs = Prefs.getInstanceWithoutContext();
+        if (prefs == null) {
+            return;
+        }
+        mAutoGeofencing = prefs.getGeofenceHere();
     }
 
     public boolean isGeofenced() {
         return (mBlockList != null) && mBlockList.isGeofenced();
     }
 
     private void sendToLogActivity(String msg) {
         AppGlobals.guiLogInfo(msg, "#33ccff", false);
@@ -181,17 +185,17 @@ public class GPSScanner implements Locat
             reportLocationLost();
             return;
         }
 
         String logMsg = (mIsPassiveMode)? "[Passive] " : "[Active] ";
 
         String provider = location.getProvider();
         if (!provider.toLowerCase().contains("gps")) {
-            sendToLogActivity(logMsg + "Discard fused/network location.");
+            Log.d(LOG_TAG, "Discard fused/network location.");
             // only interested in GPS locations
             return;
         }
 
         // Seem to get greater likelihood of non-fused location with higher update freq.
         // Check dist and time threshold here, not set on the listener.
         if (mIsPassiveMode) {
             final long timeDelta = location.getTime() - mLocation.getTime();
--- a/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/LocationBlockList.java
+++ b/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/LocationBlockList.java
@@ -23,18 +23,22 @@ public final class LocationBlockList {
     private boolean mGeofencingEnabled;
     private boolean mIsGeofenced = false;
 
     public LocationBlockList() {
         updateBlocks();
     }
 
     public void updateBlocks()    {
-        mBlockedLocation = Prefs.getInstance().getGeofenceLocation();
-        mGeofencingEnabled = Prefs.getInstance().getGeofenceEnabled();
+        Prefs prefs = Prefs.getInstanceWithoutContext();
+        if (prefs == null) {
+            return;
+        }
+        mBlockedLocation = prefs.getGeofenceLocation();
+        mGeofencingEnabled = prefs.getGeofenceEnabled();
     }
 
     public boolean contains(Location location) {
         final float inaccuracy = location.getAccuracy();
         final double altitude = location.getAltitude();
         final float bearing = location.getBearing();
         final double latitude = location.getLatitude();
         final double longitude = location.getLongitude();
--- a/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/WifiScanner.java
+++ b/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/WifiScanner.java
@@ -69,22 +69,23 @@ public class WifiScanner extends Broadca
         if (manager == null) {
             return null;
         }
         return getWifiManager().getScanResults();
     }
 
 
     public synchronized void start(final ActiveOrPassiveStumbling stumblingMode) {
-        if (mStarted) {
+        Prefs prefs = Prefs.getInstanceWithoutContext();
+        if (mStarted || prefs == null) {
             return;
         }
         mStarted = true;
 
-        boolean scanAlways = Prefs.getInstance().getWifiScanAlways();
+        boolean scanAlways = prefs.getWifiScanAlways();
 
         if (scanAlways || isWifiEnabled()) {
             activatePeriodicScan(stumblingMode);
         }
 
         IntentFilter i = new IntentFilter(WifiManager.SCAN_RESULTS_AVAILABLE_ACTION);
         if (!scanAlways) i.addAction(WifiManager.WIFI_STATE_CHANGED_ACTION);
         mContext.registerReceiver(this, i);
--- a/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/uploadthread/AsyncUploader.java
+++ b/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/uploadthread/AsyncUploader.java
@@ -107,20 +107,16 @@ public class AsyncUploader extends Async
     @Override
     protected void onCancelled(SyncSummary result) {
         sIsUploading.set(false);
     }
 
     private class Submitter extends AbstractCommunicator {
         private static final String SUBMIT_URL = "https://location.services.mozilla.com/v1/submit";
 
-        public Submitter() {
-            super(Prefs.getInstance().getUserAgent());
-        }
-
         @Override
         public String getUrlString() {
             return SUBMIT_URL;
         }
 
         @Override
         public String getNickname(){
             return mNickname;
--- a/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/uploadthread/UploadAlarmReceiver.java
+++ b/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/uploadthread/UploadAlarmReceiver.java
@@ -73,19 +73,19 @@ public class UploadAlarmReceiver extends
                     return;
                 }
             }
 
             if (NetworkUtils.getInstance().isWifiAvailable() &&
                 !AsyncUploader.isUploading()) {
                 Log.d(LOG_TAG, "Alarm upload(), call AsyncUploader");
                 AsyncUploader.UploadSettings settings =
-                    new AsyncUploader.UploadSettings(Prefs.getInstance().getWifiScanAlways(), Prefs.getInstance().getUseWifiOnly());
+                    new AsyncUploader.UploadSettings(Prefs.getInstance(this).getWifiScanAlways(), Prefs.getInstance(this).getUseWifiOnly());
                 AsyncUploader uploader = new AsyncUploader(settings, null);
-                uploader.setNickname(Prefs.getInstance().getNickname());
+                uploader.setNickname(Prefs.getInstance(this).getNickname());
                 uploader.execute();
                 // we could listen for completion and cancel, instead, cancel on next alarm when db empty
             }
         }
     }
 
     static PendingIntent createIntent(Context c, boolean isRepeating) {
         Intent intent = new Intent(c, UploadAlarmReceiver.class);
--- a/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/AbstractCommunicator.java
+++ b/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/AbstractCommunicator.java
@@ -56,24 +56,26 @@ public abstract class AbstractCommunicat
     }
 
     public abstract NetworkSendResult cleanSend(byte[] data);
 
     public String getNickname() {
         return null;
     }
 
-    public AbstractCommunicator(String userAgent) {
-        mUserAgent = userAgent;
+    public AbstractCommunicator() {
+        Prefs prefs = Prefs.getInstanceWithoutContext();
+        mUserAgent = (prefs != null)? prefs.getUserAgent() : "fennec-stumbler-unset-user-agent";
     }
 
     private void openConnectionAndSetHeaders() {
         try {
-            if (sMozApiKey == null) {
-                sMozApiKey = Prefs.getInstance().getMozApiKey();
+            Prefs prefs = Prefs.getInstanceWithoutContext();
+            if (sMozApiKey == null || prefs != null) {
+                sMozApiKey = prefs.getMozApiKey();
             }
             URL url = new URL(getUrlString() + "?key=" + sMozApiKey);
             mHttpURLConnection = (HttpURLConnection) url.openConnection();
             mHttpURLConnection.setRequestMethod("POST");
         } catch (MalformedURLException e) {
             throw new IllegalArgumentException(e);
         } catch (IOException e) {
             Log.e(LOG_TAG, "Couldn't open a connection: " + e);