1038843 - Integrate stumbler into Fennec

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Geolocation, defect)

Description

[:garvan]

Cloned from bug 1024708

See my notes here: https://bugzilla.mozilla.org/show_bug.cgi?id=1024708#c40

Conceptually, this code needs to go from side-project quality to production quality. There has been some review, but less review in recent weeks as the refactor got more aggressive. My fault for not tracking someone down to review during the process. The project relies heavily on community QA, which has been invaluable.

Large chunks of code we not touched, however, I think all the code needs review (in the service). Some of the bugs I would find were in code that was thought to be well-covered, reliable. But I would find bugs in there like LTE network scanning code not hooked up, because of an inverted if statement.

Question I have is, do I attach the code to this bug, or is it reviewed here:
https://github.com/mozilla/MozStumbler

Comment 1

[Richard Newman [:rnewman]]

Nick and I are planning to read through the existing code today or later this week. We've already briefly skimmed.

We'll read in the GitHub repo to get a sense of context, and see where we go from there.


To record our expectations for posterity:

It's quite possible that we'll need to do some rework in order to turn this on, above and beyond mere code review/tests/etc.

For example, tweaks to the upload strategy to avoid any power impact (consider direct upload on capture and shortly after Fennec startup, rather than any kind of timer and a separate thread); switching to a different storage model (perhaps a CP with in-memory buffering that flushes slugs to disk, rather than using an enormous DB handle to store a few KB); and I've heard rumblings of some necessary settings work. We'll also need to address pruning, if you haven't already, and file the follow-ups for telemetry and FHR.

My understanding is that you're hoping to substantially share the code with MozStumbler; that might not work out in the end, particularly given that in Fennec we're aiming for zero resource utilization -- another DB and periodic wakelock usage might not be suitable.

This might turn into "implement a simple in-Fennec stumbler that uses MozStumbler's data formatter and uploader". We'll see when we read the code!

Comment 2

[:garvan]

Thanks Richard, I know your notes are preliminary, but I'll comment anyway.

> For example, tweaks to the upload strategy to avoid any power impact
> (consider direct upload on capture and shortly after Fennec startup, rather
> than any kind of timer and a separate thread);

https://github.com/mozilla/MozStumbler/blob/master/src/org/mozilla/mozstumbler/service/StumblerService.java#L193
startPassiveModeUploadTimer(), triggered only after data is captured, stopped when no data in db.
I should add a unit test in stumbler to demonstrate the expected capture rate, and demonstrate that data never aggregates in db in the common case. When I am stumbling, it collects 1-2kb every few mins, and uploads it every few mins, timer shuts off.

 switching to a different
> storage model (perhaps a CP with in-memory buffering that flushes slugs to
> disk, rather than using an enormous DB handle to store a few KB);

Yes, a db is not necessary here. Prior to the refactor, I think the DB had a clearer role, that it was used for rapid development, along with the Sync Service. The stumbles could be 1kb files saved to disk (1kb can store 50 stumbles).

 and I've
> heard rumblings of some necessary settings work. 

With no UI, it is very hard for anyone (other than devs) to see what is happening without turning on some logging in the prefs. That, and the future setting for allowing upload on cell data are the only 2 I'll be pushing for.

> We'll also need to address
> pruning, 

Pruning of what?

> My understanding is that you're hoping to substantially share the code with
> MozStumbler; that might not work out in the end, particularly given that in
> Fennec we're aiming for zero resource utilization -- another DB and periodic
> wakelock usage might not be suitable.

I would say this is an absolute requirement as it is our greatest asset on this project, moreover, I can't imagine how MozStumbler wouldn't be able to use the service. MozStumbler is a client of the stumbler service, the same way Fennec is. With MozStumbler, we are getting a few hundred users QA'ing the code, users who are knowledgeable enough to be as valued as Mozilla QA. And they are happy to try out prototype ideas, or test unstable builds. CPeterson has an aggressive auto-update feature in MozStumbler, so when releases go out, feedback starts within a day. 
MozStumbler is about to get some additional investment ($$$), in hopes of pushing the user base in the tens of thousands. I'd rather find out the problems from MozStumbler users before it gets into Fennec.

Comment 3

[:garvan]

One more idea to reduce impact. For the initial version, we could not store any data to disk. Aggregate 1kb or so of stumbles in memory and then upload them (only if wifi active [and maybe also 'is screen on', I like that added condition as it ensures the device is not in low power state]). Limit uploads to 250kb an hour or something.

Perhaps allow for some small amount of circular memory buffer, 100kb or so. If we get killed, or we fail to clear the buffer after a day or so, it is wiped. The data collection goal is sparse, pseudo-random collection. I am not aware of any data integrity requirements. 

This may actually be a long term solution when we can get an option to allow for cell data uploads, or if we determine cell data can be used because data uses is sufficiently low (and bounded in volume).

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Garvan Keeley [:garvank] from comment #2)
> Thanks Richard, I know your notes are preliminary, but I'll comment anyway.
> 
> > For example, tweaks to the upload strategy to avoid any power impact
> > (consider direct upload on capture and shortly after Fennec startup, rather
> > than any kind of timer and a separate thread);
> 
> https://github.com/mozilla/MozStumbler/blob/master/src/org/mozilla/
> mozstumbler/service/StumblerService.java#L193
> startPassiveModeUploadTimer(), triggered only after data is captured,
> stopped when no data in db.
> I should add a unit test in stumbler to demonstrate the expected capture
> rate, and demonstrate that data never aggregates in db in the common case.
> When I am stumbling, it collects 1-2kb every few mins, and uploads it every
> few mins, timer shuts off.

This might be worrying. I thought the prevailing thought around power efficient uploading was to upload a larger amount of data a few times, not to upload a small amount of data more often.

The cell modem has a power cycle that keeps it at half power for a time period after uploading has finished. We need to be careful that we don't end up keeping the modem at full or half power many times a day.

Comment 5

[:garvan]

Marking comment 3 as spam, was thinking out loud, and in retrospect, this will result in far too little data, basically 90% of data will be thrown out.

Comment 4. Its current form is handy for testing/development, but not sensible for production. Agreed: the upload needs to change to upload a larger amount of data and not use a timer.

Comment 6

[Richard Newman [:rnewman]]

> I should add a unit test in stumbler to demonstrate the expected capture
> rate, and demonstrate that data never aggregates in db in the common case.
> When I am stumbling, it collects 1-2kb every few mins, and uploads it every
> few mins, timer shuts off.

I think we can accept writing 1-2KB to disk every few minutes, accruing in memory the rest of the time, and uploading once or twice a day. This isn't time-sensitive data, so we should heavily skew in favor of batching.


> Yes, a db is not necessary here. Prior to the refactor, I think the DB had a
> clearer role, that it was used for rapid development, along with the Sync
> Service. The stumbles could be 1kb files saved to disk (1kb can store 50
> stumbles).

Great!

Presumably we can tune that batch size, and the upload threshold, as we learn more?


> With no UI, it is very hard for anyone (other than devs) to see what is
> happening without turning on some logging in the prefs. That, and the future
> setting for allowing upload on cell data are the only 2 I'll be pushing for.

Yup, but we might also need to rev the Fennec data choice opt-out, or otherwise improve the experience when legal/privacy gets involved. (Been there, done that!)


> Pruning of what?

Imagine we have "upload only on wifi" as the default. Imagine a user who isn't home for two weeks -- or two months! -- and is using their unlimited cell data instead of crap hotel wifi, but leaving wifi on to help with Google Maps. At some point we need to start throwing data away, rather than filling up their storage.

We're talking maybe 1.5MB of stored data in a month. On some phones that's non-trivial. We'll probably want a handler for ACTION_DEVICE_STORAGE_LOW that throws away queued data, and the same for in-memory.

Our principles have to include "don't be selfish" -- the user is doing us a favor with all data they contribute to Mozilla, so we must tread lightly and cherish their resources.


> I would say this is an absolute requirement as it is our greatest asset on
> this project, moreover, I can't imagine how MozStumbler wouldn't be able to
> use the service.

My (preliminary) thinking was that MozStumbler wants to aggressively collect, proactively display (e.g., mapping?, but at least counting), and frequently upload; Fennec wants to passively collect, never display, and rarely upload, with a strong bias towards zero power usage.

That you want to move away from a DB for the service indicates that perhaps these requirements are closer than I imagined, which is great.

But I can certainly imagine tension between the two sets, based on the above, and we'd have to break that tension either by partly splitting implementations (even if just by making the service more flexible), or having MozStumbler jump through hoops (such as parsing data from slug files versus simply querying a DB).

(It might already jump through some of the needed hoops, which will help!)

Comment 7

[:garvan]

> We're talking maybe 1.5MB of stored data in a month. On some phones that's
> non-trivial. We'll probably want a handler for ACTION_DEVICE_STORAGE_LOW
> that throws away queued data, and the same for in-memory.
> 
> Our principles have to include "don't be selfish" -- the user is doing us a
> favor with all data they contribute to Mozilla, so we must tread lightly and
> cherish their resources.

Ok I understand. The code deletes the entire db if any data is >2wks old. Passive stumbling is limited to an entry in the db every 3 mins and 30 metres. So 20 per hour, 60 stumbles is <1kb, 8kb/day. Now that I type this out I am thinking I went a little too defensive on this :).

Comment 8

[:garvan]

> My (preliminary) thinking was that MozStumbler wants to aggressively
> collect, proactively display (e.g., mapping?, but at least counting), and
> frequently upload; Fennec wants to passively collect, never display, and
> rarely upload, with a strong bias towards zero power usage.

MozStumbler wants different modes, one of which is a low power mode. The frequency of upload in MozStumbler is arbitrary, could be more or less often, the data is stored on disk until upload is successful. Some folks want want manual control only of the upload in MozStumbler, they prefer to collect large amounts of data, then trigger the upload themselves.

If the stumbler service for Fennec switches to file-based storage, then MozStumbler will too. 

In terms of visualization, I would imagine at some point we would want to have an about:stumbling page that displays the data that MozStumbler displays.

Comment 9

[:garvan]

> Presumably we can tune that batch size, and the upload threshold, as we
> learn more?

Everything can and will be tuned as we get this in. It is very much 'learn as we go" but be defensive in the process. We have assumptions about user behaviours, how often people look stuff up on Google Maps, or use a navigation app, and these assumptions are used to estimate the amount of data, they type of data, etc. We have much to learn by real-world use of this.

Comment 10

[:garvan]

> Ok I understand. The code deletes the entire db if any data is >2wks old.
> Passive stumbling is limited to an entry in the db every 3 mins and 30
> metres. So 20 per hour, 60 stumbles is <1kb, 8kb/day. Now that I type this
> out I am thinking I went a little too defensive on this :).

I am on crack. Let me correct this, a single stumble for me is 0.5 kb or less given 15 wifi networks and 2-3 cell towers. So, 20stumbles/hour is 10kb/hour, ~1.5 MB a week. 
Impossible to stumble 24 hours a day, probably hard to hit 250kb in a week.

Comment 11

[:garvan]

I think B2G has fried my brain. Let me do this one more time. 

- 3 *seconds* and 30 meters are required between collections.
- Max stumble size I get is 0.5 kb
- 0.5 kb * 20 stumbles/min = 10 kb/min max or ~0.5MB/hour

Forget everything I said earlier :), data volumes can be significant (someone in a car running a navigation app for may hours a day).

Comment 12

[Nick Alexander :nalexander [he/him]]

Clearing my NI since I'm going on PTO for 2.5 weeks in a day or so.

Comment 13

[Richard Newman [:rnewman]]

Attachment: Current service code at 8992e1ca10e635a078f49b6b847dd627fb4d8937.

This is the output of

git diff "5d38994b2a807fda25aac098df0ec8a227562e0e^" . > /tmp/service.patch

from within the service dir.

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8465878 [details] [diff] [review]
Current service code at 8992e1ca10e635a078f49b6b847dd627fb4d8937.

Review of attachment 8465878 [details] [diff] [review]:
-----------------------------------------------------------------

I did a quick skim through an assortment of code to pick up high-level issues.

The main one is a lack of emphasis on concurrency: more immutable objects, documentation of concurrency assumptions, synchronization and volatile where appropriate, etc.

I haven't yet reviewed the ins and outs of saving and upload, but I note that there's still an upload alarm receiver, which seems to run counter to our earlier discussions.

More when I get time!

::: src/org/mozilla/mozstumbler/service/AppGlobals.java
@@ +10,5 @@
> +public class AppGlobals {
> +    /** All intent actions start with this string  */
> +    public static final String ACTION_NAMESPACE = "org.mozilla.mozstumbler.intent.action";
> +
> +    /** Handle this for logging reporter info */

Nit: double-* implies a JavaDoc comment. Single is fine.

@@ +14,5 @@
> +    /** Handle this for logging reporter info */
> +    public static final String ACTION_GUI_LOG_MESSAGE = AppGlobals.ACTION_NAMESPACE + ".LOG_MESSAGE";
> +    public static final String ACTION_GUI_LOG_MESSAGE_EXTRA = ACTION_GUI_LOG_MESSAGE + ".MESSAGE";
> +
> +    /** A common intent action argument, but you shouldn't need  to access this directly,

Nit: extra spacing.

@@ +48,5 @@
> +        guiLogInfo(msg, "white", false);
> +    }
> +
> +    public static void guiLogInfo(String msg, String color, boolean isBold) {
> +        if (guiLogMessageBuffer != null) {

This seems like something that needs attention before shipping the service as part of Fennec.

@@ +55,5 @@
> +            guiLogMessageBuffer.add("<font color='" + color +"'>" + msg + "</font>");
> +        }
> +    }
> +
> +    public static DataStorageManager dataStorageManager;

It's public, it's not assigned, and it's not volatile. Bad sign, concurrency-wise. Why is this here?

::: src/org/mozilla/mozstumbler/service/Prefs.java
@@ +14,5 @@
> +import android.util.Log;
> +
> +public final class Prefs {
> +    private static final String     LOGTAG        = Prefs.class.getName();
> +    public  static final String     PREFS_FILE    = Prefs.class.getName();

These don't seem right...

@@ +27,5 @@
> +    private static final String     FIREFOX_SCAN_ENABLED = "firefox_scan_on";
> +    private static final String     MOZ_API_KEY = "moz_api_key";
> +
> +    private final SharedPreferences mSharedPrefs;
> +    static private Prefs sInstance;

Concurrency red flag.

@@ +49,5 @@
> +     * TODO: turn into regular singleton if Context dependency can be removed. */
> +    public static void createGlobalInstance(Context c) {
> +        if (sInstance != null)
> +            return;
> +        sInstance = new Prefs(c);

This is what an `init` method is for. But why do you need a global instance anyway? SharedPreferences does this for you!

I quote:

   For any particular set of preferences, there is a single instance of this class that all clients share.

Your Prefs class ends up as really just a pre-defined name. Probably most of this class can just go away.

::: src/org/mozilla/mozstumbler/service/Reporter.java
@@ +22,5 @@
> +import org.mozilla.mozstumbler.service.scanners.cellscanner.CellScanner;
> +import org.mozilla.mozstumbler.service.scanners.GPSScanner;
> +import org.mozilla.mozstumbler.service.scanners.WifiScanner;
> +
> +final public class Reporter extends BroadcastReceiver {

public final

@@ +23,5 @@
> +import org.mozilla.mozstumbler.service.scanners.GPSScanner;
> +import org.mozilla.mozstumbler.service.scanners.WifiScanner;
> +
> +final public class Reporter extends BroadcastReceiver {
> +    private static final String LOGTAG = Reporter.class.getName();

Same comment as always.

@@ +24,5 @@
> +import org.mozilla.mozstumbler.service.scanners.WifiScanner;
> +
> +final public class Reporter extends BroadcastReceiver {
> +    private static final String LOGTAG = Reporter.class.getName();
> +    public  static final String ACTION_BASE = AppGlobals.ACTION_NAMESPACE;

Only used once; inline.

@@ +25,5 @@
> +
> +final public class Reporter extends BroadcastReceiver {
> +    private static final String LOGTAG = Reporter.class.getName();
> +    public  static final String ACTION_BASE = AppGlobals.ACTION_NAMESPACE;
> +    public  static final String ACTION_FLUSH_TO_DB = ACTION_BASE + ".FLUSH";

Rename: no DB.

@@ +30,5 @@
> +
> +    /**
> +     * The maximum time of observation
> +     */
> +    private static final int REPORTER_WINDOW  = 24 * 60 * 60 * 1000; //ms

Name this REPORTER_WINDOW_MSEC.

@@ +43,5 @@
> +     */
> +    private static final int CELLS_COUNT_WATERMARK = 50;
> +
> +    private final Context       mContext;
> +    private final int             mPhoneType;

This is why we don't do block alignment :)

@@ +46,5 @@
> +    private final Context       mContext;
> +    private final int             mPhoneType;
> +
> +    private StumblerBundle mBundle;
> +    private StumblerBundleReceiver mStumblerBundleReceiver;

Concurrency!

::: src/org/mozilla/mozstumbler/service/StumblerService.java
@@ +68,5 @@
> +    }
> +
> +    public void stopScanning() {
> +        if (mScanManager.isScanning()) {
> +            mScanManager.stopScanning();

This seems like a setup for a concurrency problem.

Why not, in ScanManager, something like:

  /**
   * @return true if we were scanning; false if already stopped. 
   */
  public synchronized boolean stopScanning() {


then here

  if (mScanManager.stopScanning()) {
    mReporter.flush();
  }

?

::: src/org/mozilla/mozstumbler/service/blocklist/BSSIDBlockList.java
@@ +9,5 @@
> +import java.util.Locale;
> +import java.util.regex.Pattern;
> +
> +public final class BSSIDBlockList {
> +    private static final String  LOGTAG = BSSIDBlockList.class.getName();

Nit: these should ideally be prefixed (e.g., with "Gecko").

And getName is almost certainly not what you want. That will return

  "org.mozilla.mozstumbler.service.blocklist.BSSIDBlockList"

which is too long to be an Android logtag (23 char max).

@@ +24,5 @@
> +        "7c6d62",
> +        "7cc537",
> +        "88c663",
> +        "8c7712",
> +        

Nit: trailing whitespace.

@@ +43,5 @@
> +    private BSSIDBlockList() {
> +    }
> +
> +    public static boolean contains(ScanResult scanResult) {
> +        String BSSID = scanResult.BSSID;

Prefer `final` whenever you can; the compiler will catch your errors.

@@ +74,5 @@
> +
> +        // Some devices may return BSSIDs with '-' or '.' delimiters.
> +        BSSID = BSSID.toLowerCase(Locale.US).replace(":", "")
> +                                            .replace("-", "")
> +                                            .replace(".", "");

Consider .replaceAll("\\.-:", "").

::: src/org/mozilla/mozstumbler/service/blocklist/SSIDBlockList.java
@@ +40,5 @@
> +        "FirefoxHotspot",
> +        "Mobile Hotspot", // BlackBerry OS 10
> +
> +        // Transportation Wi-Fi
> +        ".DeinBus.de", // DeinBus on-bus WiFi (Germany)

This does not look like a scalable solution for this problem. Consider alternatives for the future:

* Shipping some kind of bloom filter.
* Server-side filtering.
* Client-side filtering from a downloadable blocklist.

As a short-term solution, you might consider a build step that turns this list into a minimal regex, which will allow you to do one test with the regex rather than a painful linear search, and will probably be smaller than this list.

@@ +112,5 @@
> +        "iphone",
> +        "MIFI",
> +        "MiFi",
> +        "Mifi",
> +        "mifi",

Case-insensitive comparison instead?

::: src/org/mozilla/mozstumbler/service/datahandling/DataStorageContract.java
@@ +13,5 @@
> +
> +public final class DataStorageContract {
> +
> +    public static class ReportsColumns {
> +        public final static String LAT = "lat";

Style nit: we tend to go with "public static final", but nbd.

::: src/org/mozilla/mozstumbler/service/datahandling/DataStorageManager.java
@@ +8,5 @@
> +import android.database.Cursor;
> +import android.database.sqlite.SQLiteDatabase;
> +import android.util.Log;
> +
> +import org.mozilla.mozstumbler.R;

This might be problematic when integrated into the build.

@@ +23,5 @@
> +import java.util.Map;
> +import java.util.Properties;
> +import java.util.ResourceBundle;
> +
> +/** Stores reports in memory (mCurrentReports) until MAX_REPORTS_IN_MEMORY,

Nit:

  /**
   * Stores ...

@@ +29,5 @@
> + * the time written, the # of reports, and the # of cells and wifis.
> + *
> + * Each .gz file is typically 1-5KB.
> + *
> + * The sync stats are written as a key-value pair file (not zipped).

My main concern with this file: there are a few methods labeled as synchronized, but plenty others that aren't. It would help to have a little time spent organizing and labeling chunks of members and methods based on their concurrency facts, making it clear what's going on from a threading perspective.

@@ +43,5 @@
> + * Also of note: the in-memory buffers (both mCurrentReports and mCurrentReportsSendBuffer) are saved
> + * when the service is destroyed.
> + */
> +public class DataStorageManager {
> +    private final String LOG_TAG = DataStorageManager.class.getName();

Same.

@@ +45,5 @@
> + */
> +public class DataStorageManager {
> +    private final String LOG_TAG = DataStorageManager.class.getName();
> +    private final int MAX_REPORTS_IN_MEMORY = 50;
> +    private long mMaxBytesDiskStorage = 1024 * 1024 * 3; // 3 megabytes max by default

Is this mutable for a reason?

3MB seems high for a default, unless you're storing data in the Android cache directory.

@@ +46,5 @@
> +public class DataStorageManager {
> +    private final String LOG_TAG = DataStorageManager.class.getName();
> +    private final int MAX_REPORTS_IN_MEMORY = 50;
> +    private long mMaxBytesDiskStorage = 1024 * 1024 * 3; // 3 megabytes max by default
> +    private int mMaxWeeksStored = 2;

Same re mutable.

@@ +48,5 @@
> +    private final int MAX_REPORTS_IN_MEMORY = 50;
> +    private long mMaxBytesDiskStorage = 1024 * 1024 * 3; // 3 megabytes max by default
> +    private int mMaxWeeksStored = 2;
> +    private ReportBatchBuilder mCurrentReports = new ReportBatchBuilder();
> +    private int mTotalReportCount;

There is a lot of mutable state going on here. Time to put on my thread-safety hat.

@@ +53,5 @@
> +    private File mReportsDir;
> +    private File mStatsFile;
> +    private ReportBatch mCurrentReportsSendBuffer;
> +    private ReportBatchIterator mReportBatchIterator;
> +    private DatabaseIsEmptyTracker mTracker;

Most of these can be final.

@@ +67,5 @@
> +    public static class QueuedCounts {
> +        public int reportCount;
> +        public int wifiCount;
> +        public int cellCount;
> +        public long bytes;

Make these final, add a constructor, and build it in one shot in getQueuedCounts.

@@ +107,5 @@
> +        long filesOnDiskBytes;
> +
> +        public ReportFileList() {}
> +        public ReportFileList(ReportFileList other) {
> +            files = other.files.clone();

other.files can be null.

@@ +117,5 @@
> +
> +        void update(File directory) {
> +            files = directory.listFiles();
> +            if (files == null)
> +                return;

Style nit: always brace conditionals.

@@ +134,5 @@
> +        public byte[] data;
> +        public String filename;
> +        public int reportCount;
> +        public int wifiCount;
> +        public int cellCount;

final plus constructor.

@@ +139,5 @@
> +    }
> +
> +    private static class ReportBatchBuilder {
> +        public ArrayList<String> reports = new ArrayList<String>();
> +        public int wifiCount;

Same.

@@ +160,5 @@
> +
> +    public Map<String, Long> getOldDbStats(Context context) {
> +        final File dbFile = new File(context.getFilesDir().getParent() + "/databases/" + "stumbler.db");
> +        if (!dbFile.exists())
> +            return null;

{}

@@ +166,5 @@
> +        SQLiteDatabase db = SQLiteDatabase.openDatabase(dbFile.toString(), null, 0);
> +
> +        Cursor cursor = null;
> +        try {
> +            cursor = db.rawQuery("select * from stats", null);

final Cursor cursor = db.rawQuery(...);
if (cursor == null) {
  // Should only happen if there's a syntax error.
  return null;
}
try {
  ...
} finally {
  cursor.close();
}

@@ +178,5 @@
> +                Long value = cursor.getLong(cursor.getColumnIndex("value"));
> +                kv.put(key, value);
> +                cursor.moveToNext();
> +            }
> +            assert(kv.size() == 4);

Is an assertion really what you want here?

@@ +203,5 @@
> +            dir = c.getFilesDir();
> +        }
> +
> +        if (!dir.exists()) {
> +            dir.mkdirs();

You must check the result value here.

@@ +207,5 @@
> +            dir.mkdirs();
> +        }
> +        return dir.getPath();
> +    }
> +    

Nit: whitespace.

@@ +359,5 @@
> +    public void saveCurrentReportsSendBufferToDisk() throws IOException {
> +        if (mCurrentReportsSendBuffer == null)
> +            return;
> +
> +        saveToDisk(mCurrentReportsSendBuffer.data, 

Nit: trailing whitespace. (throughout)

@@ +424,5 @@
> +    }
> +
> +    public Properties readSyncStats() throws IOException {
> +        if (!mStatsFile.exists())
> +            return new Properties();

{}

@@ +439,5 @@
> +    }
> +
> +    public void incrementSyncStats(long bytesSent, long reports, long cells, long wifis) throws IOException {
> +        if (reports + cells + wifis < 1)
> +            return;

{}

@@ +453,5 @@
> +
> +    public void writeSyncStats(long time, long bytesSent, long totalObs, long totalCells, long totalWifis) throws IOException {
> +        FileOutputStream out = null;
> +        try {
> +            out = new FileOutputStream(mStatsFile);

Same as the cursor pattern.

@@ +461,5 @@
> +            props.setProperty(DataStorageContract.Stats.KEY_OBSERVATIONS_SENT, "" + totalObs);
> +            props.setProperty(DataStorageContract.Stats.KEY_CELLS_SENT, "" + totalCells);
> +            props.setProperty(DataStorageContract.Stats.KEY_WIFIS_SENT, "" + totalWifis);
> +            props.setProperty(DataStorageContract.Stats.KEY_VERSION, "" + DataStorageContract.Stats.VERSION_CODE);
> +            props.store(out, null);

... shoulda null checked!

@@ +477,5 @@
> +    }
> +
> +    public void notifyDbIsEmpty(boolean isEmpty) {
> +        if (mTracker != null)
> +            mTracker.databaseIsEmpty(isEmpty);

{}

Comment 15

[:garvan]

Thanks Richard, updates to these items are here:

The upload alarm is still used, but the Fennec upload ping is TODO here:
https://github.com/mozilla/MozStumbler/issues/744
I'll follow up with you on that, pretty quick issue to address (I think).

I was cleaning up some last minute items in DataStorageManaager.java, and snuck a some cleanup in:
https://github.com/mozilla/MozStumbler/pull/745
Specifically this commit: 0ffe83

The BIG syntax cleanup PR after reading this bug is here:
https://github.com/mozilla/MozStumbler/pull/752

The Blocklist-related code I can just pull, it isn't necessary for Fennec. I have some bugs filed about refactoring that. Fennec only needs to respect _nomap, which is a one-line if-statement :).

The Prefs code, I have no particular preference :) as to how it is implemented, other than the codebase (as-written) requires accessing it from *.java without a context. 

*Concurrency* is the elephant in the room that we can still ignore I hope. In Fennec, stumbler is single-threaded, on its own thread. The DataStorageManager *is* accessed concurrently by MozStumbler UI, so I had sloppily synchronized parts of the class. In my cleanup PRs, I made this class have a proper synchronized public interface. 

p.s. just occurred to me I should probably just dump the updated code to a patch, and put in this bug. Will do that.

Comment 16

[:garvan]

Attachment: code up to 54bb06f

Comment 17

[:garvan]

Attachment: code up to 54bb06f

Here is the all the latest service code. Not sure what the preferred format is for dropping this from git to here. I could show the diff from the prev patch?

Comment 18

[:garvan]

> *Concurrency* is the elephant in the room that we can still ignore I hope.
> In Fennec, stumbler is single-threaded, on its own thread. The
> DataStorageManager *is* accessed concurrently by MozStumbler UI, so I had
> sloppily synchronized parts of the class. In my cleanup PRs, I made this
> class have a proper synchronized public interface. 

Oops, what, whoa -this way is wrong, sorry. The upload process *is* concurrent. All the data collection, writing to disk, (and everything *else* that happens), is single-threaded synchronous. The upload process calls in to the data storage class to do the uploads, and calls synchronized methods on the data storage class. But the utility classes are not thread-safe, and there is no guarantee the code won't change in such a way to introduce a concurrency bug. 

I'll have to update the patch to address this. Also I have the TODO of adding the Fennec hook for upload triggering.

Comment 19

[:garvan]

Comment on attachment 8466501 [details] [diff] [review]
code up to 54bb06f

Clearing the review flag, more cleanup tomorrow

Comment 20

[:garvan]

Attachment: code_up_to_6aeb285.patch

not sure best way to format this attachment.

Git log is here https://github.com/mozilla/MozStumbler/pull/752

Comment 21

[:garvan]

Attachment: service_code_up_to_e2bc221.patch

Updating this to match the latest commit:
https://github.com/mozilla/MozStumbler/pull/752

In my Fennec stumbling, I am satisfied with the upload mechanism, and data volumes (I am seeing only about 10 KB a day). The upload happens for me after I arrive at home/office and open Fennec, and mixes in with web browsing network traffic.

Comment 22

[Richard Newman [:rnewman]]

Comment on attachment 8468945 [details] [diff] [review]
service_code_up_to_e2bc221.patch

Splinter seems pretty unhappy about this, showing every file as removed, and I think there's something you still wanted to fix, so clearing flag for now.

Comment 23

[:garvan]

Attachment: stumbler code up to b5854f5

The latest patches from https://github.com/mozilla/MozStumbler/pull/752.

This patch includes the necessary changes in m/a/b/BrowserApp.java. If you patch this onto the m-c code, and turn on MOZ_ANDROID_MLS_STUMBLER in configure.in, build, turn on the pref in the settings, you are a-stumbling.

My testing included hitting the pref on-off in various circumstances.

Filter messages with adb logcat | grep Stumbler

Comment 24

[Richard Newman [:rnewman]]

Can we split this into three?

1. The generated patch.
2. The build integration, complete with flag, defines, moz.build, etc. -- Nick will review this.
3. The settings changes. I'll ask another pair of eyes to make sure this is right.

Thanks!

Comment 25

[:garvan]

Good point, the build config changes are supposed to go in bug 1038923.

Comment 26

[Richard Newman [:rnewman]]

Either here, or morph that bug; don't mind. All gotta land together.

Comment 27

[:garvan]

Attachment: stumbler_service_code_to_b5854f.patch

This is the service/ java code only

Comment 28

[:garvan]

Attachment: stumbler_manifest_and_mozbuild.patch

Changes to build config for latest stumbler code, also updated the stumbler manifest.

Comment 29

[:garvan]

Attachment: prefs_changes__build_and_code.patch

Preference handling code and build config changes.

1) GeckoPreferences.java gets a broadcastStumblerPref utility function, and also calls this on settings changed

2) MOZ_MOZILLA_API_KEY: handling in the build, and pass to service as a required bit of info in the start intent.

3) BrowserApp.java -> call the broadcastStumblerPref() onResume and onPause. This starts the service if need be, and tells the stumbler service that Fennec is running or has recently been run, now is a good time to upload any stumbles (more accurately, it schedules a check for upload a few seconds later).

Comment 32

[:garvan]

Comment on attachment 8469764 [details] [diff] [review]
stumbler_service_code_to_b5854f.patch

Hmm, that didn't split right, let's try again.

Comment 33

[:garvan]

Attachment: service_java_code_to_b5854f.patch

stumbler service java code on its own

Comment 34

[Richard Newman [:rnewman]]

Comment on attachment 8469769 [details] [diff] [review]
prefs_changes__build_and_code.patch

Review of attachment 8469769 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/AppConstants.java.in
@@ +92,5 @@
>      // MOZILLA_VERSION is already quoted when it gets substituted in. If we
>      // add additional quotes we end up with ""x.y"", which is a syntax error.
>      public static final String MOZILLA_VERSION = @MOZILLA_VERSION@;
>  
> +    public static final String MOZ_MOZILLA_API_KEY = "@MOZ_MOZILLA_API_KEY@";

Is this the best name we could use for this?

Should this have "STUMBLER" in there somewhere?

::: mobile/android/base/BrowserApp.java
@@ +639,5 @@
>      public void onResume() {
>          super.onResume();
>          EventDispatcher.getInstance().unregisterGeckoThreadListener((GeckoEventListener)this,
>              "Prompt:ShowTop");
> +        //Ping the stumbler lib; if it is running and has queued data, it will upload it

Nit: spacing, clarity, punctuation.

  // Broadcast the stumbler pref to cause the service to upload any
  // queued data.

I'm not sure if you're right or not for "it is running" -- Android will start the service to receive the broadcast.

Also, do this in Gecko:DelayedStartup, not in onResume.

@@ +650,5 @@
>          super.onPause();
>          // Register for Prompt:ShowTop so we can foreground this activity even if it's hidden.
>          EventDispatcher.getInstance().registerGeckoThreadListener((GeckoEventListener)this,
>              "Prompt:ShowTop");
> +       // Pings the stumbler lib, see also usage in onResume()

Nit: final period.

::: mobile/android/base/moz.build
@@ +701,5 @@
>  if CONFIG['MOZ_ANDROID_MLS_STUMBLER']:
>      main.included_projects += ['../FennecStumbler']
>      main.referenced_projects += ['../FennecStumbler']
> +
> +DEFINES['MOZ_MOZILLA_API_KEY'] = CONFIG['MOZ_MOZILLA_API_KEY']

This should probably (a) be conditional, and (b) be somewhere else in the file.

Comment 35

[Richard Newman [:rnewman]]

Comment on attachment 8469778 [details] [diff] [review]
service_java_code_to_b5854f.patch

Review of attachment 8469778 [details] [diff] [review]:
-----------------------------------------------------------------

This review will be in many bits.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/AppGlobals.java
@@ +8,5 @@
> +import org.mozilla.mozstumbler.service.datahandling.DataStorageManager;
> +import java.util.concurrent.ConcurrentLinkedQueue;
> +
> +public class AppGlobals {
> +    /** All intent actions start with this string  */

Nit: prefer comments to be full sentences with punctuation.

Don't mix JavaDoc /** */ comments with non-JavaDoc content. This (and below) can just be a //.

@@ +9,5 @@
> +import java.util.concurrent.ConcurrentLinkedQueue;
> +
> +public class AppGlobals {
> +    /** All intent actions start with this string  */
> +    public static final String ACTION_NAMESPACE = "org.mozilla.mozstumbler.intent.action";

If this is ever broadcast, then this should be per-package, so that stumblers from different Fennecs don't interact.

If it's never broadcast, and you always directly deliver intents to the destination service, then make a comment to that effect.

@@ +16,5 @@
> +    public static final String ACTION_GUI_LOG_MESSAGE = AppGlobals.ACTION_NAMESPACE + ".LOG_MESSAGE";
> +    public static final String ACTION_GUI_LOG_MESSAGE_EXTRA = ACTION_GUI_LOG_MESSAGE + ".MESSAGE";
> +
> +    /** A common intent action argument, but you shouldn't need to access this directly,
> +     * the specific intent action has its own constant that is assigned to this

This comment doesn't make perfect sense...

@@ +35,5 @@
> +    public static String appName = "StumblerService";
> +    public static boolean isDebug;
> +
> +    /* The log activity will clear this periodically, and display the messages */
> +    public static ConcurrentLinkedQueue<String> guiLogMessageBuffer;

Strictly speaking this should be volatile, because you presumably assign it from somewhere in MozStumbler, and check it in guiLogInfo.

Comment 36

[:garvan]

Good so far, will address all, except perhaps this:

> > +    /* The log activity will clear this periodically, and display the messages */
> > +    public static ConcurrentLinkedQueue<String> guiLogMessageBuffer;
> 
> Strictly speaking this should be volatile, because you presumably assign it
> from somewhere in MozStumbler, and check it in guiLogInfo.

Seems odd, that would imply changing assignment to the var during concurrent ops. This value is set by the MozStumbler GUI at startup and never changes, surely ConcurrentLinkedQueue's member data is volatile.

Comment 37

[:garvan]

Comment 36, never mind, you are right again. Just after writing the comment I looked at that header, and there is no contract ensuring what I said, and I was about to check the log class -but if I have to go that far into the code, it is already suspect from an API perspective.

Comment 38

[:garvan]

 
> ::: mobile/android/base/moz.build
> @@ +701,5 @@
> >  if CONFIG['MOZ_ANDROID_MLS_STUMBLER']:
> >      main.included_projects += ['../FennecStumbler']
> >      main.referenced_projects += ['../FennecStumbler']
> > +
> > +DEFINES['MOZ_MOZILLA_API_KEY'] = CONFIG['MOZ_MOZILLA_API_KEY']
> 
> This should probably (a) be conditional, and (b) be somewhere else in the
> file.

a) ok
b) Has to come after main is defined, where else would it go?

Comment 39

[:garvan]

Attachment: prefs_changes__build_and_code.patch

Addressed feedback from Comment 34

Comment 40

[:garvan]

Attachment: service_java_code_to_7ab0f64.patch

Comment 35 addressed.
https://github.com/garvankeeley/MozStumbler/commit/7ab0f643668a7307f75bb0b1cc23189b4b61d1d9

Comment 41

[Richard Newman [:rnewman]]

Comment on attachment 8470915 [details] [diff] [review]
prefs_changes__build_and_code.patch

Review of attachment 8470915 [details] [diff] [review]:
-----------------------------------------------------------------

Gotta make sure this works for non-stumbler build configurations.

::: mobile/android/base/AppConstants.java.in
@@ +92,5 @@
>      // MOZILLA_VERSION is already quoted when it gets substituted in. If we
>      // add additional quotes we end up with ""x.y"", which is a syntax error.
>      public static final String MOZILLA_VERSION = @MOZILLA_VERSION@;
>  
> +    public static final String MOZ_STUMBLER_API_KEY = "@MOZ_STUMBLER_API_KEY@";

This should be conditionalized, else:

 0:14.35 mozbuild.preprocessor.Error: ('/Users/rnewman/moz/hg/fx-team/mobile/android/base/AppConstants.java.in', 96, 'UNDEFINED_VAR', 'MOZ_STUMBLER_API_KEY')

::: mobile/android/base/BrowserApp.java
@@ +648,5 @@
>          // Register for Prompt:ShowTop so we can foreground this activity even if it's hidden.
>          EventDispatcher.getInstance().registerGeckoThreadListener((GeckoEventListener)this,
>              "Prompt:ShowTop");
> +        // Starts or pings the stumbler lib, see also usage in handleMessage(): Gecko:DelayedStartup.
> +        GeckoPreferences.broadcastStumblerPref(this);

This should be conditionalized on the build flag.

@@ +1371,5 @@
>  
> +                // Start (this acts as ping if started already) the stumbler lib; if the stumbler has queued data it will upload it.
> +                // Stumbler operates on its own thread, and startup impact is further minimized by delaying work (such as upload) a few seconds.
> +                GeckoPreferences.broadcastStumblerPref(this);
> +

Same.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +870,5 @@
> +       Intent intent = new Intent(PREFS_STUMBLER_ENABLED)
> +                .putExtra("pref", PREFS_GEO_REPORTING)
> +                .putExtra("branch", GeckoSharedPrefs.APP_PREFS_NAME)
> +                .putExtra("enabled", value)
> +                .putExtra("moz_mozilla_api_key", AppConstants.MOZ_STUMBLER_API_KEY);

You should check whether there's an API key. There won't be if the stumbler isn't built-in, or there's a build problem of some kind.

Comment 42

[Richard Newman [:rnewman]]

Comment on attachment 8470964 [details] [diff] [review]
service_java_code_to_7ab0f64.patch

Review of attachment 8470964 [details] [diff] [review]:
-----------------------------------------------------------------

This is a little bit context-free, 'cos it won't build yet, but here are some comments.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/StumblerService.java
@@ +26,5 @@
> +import java.io.IOException;
> +
> +public final class StumblerService extends PersistentIntentService
> +        implements DataStorageManager.StorageIsEmptyTracker {
> +    private static final String LOG_TAG = "Stumbler" + StumblerService.class.getSimpleName();

How about just = "StumblerService"?

Ordinarily we use "GeckoStumbler", but doesn't bother me too much.

@@ +34,5 @@
> +
> +    public enum FirefoxStumbleState {
> +        UNKNOWN, ON, OFF
> +    }
> +    public static FirefoxStumbleState sFirefoxStumblingEnabled = FirefoxStumbleState.UNKNOWN;

Make this thread-safe, and document what it's for.

@@ +37,5 @@
> +    }
> +    public static FirefoxStumbleState sFirefoxStumblingEnabled = FirefoxStumbleState.UNKNOWN;
> +
> +    private ScanManager mScanManager;
> +    private Reporter               mReporter;

Either no column alignment or all column alignment; no mixing.

@@ +89,5 @@
> +    public void setWifiBlockList(WifiBlockListInterface list) {
> +        mScanManager.setWifiBlockList(list);
> +    }
> +
> +    public Prefs getPrefs() { return Prefs.getInstance(); }

Multi-line please. Makes visual scanning easier.

@@ +92,5 @@
> +
> +    public Prefs getPrefs() { return Prefs.getInstance(); }
> +
> +    public void checkPrefs() {
> +        mScanManager.checkPrefs();

All of these `mScanManager` accesses will cause a user-visible crash via NPE if they're called prior to init, or if early parts of init fail.

Consider guarding them, throwing a better exception, or making them package-private as a vain attempt to discourage incorrect access.

Consider instead having a getScanManager method; if these are public, and thus not at all encapsulated -- you're caught between two stools -- you might as well leak the scan manager and allow the caller to do the null check.

@@ +137,5 @@
> +    }
> +
> +    // 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.
> +    private void init() {

I don't see any guards to make sure that we don't init multiple times.

@@ +162,5 @@
> +
> +        if (AppGlobals.appVersionCode < 1) {
> +            //TODO look at how to set these
> +            //SharedConstants.appVersionName =;
> +            //SharedConstants.appVersionCode =;

File or fix.

@@ +174,5 @@
> +        // Used to move these disk I/O ops off the calling thread
> +        new AsyncTask<Void, Void, Void>() {
> +            @Override
> +            protected Void doInBackground(Void... params) {
> +                if (AppGlobals.isDebug) Log.d(LOG_TAG, "onDestroy");

{}, newlines.

@@ +198,5 @@
> +    }
> +
> +    @Override
> +    protected void onHandleIntent(Intent intent) {
> +        if (intent != null && intent.getBooleanExtra(ACTION_START_PASSIVE, false)) {

Invert this conditional and early return.

@@ +230,5 @@
> +
> +    @Override
> +    public IBinder onBind(Intent intent) {
> +        mIsBound = true;
> +        if (AppGlobals.isDebug)Log.d(LOG_TAG, "onBind");

Multiline. (Same throughout file.)

@@ +238,5 @@
> +    @Override
> +    public boolean onUnbind(Intent intent) {
> +        if (AppGlobals.isDebug) Log.d(LOG_TAG, "onUnbind");
> +        if (!mScanManager.isScanning()) {
> +            stopSelf();

Why not call stopScanning? You don't want to flush?

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/datahandling/DataStorageManager.java
@@ +43,5 @@
> + * when the service is destroyed.
> + */
> +public class DataStorageManager {
> +    private final String LOG_TAG = "Stumbler:" + DataStorageManager.class.getSimpleName();
> +    private final int MAX_REPORTS_IN_MEMORY = 50;

These two should be static.

@@ +45,5 @@
> +public class DataStorageManager {
> +    private final String LOG_TAG = "Stumbler:" + DataStorageManager.class.getSimpleName();
> +    private final int MAX_REPORTS_IN_MEMORY = 50;
> +    private long mMaxBytesDiskStorage = 1024 * 1024; // 1 megabyte max by default
> +    private int mMaxWeeksStored = 2;

These two should probably be volatile -- read from service, set from UI?

@@ +46,5 @@
> +    private final String LOG_TAG = "Stumbler:" + DataStorageManager.class.getSimpleName();
> +    private final int MAX_REPORTS_IN_MEMORY = 50;
> +    private long mMaxBytesDiskStorage = 1024 * 1024; // 1 megabyte max by default
> +    private int mMaxWeeksStored = 2;
> +    private ReportBatchBuilder mCurrentReports = new ReportBatchBuilder();

final.

@@ +53,5 @@
> +    private ReportBatch mCurrentReportsSendBuffer;
> +    private ReportBatchIterator mReportBatchIterator;
> +    private StorageIsEmptyTracker mTracker;
> +    private ReportFileList mFileList;
> +    private Timer mFlushMemoryBuffersToDiskTimer;

These five items are the ones that are mutated as part of ordinary operation. Let's see them broken out by whitespace for clarity.

@@ +54,5 @@
> +    private ReportBatchIterator mReportBatchIterator;
> +    private StorageIsEmptyTracker mTracker;
> +    private ReportFileList mFileList;
> +    private Timer mFlushMemoryBuffersToDiskTimer;
> +    private static DataStorageManager sInstance;

Newline to separate out the static.

@@ +84,5 @@
> +        int wifiCount = mFileList.mWifiCount + mCurrentReports.wifiCount;
> +        int cellCount = mFileList.mCellCount + mCurrentReports.cellCount;
> +        long bytes = 0;
> +
> +        if (mCurrentReports.reports.size() > 0) {

Calculate the size once; see line 83.

@@ +86,5 @@
> +        long bytes = 0;
> +
> +        if (mCurrentReports.reports.size() > 0) {
> +            try {
> +                bytes = Zipper.zipData(finalizeReports(mCurrentReports.reports).getBytes()).length;

If this fails, then presumably reportCount is wrong on line 83, right?

That's fine if the error state concludes with mCurrentReportsSendBuffer == null, but I doubt that's always the case.

Comment 43

[:garvan]

Attachment: prefs_changes__build_and_code.patch

Addressing comment 41

Try: https://tbpl.mozilla.org/?tree=Try&rev=142bd4efa20c
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=142bd4efa20c

Comment 44

[:garvan]

Comment on attachment 8471192 [details] [diff] [review]
prefs_changes__build_and_code.patch

hg unshelve didn't patch in my changes, lets try this again

Comment 45

[:garvan]

Attachment: prefs_changes__build_and_code.patch

Addressed comments

Try is green:
https://tbpl.mozilla.org/?tree=Try&rev=d8fcf05860ac
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d8fcf05860ac
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d8fcf05860ac

Comment 46

[:garvan]

Clear on all except:

> @@ +84,5 @@
> > +        int wifiCount = mFileList.mWifiCount + mCurrentReports.wifiCount;
> > +        int cellCount = mFileList.mCellCount + mCurrentReports.cellCount;
> > +        long bytes = 0;
> > +
> > +        if (mCurrentReports.reports.size() > 0) {
> 
> Calculate the size once; see line 83.

Line 83 is a different value: the current in-memory report count plus those on disk (which is used a report count as opposed to a byte count). This is checking just the in-memory current reports is non-empty, and if so, get the byte count.

> @@ +86,5 @@
> > +        long bytes = 0;
> > +
> > +        if (mCurrentReports.reports.size() > 0) {
> > +            try {
> > +                bytes = Zipper.zipData(finalizeReports(mCurrentReports.reports).getBytes()).length;
> 
> If this fails, then presumably reportCount is wrong on line 83, right?

Do you mean if gzip operation throws? Probably that would be in low-mem conditions. This func is an annoying necessity for GUI reporting, not used by service -maybe i should doc that. I don't rethrow, because the client GUI will just eat the exception anyway, but more because if the Android SDK can't do a zip op, the service will have much bigger problems.

> That's fine if the error state concludes with mCurrentReportsSendBuffer ==
> null, but I doubt that's always the case.

Not sure what you mean. The zip operation is on the mCurrentReports.

Comment 47

[:garvan]

Attachment: service_java_code_to_92842d4.patch

Comment 42 review items addressed.

Comment 48

[Richard Newman [:rnewman]]

Comment on attachment 8471334 [details] [diff] [review]
service_java_code_to_92842d4.patch

Review of attachment 8471334 [details] [diff] [review]:
-----------------------------------------------------------------

After this review I'm happy with StumblerService and DataStorageManager. Will blitz the rest briefly this afternoon. I'm on half-PTO today.

> Do you mean if gzip operation throws?

finalizeReports can throw (OOM). Zipper can fail similarly.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/StumblerService.java
@@ +64,5 @@
> +        if (mScanManager.isScanning()) {
> +            return;
> +        }
> +
> +        mScanManager.startScanning(this);

No point in the check on 64, if mScanManager knows to not start twice. Which it should.

@@ +69,5 @@
> +    }
> +
> +    public void stopScanning() {
> +        if (mScanManager.isScanning()) {
> +            mScanManager.stopScanning();

I prefer the return value version of this:

  if (mScanManager.stopScanning()) {
      mReporter.flush();
      ...
  }

@@ +192,5 @@
> +        init();
> +
> +        if (intent.getBooleanExtra(ACTION_START_PASSIVE, false) == false) {
> +            stopSelf();
> +        } else {

return;, kill the else.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/datahandling/DataStorageManager.java
@@ +168,5 @@
> +        }
> +    }
> +
> +    private static class ReportBatchBuilder {
> +        public ArrayList<String> reports = new ArrayList<String>();

final

@@ +196,5 @@
> +
> +        final SQLiteDatabase db = SQLiteDatabase.openDatabase(dbFile.toString(), null, 0);
> +        final Cursor cursor = db.rawQuery("select * from stats", null);
> +        if (cursor == null) {
> +            return null;

This will leak the DB.

You need nested try blocks here -- one for the DB, one for the cursor.

@@ +199,5 @@
> +        if (cursor == null) {
> +            return null;
> +        }
> +        try {
> +            cursor.moveToFirst();

if (!cursor.moveToFirst()) {
    return;
}

@@ +241,5 @@
> +    public static void createGlobalInstance(Context context, StorageIsEmptyTracker tracker) {
> +        if (sInstance != null) {
> +            return;
> +        }
> +        sInstance =  new DataStorageManager(context, tracker);

Double space.

@@ +335,5 @@
> +        mReportBatchIterator = new ReportBatchIterator(mFileList);
> +
> +        if (mCurrentReports.reports.size() > 0) {
> +            String filename = MEMORY_BUFFER_NAME;
> +            int reportCount = mCurrentReports.reports.size();

Three uses of mCurrentReports.reports.size() here.

@@ +381,5 @@
> +                      SEP_REPORT_COUNT + reportCount +
> +                      SEP_WIFI_COUNT + wifiCount +
> +                      SEP_CELL_COUNT + cellCount + ".gz";
> +        File file = new File(mReportsDir, name);
> +        return file;

Inline file.

@@ +423,5 @@
> +            fos = new FileOutputStream(createFile(reportCount, wifiCount, cellCount));
> +            fos.write(bytes);
> +        } finally {
> +            if (fos != null) {
> +                fos.close();

Fix up this block to avoid the null check, as you do in writeSyncStats.

@@ +434,5 @@
> +        final String kPrefix = "{\"items\":[";
> +        final String kSuffix = "]}";
> +
> +        StringBuilder sb = new StringBuilder();
> +        sb.append(kPrefix);

final, and init the StringBuilder with the prefix.

@@ +436,5 @@
> +
> +        StringBuilder sb = new StringBuilder();
> +        sb.append(kPrefix);
> +        String sep = "";
> +        String separator = ",";

final.

@@ +439,5 @@
> +        String sep = "";
> +        String separator = ",";
> +        for(String s: reports) {
> +            sb.append(sep).append(s);
> +            sep = separator;

Ooh, that's a SB join technique I've never seen before. Neat.

@@ +443,5 @@
> +            sep = separator;
> +        }
> +
> +        String result = sb.append(kSuffix).toString();
> +        Log.d(LOG_TAG, result);

Wrap in your debug condition.

@@ +483,5 @@
> +                public void run() {
> +                    try {
> +                        saveCurrentReportsToDisk();
> +                    } catch (IOException ex) {
> +                        Log.e(LOG_TAG, "mFlushMemoryBuffersToDiskTimer exception", ex);

I'd be inclined to do "exception: " + ex, simply avoid an error state resulting in spamming of stack traces to the log. Or guard with isDebug.

@@ +500,5 @@
> +        try {
> +            Properties props = new Properties();
> +            props.load(input);
> +            return props;
> +        }

Cuddle braces.

@@ +540,5 @@
> +        }
> +    }
> +
> +    public synchronized void deleteAll() {
> +        for (File f : mFileList.mFiles) {

I don't remember if the Java for: syntax handles null safely. If it doesn't, please null check here.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/scanners/ScanManager.java
@@ +25,5 @@
> +public class ScanManager {
> +    private static final String LOG_TAG = AppGlobals.LOG_PREFIX + ScanManager.class.getSimpleName();
> +    private Timer mPassiveModeFlushTimer;
> +    private Context mContext;
> +    private boolean mIsScanning;

This class can only safely be accessed from a single thread. Make sure that's the case, or make it thread-safe.

@@ +92,5 @@
> +        return ActiveOrPassiveStumbling.PASSIVE_STUMBLING == mStumblingMode;
> +    }
> +
> +    public void startScanning(Context context) {
> +        mContext = context;

Consider doing context.getApplicationContext() here.

@@ +100,5 @@
> +            mCellScanner = new CellScanner(context);
> +        }
> +
> +        if (mIsScanning) {
> +            return;

Move to the start of the method, log.

@@ +117,5 @@
> +    }
> +
> +    public void stopScanning() {
> +        if (!mIsScanning) {
> +            return;

Log.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/sync/UploadAlarmReceiver.java
@@ +57,5 @@
> +            if (oldestMs > 0) {
> +                long currentTime = System.currentTimeMillis();
> +                long msPerWeek = 604800 * 1000;
> +                if (currentTime - oldestMs > maxWeeks * msPerWeek) {
> +                    DataStorageManager.getInstance().deleteAll();

This can throw (because filesystem).

Comment 49

[:garvan]

All is good, updating patch now.

> > Do you mean if gzip operation throws?
> 
> finalizeReports can throw (OOM). Zipper can fail similarly.

Uh oh. OOM handling is a nasty can to open. Is there a recommended handling of this in Fennec? These are 5 KB blobs, I figured this wouldn't need special OOM consideration.
 
> @@ +196,5 @@
> > +
> > +        final SQLiteDatabase db = SQLiteDatabase.openDatabase(dbFile.toString(), null, 0);
> > +        final Cursor cursor = db.rawQuery("select * from stats", null);
> > +        if (cursor == null) {
> > +            return null;

Thanks for pointing out this code, this DB migration function is gone now, it was MozStumbler client specific code, moved it out of service -and fixed your comments :).

> > +    public synchronized void deleteAll() {
> > +        for (File f : mFileList.mFiles) { 
> I don't remember if the Java for: syntax handles null safely. If it doesn't,
> please null check here.

Good catch, it doesn't.

> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/sync/
> UploadAlarmReceiver.java
> @@ +57,5 @@
> > +            if (oldestMs > 0) {
> > +                long currentTime = System.currentTimeMillis();
> > +                long msPerWeek = 604800 * 1000;
> > +                if (currentTime - oldestMs > maxWeeks * msPerWeek) {
> > +                    DataStorageManager.getInstance().deleteAll();
> 
> This can throw (because filesystem).

File.delete() is a no-throw on android:
http://developer.android.com/reference/java/io/File.html#delete()

Comment 50

[:garvan]

Attachment: service_java_code_to_58eac46.patch

Stumbler service code, updated for Comment 48

Comment 51

[Richard Newman [:rnewman]]

> Uh oh. OOM handling is a nasty can to open. Is there a recommended handling
> of this in Fennec? These are 5 KB blobs, I figured this wouldn't need
> special OOM consideration.

If it's a place that an Error being thrown would be expected, or bad, then it's best to either `finally` important stuff, or catch OutOfMemoryError.

> > This can throw (because filesystem).
> 
> File.delete() is a no-throw on android:
> http://developer.android.com/reference/java/io/File.html#delete()

Heh, I made a mental note to delete that comment in my second pass, and never did!

Comment 52

[Richard Newman [:rnewman]]

Comment on attachment 8471835 [details] [diff] [review]
service_java_code_to_58eac46.patch

Review of attachment 8471835 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/datahandling/DataStorageManager.java
@@ +28,5 @@
> + *
> + * The tricky bit is the mCurrentReportsSendBuffer. When the uploader code begins accessing the
> + * report batches, mCurrentReports gets pushed to mCurrentReportsSendBuffer.
> + * The mCurrentReports is then cleared, and can continue receiving new reports.
> + * From the uploader perspective, mCurrentReportsSendBuffer looks and acts exactly like a batch file on disk.

Is it possible for us to touch mCurrentReportsSendBuffer during an upload? For example, flushing to disk nulls it out.

@@ +188,5 @@
> +        if (AppGlobals.isDebug) {
> +            // in debug, put files in public location
> +            dir = c.getExternalFilesDir(null);
> +            if (dir != null) {
> +                dir = new File(dir.getAbsolutePath() + "/mozstumbler");

new File(dir, "mozstumbler")?

@@ +193,5 @@
> +            }
> +
> +            if (!dir.exists()) {
> +                boolean ok = dir.mkdirs();
> +                assert(ok);

Not sure what this assertion is buying us. Either we succeeded, and can proceed, or we didn't, and can't -- the next attempt to write into the non-created directory is going to fail. I'd suggest throwing IllegalStateException here, and higher up the stack let's turn ourselves off.

@@ +293,5 @@
> +            final String filename = MEMORY_BUFFER_NAME;
> +            final byte[] data = Zipper.zipData(finalizeReports(mCurrentReports.reports).getBytes());
> +            final int wifiCount = mCurrentReports.wifiCount;
> +            final int cellCount = mCurrentReports.cellCount;
> +            clearCurrentReports();

My contention: finalizing and zipping are the most likely place to OOM; peak memory usage due to building a big string and then trying to zip it up. If we OOM, we might well do so again -- we're only going to accrue more reports, right?

And an error in a background service manifests as a visible crash. Not good.

So let's wrap 294-296 in catch (OutOfMemoryError) { ... } finally { clearCurrentReports(); }, so that we discard the current reports. "..." should be "do whatever makes sense and return something".

@@ +314,5 @@
> +        }
> +
> +        mReportBatchIterator.currentIndex++;
> +        if (mReportBatchIterator.currentIndex < 0 ||
> +            mReportBatchIterator.currentIndex > mReportBatchIterator.fileList.mFiles.length - 1) {

>= and no arithmetic?

@@ +330,5 @@
> +
> +    private File createFile(int reportCount, int wifiCount, int cellCount) {
> +        final long time = System.currentTimeMillis();
> +        final String name = FILENAME_PREFIX +
> +                      SEP_TIME_MS + time +

Prefer consistent indenting: "S" under "F".

@@ +337,5 @@
> +                      SEP_CELL_COUNT + cellCount + ".gz";
> +        return new File(mReportsDir, name);
> +    }
> +
> +    public synchronized long getOldestBatchTimeMs() {

Prefer "Msec" for consistency.

Consider whether there are ramifications here of clocks jumping around. This value might be 1980, it might be 2017.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/sync/AsyncUploader.java
@@ +23,5 @@
> +public class AsyncUploader extends AsyncTask<Void, Void, SyncSummary> {
> +    private static final String LOG_TAG = AppGlobals.LOG_PREFIX + AsyncUploader.class.getSimpleName();
> +    private final UploadSettings mSettings;
> +    private final Object mListenerLock = new Object();
> +    private AsyncUploaderListener mListener;

final?

@@ +45,5 @@
> +        mListener = listener;
> +        mSettings = settings;
> +    }
> +
> +    public void clearListener() {

This is never called in this patch set, and seems like a bit of an anti-pattern. It stops you making mListener final, for one thing. Can you explain why you need it?

Comment 53

[Richard Newman [:rnewman]]

Comment on attachment 8471835 [details] [diff] [review]
service_java_code_to_58eac46.patch

Review of attachment 8471835 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/StumblerService.java
@@ +72,5 @@
> +            }
> +        }
> +    }
> +
> +    public void setWifiBlockList(WifiBlockListInterface list) {

This, and other related methods, are never called. Intentional?

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/datahandling/StumblerBundle.java
@@ +107,5 @@
> +    public Map<String, CellInfo> getCellData() {
> +        return mCellData;
> +    }
> +
> +    public JSONObject  toMLSJSON() throws JSONException {

Nit: redundant double spaces. Might be worth doing some kind of find and replace:

  :%s,\([^s])\s\+,\1 ,g

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/datahandling/StumblerBundleReceiver.java
@@ +27,5 @@
> +        } catch (JSONException e) {
> +            Log.w(LOG_TAG, "Failed to convert bundle to JSON: " + e);
> +            return;
> +        }
> +            if (AppGlobals.isDebug) {

Indenting in this method is wonky.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/scanners/GPSScanner.java
@@ +50,5 @@
> +    private LocationBlockList mBlockList = new LocationBlockList();
> +    private boolean mAutoGeofencing;
> +    private boolean mIsPassiveMode;
> +
> +    private ScanManager mScanManager;

final

@@ +67,5 @@
> +        }
> +    }
> +
> +    private void startPassiveMode() {
> +        LocationManager locationManager = (LocationManager)mContext.getSystemService(Context.LOCATION_SERVICE);

Nit: space between cast and expression (throughout).

@@ +92,5 @@
> +                        int fixes = 0;
> +
> +                        for (GpsSatellite sat : sats) {
> +                            satellites++;
> +                            if(sat.usedInFix()) {

Nit: space between if and (.

@@ +96,5 @@
> +                            if(sat.usedInFix()) {
> +                                fixes++;
> +                            }
> +                        }
> +                        reportNewGpsStatus(fixes,satellites);

Nit: space after comma.

@@ +108,5 @@
> +                    } else if (event == GpsStatus.GPS_EVENT_STOPPED) {
> +                        reportLocationLost();
> +                    }
> +                }
> +            };

Shift this whole block 86-112 left four spaces.

@@ +113,5 @@
> +
> +        lm.addGpsStatusListener(mGPSListener);
> +    }
> +
> +    public void stop() {

Which thread is this being called from?

@@ +141,5 @@
> +        return mLocation;
> +    }
> +
> +    public void checkPrefs() {
> +        if (mBlockList!=null) mBlockList.update_blocks();

Multiline, whitespace around !=.

@@ +147,5 @@
> +        mAutoGeofencing = Prefs.getInstance().getGeofenceHere();
> +    }
> +
> +    public boolean isGeofenced() {
> +        return (mBlockList != null) && mBlockList.isGeofenced();

Multiline.

@@ +175,5 @@
> +        // Check dist and time threshold here, not set on the listener.
> +        if (mIsPassiveMode &&
> +            (location.distanceTo(mLocation) < PASSIVE_GEO_MIN_UPDATE_DISTANCE ||
> +             location.getTime() - mLocation.getTime() < PASSIVE_GEO_MIN_UPDATE_TIME))
> +        {

{ on same line

@@ +176,5 @@
> +        if (mIsPassiveMode &&
> +            (location.distanceTo(mLocation) < PASSIVE_GEO_MIN_UPDATE_DISTANCE ||
> +             location.getTime() - mLocation.getTime() < PASSIVE_GEO_MIN_UPDATE_TIME))
> +        {
> +            //if (SharedConstants.isDebug) Log.d(LOG_TAG, "Distance/time below threshold:" + (location.getTime() - mLocation.getTime()));

Delete or fix up.

@@ +180,5 @@
> +            //if (SharedConstants.isDebug) Log.d(LOG_TAG, "Distance/time below threshold:" + (location.getTime() - mLocation.getTime()));
> +            return;
> +        }
> +
> +        java.util.Date date = new java.util.Date(location.getTime());

Import Date?

@@ +223,5 @@
> +
> +    @Override
> +    public void onStatusChanged(String provider, int status, Bundle extras) {
> +        if ((status != LocationProvider.AVAILABLE)
> +                && (LocationManager.GPS_PROVIDER.equals(provider))) {

&& at end of line, align.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/scanners/LocationBlockList.java
@@ +17,5 @@
> +    private static final double MAX_ALTITUDE    = 8848;      // Mount Everest's altitude in meters
> +    private static final double MIN_ALTITUDE    = -418;      // Dead Sea's altitude in meters
> +    private static final float  MAX_SPEED       = 340.29f;   // Mach 1 in meters/second
> +    private static final float  MIN_ACCURACY    = 500;       // meter radius
> +    private static final long   MIN_TIMESTAMP   = 946684801; // 2000-01-01 00:00:01

Why not the build date minus two weeks? Or now minus two weeks?

@@ +28,5 @@
> +    public LocationBlockList() {
> +        update_blocks();
> +    }
> +
> +    public void update_blocks()    {

Naming convention.

@@ +88,5 @@
> +        }
> +
> +        if (mGeofencingEnabled &&
> +                Math.abs(location.getLatitude() - mBlockedLocation.getLatitude()) < GEOFENCE_RADIUS &&
> +                Math.abs(location.getLongitude() - mBlockedLocation.getLongitude()) < GEOFENCE_RADIUS) {

Indenting.

@@ +91,5 @@
> +                Math.abs(location.getLatitude() - mBlockedLocation.getLatitude()) < GEOFENCE_RADIUS &&
> +                Math.abs(location.getLongitude() - mBlockedLocation.getLongitude()) < GEOFENCE_RADIUS) {
> +            block = true;
> +            mIsGeofenced = true;
> +            Log.i(LOG_TAG, "Hit the geofence: " + mBlockedLocation.getLatitude() +" / " + mBlockedLocation.getLongitude());

Watch logspam.

@@ +97,5 @@
> +
> +        return block;
> +    }
> +
> +    public boolean isGeofenced() { return mIsGeofenced; }

Multiline.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/scanners/WifiScanner.java
@@ +40,5 @@
> +    public static final int STATUS_ACTIVE = 1;
> +    public static final int STATUS_WIFI_DISABLED = -1;
> +
> +    private static final String LOG_TAG = AppGlobals.LOG_PREFIX + WifiScanner.class.getSimpleName();
> +    private static final long WIFI_MIN_UPDATE_TIME = 5000; // milliseconds

_MSEC

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/scanners/cellscanner/CellInfo.java
@@ +17,5 @@
> +import org.json.JSONException;
> +import org.json.JSONObject;
> +import org.mozilla.mozstumbler.service.AppGlobals;
> +
> +public class CellInfo implements Parcelable {

You have to be super careful with this. It's a mutable record. Threading with these is hard.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/sync/AsyncUploader.java
@@ +17,5 @@
> +* it will return immediately, and SyncResult is null.
> +*
> +* Threading:
> +* Uploads on a separate thread. ONLY DataStorageManager is thread-safe, do not call
> +* preferences, do not call any code that isn't thread-safe. You will cause suffering.

Submitter touches prefs, and you instantiate one in uploadReports.

 public Submitter() {
     super();
     mNickname = Prefs.getInstance().getNickname();
 }

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/sync/Submitter.java
@@ +11,5 @@
> +import org.mozilla.mozstumbler.service.AppGlobals;
> +import org.mozilla.mozstumbler.service.Prefs;
> +
> +public class Submitter extends AbstractCommunicator {
> +    private static final String SUBMIT_URL = "https://location.services.mozilla.com/v1/submit";

This reminds me: mention stumbler in Bug 507641.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/sync/UploadAlarmReceiver.java
@@ +38,5 @@
> +        }
> +
> +        @Override
> +        protected void onHandleIntent(Intent intent) {
> +            boolean isRepeating = intent.getBooleanExtra(EXTRA_IS_REPEATING, true);

final

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/DateTimeUtils.java
@@ +15,5 @@
> +public final class DateTimeUtils {
> +    private static final DateFormat sLocaleFormat = DateFormat.getDateTimeInstance(DateFormat.SHORT, DateFormat.SHORT);
> +    private static final DateFormat sISO8601Format = new SimpleDateFormat("yyyy-MM-dd", Locale.US);
> +
> +    public static final long MILLISECONDS_PER_DAY = 86400000;  // milliseconds/day

Remove comment.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/Zipper.java
@@ +13,5 @@
> +        final ByteArrayOutputStream os = new ByteArrayOutputStream();
> +        GZIPOutputStream gstream = null;
> +        byte[] output;
> +        try {
> +            gstream = new GZIPOutputStream(os);

Fix this pattern.

@@ +32,5 @@
> +        GZIPInputStream gstream = null;
> +        String result = "";
> +
> +        try {
> +            gstream = new GZIPInputStream(bs);

Same.

Comment 54

[:garvan]

Will update code now. Think all is clear.

> > +    public void setWifiBlockList(WifiBlockListInterface list) {
> 
> This, and other related methods, are never called. Intentional?

Yeah, not called in the Fennec context. 
 
> :::
> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/datahandling/
> StumblerBundle.java
> @@ +107,5 @@
> > +    public Map<String, CellInfo> getCellData() {
> > +        return mCellData;
> > +    }
> > +
> > +    public JSONObject  toMLSJSON() throws JSONException {
> 
> Nit: redundant double spaces. Might be worth doing some kind of find and
> replace:
> 
>   :%s,\([^s])\s\+,\1 ,g

In general, there was no strict coding standard over the last year on this code, so there is a lot of formatting issues. I basically left them, not realizing that was within the scope of getting this into Fennec. I do appreciate you catching this stuff, I'll try this afternoon to go over the service code for this and any other formatting inconsistencies.

> @@ +113,5 @@
> > +
> > +        lm.addGpsStatusListener(mGPSListener);
> > +    }
> > +
> > +    public void stop() {
> 
> Which thread is this being called from?

Thread Summary:
Calls from the main thread happen on the PassiveServiceReceiver, which starts the service on its own thread, everything collection-wise happens synchronously on this thread. The only other thread is for uploading, which communicates only with the DataSourceManager which is (hopefully!) properly synchronized.

Red flag for me. If you aren't seeing this clearly, it means when this code goes back to community contribution, they will not see this for sure, and break something. 
 
> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/scanners/
> LocationBlockList.java
> @@ +17,5 @@
> > +    private static final double MAX_ALTITUDE    = 8848;      // Mount Everest's altitude in meters
> > +    private static final double MIN_ALTITUDE    = -418;      // Dead Sea's altitude in meters
> > +    private static final float  MAX_SPEED       = 340.29f;   // Mach 1 in meters/second
> > +    private static final float  MIN_ACCURACY    = 500;       // meter radius
> > +    private static final long   MIN_TIMESTAMP   = 946684801; // 2000-01-01 00:00:01
> 
> Why not the build date minus two weeks? Or now minus two weeks?

Code predates my involvement, I suspect this is a case of "it doesn't matter", as bad timestamps would be wildly wrong. 
 
....

I should mention, any geofencing code is unused. I'll just clean up the formatting. I have questions about the logic and implementation of that feature that I left as out-of-scope for now, as it will take time for me to get the answers, and the code is unused on Fennec.

Comment 55

[Richard Newman [:rnewman]]

> Yeah, not called in the Fennec context.

Whenever possible, either lift this kind of stuff out or rearrange to minimize the surface area for bugs in Fennec. If that's not possible, then try to minimize access modifiers (e.g., package-scoped) to encourage optimizers like ProGuard to strip dead code.

There will be a constant pressure to reduce the impact (bugs, perf, APK size) of stumbler, just as there is the rest of Fennec.

Large reorg is probably out of scope for right now, but we can at least move things into two conceptual sections and comment accordingly.

 
> > > +    public void stop() {
> > 
> > Which thread is this being called from?
> 
> Thread Summary:
> Calls from the main thread happen on the PassiveServiceReceiver, which
> starts the service on its own thread...

Concurrency analysis is hard. My main approach to handling that is to get as much help as possible in reducing the surface area: pushing for things to be final, commenting and adding explicit synchronization, adding assertions.

In this case, we have a public method that accesses unprotected members; there's nothing at all to stop a patch tomorrow from calling `stop` from some random runnable.

It might seem redundant to add a concurrency description to every non-trivial file (and "trivial" is an odd concept when talking about concurrency -- only immutable classes are trivial!), but someone reading this file won't be hopping back to this bug to figure it out.

Give them a hand by describing exactly when sets of methods will be called, and ideally providing some code support to make sure those assumptions aren't violated.

Again, reducing the number of public methods will help. private methods mean you only need to read this file. protected and package similarly give you a reduced space to study. public means terror.


> Code predates my involvement, I suspect this is a case of "it doesn't
> matter", as bad timestamps would be wildly wrong. 

Clocks on Android are wrong all the time. How we handle that is up to you; you understand the domain better than I do.

http://opensignal.com/reports/timestamps/


> I should mention, any geofencing code is unused. I'll just clean up the
> formatting. I have questions about the logic and implementation of that
> feature that I left as out-of-scope for now, as it will take time for me to
> get the answers, and the code is unused on Fennec.

If it's unused, we should probably consider ways of eliminating that code from Fennec. Again, it'll help prevent bugs. You might consider having stub files in Fennec, and full-featured versions of those in the independent stumbler repo.

Comment 56

[:garvan]

I'll respond in general to Comment 55. This library has more than one use. I have been aggressive about pulling non-Fennec code out where I can, I am not planning to pull any more code. Meaning, either I have to change my plans, or we accept that -as any 3rd party lib will have- there are some functions or code paths that aren't used. I think at this point we have identified them all, I can add comments explicitly in those few places that indicates the code is not called on Fennec.

Comment 57

[Richard Newman [:rnewman]]

I think my more general point is: it's fine to have stuff in the library that isn't used by every consumer, but having it mixed together inside classes isn't as good as having it separated -- whether that's textually or through OOP techniques.

The ideal thing from my POV is for MozStumbler's use of the library code, and Fennec's use, to involve inheritance, delegation, and composition to impose app-specific logic. Right now "library code == MozStumbler", which isn't the same, and is what I'm poking at.

This is especially important if the behavior changes because of different code paths that consumers take -- it's really scary to have only comments guarding stuff that's implicitly called with subtly different threading and lifecycle models depending on its consumer. It means I might well have made errors in my analysis, and that MozStumbler works does not mean that this will work in Fennec. Things will be less scary if they're in a well-guarded and analyzed library, and then called in separate places by well-analyzed consumers.

Being able to ProGuard stuff out is a sop until we're able to tease these things apart, but it's a sop I'd be happy to see! Fennec's APK size is one of the primary adoption problems we're working on, so every little bit is appreciated.

Comment 58

[:garvan]

(In reply to (Away Aug 14-24) Richard Newman [:rnewman] from comment #57)
> I think my more general point is: it's fine to have stuff in the library
> that isn't used by every consumer, but having it mixed together inside
> classes isn't as good as having it separated -- whether that's textually or
> through OOP techniques.

Agreed, but some of this stuff (geofencing, blocklisting) is just waiting to get refactored, retested, and turned on in later Fennec releases. The stuff that remains was there because any use of the stumbler lib would want this. However, I won't argue because looking at it again, I see I can pull it out without too much pain. 

> guarding stuff that's implicitly called with subtly different threading and
> lifecycle models depending on its consumer. It means I might well have made
> errors in my analysis, and that MozStumbler works does not mean that this
> will work in Fennec. 

I think we agree that concurrency is the big risk in this code. If there is a bug or a crash, it will be there that it happens. Let me take a look at how I can move things around to really hammer home the division of labour among threads by using packaging.

Comment 59

[:garvan]

Richard, if you aren't on a beach yet (or can type without getting sand in the keys), I would value your quick feedback on this code organization idea:
https://www.dropbox.com/s/hq2k00z1nhqs1h1/Screenshot%202014-08-13%2023.58.25.png

Providing a top-level reorg by thread: 
Maybe it is a non-standard, but it is a valuable concept to express, and looking at it, it represents accurately my mental map of the code.

Note:
- Prefs is synchronized now, although I still removed any access from the upload thread (it wasn't needed). The underlying SharedPreferences is thread-safe, but as you know this wrapper class was not.
- AsyncUploader now contains the private Submitter class (in case you were wondering what happened to that calls).
- Thread crossover classes like UploadAlarmReceiver is placed in the "uploadthread", it starts the uploadthread, but is technically scheduled from "stumblerthread" and receives notification from the main thread. I think a brief comment on the class would clarify its placement.

Comment 60

[:garvan]

Attachment: service_java_code_to_4ae95d7.patch

Nick marking this for your review, which I am not sure is proper, as Richard and I have been back on forth on this. He completed the code review -at least in terms of having reviewed every file in the project. His last review is addressed with this PR. 
Richard identified a number of potential concurrency footguns for future contributors on this code. My (hopefully final) effort at code clarity has been to repackage by thread instead of by functional category (i.e. package 'uploadthread' contains classes dedicated to that thread).   

The other 2 build-related patches need to be updated as I moved things around.

Comment 61

[:garvan]

Comment on attachment 8471270 [details] [diff] [review]
prefs_changes__build_and_code.patch

Review of attachment 8471270 [details] [diff] [review]:
-----------------------------------------------------------------

In comment 60 I mentioned this patch would need updating because of a code update, that is incorrect, this patch requires no changes.

::: mobile/android/base/BrowserApp.java
@@ +1373,5 @@
>  
> +                if (AppConstants.MOZ_STUMBLER_ENABLED) {
> +                    // Start (this acts as ping if started already) the stumbler lib; if the stumbler has queued data it will upload it.
> +                    // Stumbler operates on its own thread, and startup impact is further minimized by delaying work (such as upload) a few seconds.
> +                    GeckoPreferences.broadcastStumblerPref(this);

I preferred

Comment 62

[:garvan]

Attachment: stumbler_manifest_and_mozbuild.patch

Due to repackaging of the java files, this required updating.

Comment 63

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8471270 [details] [diff] [review]
prefs_changes__build_and_code.patch

Review of attachment 8471270 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but the commit message needs some work.  Try:

Bug 1038843 - Part XXX: Broadcast enabled pref and MOZ_MOZILLA_API_KEY to stumbler. r=nalexander

Then leave a blank line, and describe your changes in more detail, with full sentences.

::: mobile/android/base/AppConstants.java.in
@@ +98,5 @@
> +    "@MOZ_STUMBLER_API_KEY@";
> +#else
> +    null;
> +#endif
> +    public static final boolean MOZ_STUMBLER_ENABLED = (MOZ_STUMBLER_API_KEY != null);

Can we make it clear that this is build time?

::: mobile/android/base/BrowserApp.java
@@ +1373,5 @@
>  
> +                if (AppConstants.MOZ_STUMBLER_ENABLED) {
> +                    // Start (this acts as ping if started already) the stumbler lib; if the stumbler has queued data it will upload it.
> +                    // Stumbler operates on its own thread, and startup impact is further minimized by delaying work (such as upload) a few seconds.
> +                    GeckoPreferences.broadcastStumblerPref(this);

Please comment that MOZ_STUMBLER_ENABLED is a *build* flag, not the runtime preference (in both locations).

Comment 64

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8473066 [details] [diff] [review]
stumbler_manifest_and_mozbuild.patch

Review of attachment 8473066 [details] [diff] [review]:
-----------------------------------------------------------------

Again, update the commit messag to be short and r=nalexander.

You can update/rework this as much as you need before landing.  You own it!

::: mobile/android/stumbler/manifests/StumblerManifest_services.xml.in
@@ +2,5 @@
> +  android:name="org.mozilla.mozstumbler.service.stumblerthread.StumblerService"
> +  android:label="stumbler">
> +</service>
> +
> +<receiver android:name="org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver" />

We have generally put receivers in an _activities.xml.in file, but I don't really care.  (It matters for _permissions.xml.in due to the XML structure.)

We have been pretty consistent about using 4 space indentation; if you have time, update for consistency.

::: mobile/android/stumbler/stumbler_sources.mozbuild
@@ -4,5 @@
>  # 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/.
>  
>  stumbler_sources = [
> -    'java/org/mozilla/mozstumbler/PlaceHolder.java',

Make sure to |hg rm PlaceHolder.java|.

Comment 65

[Nick Alexander :nalexander [he/him]]

(In reply to Garvan Keeley [:garvank] from comment #60)
> Created attachment 8473041 [details] [diff] [review]
> service_java_code_to_4ae95d7.patch
> 
> Nick marking this for your review, which I am not sure is proper, as Richard
> and I have been back on forth on this. He completed the code review -at
> least in terms of having reviewed every file in the project. His last review
> is addressed with this PR. 

Proper?  Who cares.  I'll review by EOD tomorrow.

> Richard identified a number of potential concurrency footguns for future
> contributors on this code. My (hopefully final) effort at code clarity has
> been to repackage by thread instead of by functional category (i.e. package
> 'uploadthread' contains classes dedicated to that thread).   

I see this.  I'll read tomorrow.

> The other 2 build-related patches need to be updated as I moved things
> around.

These look good; do whatever is necessary to make it work, no need for re-review.  (Just make sure you test turning on and off in Fennec itself; and that build time disable works as usual.  A try build is a good thing.)

Comment 66

[:garvan]

Comment on attachment 8473041 [details] [diff] [review]
service_java_code_to_4ae95d7.patch

I love when I upload an empty change set, attn: order of operations

Comment 67

[:garvan]

Attachment: Step 1: service_java_code_to_4ae95d7.patch

This represents the final (planned) code review changes. 

Patch description:
This revision has a repackaging of files per-thread. An ongoing concern during the review is that the entire code base is not thread-safe, only classes that call across threads. Thus, it is critical that the reader can see where these divisions are, and not break the code by crossing the thread division (calling a different thread package) in an unsynchronized fashion.
Fortunately, packaging by thread in this code is the same as packaging by area of responsibility, and the latter logic is not obscured by the packaging change.

Comment 68

[:garvan]

Attachment: Step 2: Prefs support for checking stumbler enabled and starting service

Updated for :nalexander's comments, carrying forward the r+

Comment 69

[:garvan]

Attachment: Part 3: stumbler_manifest_and_mozbuild.patch

Carrying over r+ from comment 64. Updated the commit message. 

(I like seeing all the services-related manifest items in that one file, it is small, and the receivers are highly coupled to the service.)

Comment 70

[:garvan]

Attachment: Step 4: turn on in nightly only: confvars.patch

In this patch: ifdef nightly, turn on stumbler.

Comment 71

[:garvan]

Attachment: Step 5: eclipse integration.patch

Updated the eclipse integration. I can't test, eclipse integration doesn't work for me in general. Actually, I haven't tried in a few weeks, I'll try it again, I would like to get it working.

Comment 72

[:garvan]

You can view the progress of your build at the following URL:
remote:   https://tbpl.mozilla.org/?tree=Try&rev=791e2552a050
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=791e2552a050

Adding ni to remind me to check back when complete

Comment 73

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8473877 [details] [diff] [review]
Step 5: eclipse integration.patch

This commit isn't needed; the FennecStumbler vs. stumbler is just for Android Eclipse project names, and the change to the package_name won't do anything since the stumbler accesses no resources.

Comment 74

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8473875 [details] [diff] [review]
Step 4: turn on in nightly only: confvars.patch

Review of attachment 8473875 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, but I'd prefer to have a separate "fire-zee-missiles!!!" bug for enabling on Nightly.  We don't want to back out everything in the face of problems if we can help it.

Comment 75

[:garvan]

Comment on attachment 8473877 [details] [diff] [review]
Step 5: eclipse integration.patch

Not needed, one less piece to worry about.

Comment 76

[:garvan]

Comment on attachment 8473875 [details] [diff] [review]
Step 4: turn on in nightly only: confvars.patch

comment 75: here is bug for on-switch 
https://bugzilla.mozilla.org/show_bug.cgi?id=1054520
This patch is moved there, carried over r+.

Also, comment 72: try is green.

Comment 77

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8473816 [details] [diff] [review]
Step 1: service_java_code_to_4ae95d7.patch

Review of attachment 8473816 [details] [diff] [review]:
-----------------------------------------------------------------

OK, now we have 3 cooks in the kitchen, as well as all the others who contributed to the code base.  I tried to focus on the interaction with Fennec, concurrency issues, and the service lifecycles.  I think I found some concurrency issues -- small ones -- but I spent two hours reading this and am still not clear on the lifecycle of the services.

I'm going to post these review comments whilst I think about further comments.  Sorry to not complete this by EOD.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/mainthread/PassiveServiceReceiver.java
@@ +13,5 @@
> +import org.mozilla.mozstumbler.service.stumblerthread.StumblerService;
> +
> +/* Starts the stumblerthread.StumblerService.
> + * Registered as a receiver in manifest. Starts the StumblerService in passive listening mode.
> + * Using GPS_* event changes during development, switch to using the existing permissions for a

The tense of "using" is misleading.  Do you mean, "while this code was under development, we used GPS_*"?  Or something else?

@@ +15,5 @@
> +/* Starts the stumblerthread.StumblerService.
> + * Registered as a receiver in manifest. Starts the StumblerService in passive listening mode.
> + * Using GPS_* event changes during development, switch to using the existing permissions for a
> + * service on Fennec.
> + <meta-data name="MOZILLA_API_KEY" value="aValue">

This is either wrong or likely to be wrong very quickly.  I'd prefer a reference to the right place to look for this information, if we care at all.

@@ +28,5 @@
> +    static final String LOG_TAG = AppGlobals.LOG_PREFIX + PassiveServiceReceiver.class.getSimpleName();
> +
> +    @Override
> +    public void onReceive(Context context, Intent intent) {
> +        String action = intent.getAction();

You should verify that your intent is not-null (we see null intents a lot -- it's Android!), and you should verify that it's really the intent you want.  You get kicked by at least 3 intents.

@@ +31,5 @@
> +    public void onReceive(Context context, Intent intent) {
> +        String action = intent.getAction();
> +        String mozApiKey = null;
> +
> +        if (action.contains(".STUMBLER_PREF")) {

I'd be careful that action is non-null too.  It's Android!

@@ +46,5 @@
> +        Log.d(LOG_TAG, "Stumbler: Sending passive start message | isDebug:" + AppGlobals.isDebug);
> +
> +        Intent startServiceIntent = new Intent(context, StumblerService.class);
> +        startServiceIntent.putExtra(StumblerService.ACTION_START_PASSIVE, true);
> +        startServiceIntent.putExtra(StumblerService.ACTION_EXTRA_MOZ_API_KEY, mozApiKey);

This seems racy.  You could get a valid API key from STUMBLER_PREF, and then a null API key from a startup/SD card event.  Are null API keys filtered at the Prefs level?  If so, explain why this works!  Actually, this seems worse than racy -- it's wrong if you get a non-STUMBLER_PREF intent before you hear from Fennec with the API key.  I think the way we handled this in other places was to reflect back to Fennec, asking for the relevant data in a new intent.

So:

BOOT_COMPLETED -> service receiver
service receiver -> starts service
service sees BOOT_COMPLETED -> asks Fennec for new intent (via reflection)
Fennec -> STUMBLER_PREF
STUMBLER_PREF -> service receiver
...

See:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/background/healthreport/HealthReportBroadcastService.java#133

(As an aside, the reflection was because we work in split repositories in background/, like you are doing in stumbler/; and we didn't want android-sync to depend on too much of Fennec.)

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/StumblerService.java
@@ +25,5 @@
> +import org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver;
> +import org.mozilla.mozstumbler.service.utils.NetworkUtils;
> +import org.mozilla.mozstumbler.service.utils.PersistentIntentService;
> +
> +// Created from PassiveServiceReceiver

Explain to me what this is used for and how it interacts with the other Services.

@@ +90,5 @@
> +    public void checkPrefs() {
> +        mScanManager.checkPrefs();
> +    }
> +
> +    public int getLocationCount() {

None of these helpers are used in Fennec.  Are they necessary?

@@ +157,5 @@
> +    @Override
> +    public void onDestroy() {
> +        super.onDestroy();
> +
> +        // Used to move these disk I/O ops off the calling thread. The current operations here are synchronized,

current?  Do you mean "concurrent", or do you mean "The operations currently here...".

@@ +171,5 @@
> +                if (sFirefoxStumblingEnabled.get() == false) {
> +                    Prefs.getInstance().setFirefoxScanEnabled(false);
> +                }
> +
> +                if (DataStorageManager.getInstance() != null) {

This pattern is racy.  Prefer saving to a temporary, then null check and access the temporary copy.  (In this case, I think you're okay, because singleton nonsense.)

@@ +174,5 @@
> +
> +                if (DataStorageManager.getInstance() != null) {
> +                    try {
> +                        DataStorageManager.getInstance().saveCurrentReportsToDisk();
> +                    } catch (IOException ex) {

Is this really the only error you fear?  OOM, bad gravy, anything?  I'd catch Exception and log at least, just to make sure we don't die on a background thread.

@@ +213,5 @@
> +        Prefs.getInstance().setFirefoxScanEnabled(true);
> +
> +        String apiKey = intent.getStringExtra(ACTION_EXTRA_MOZ_API_KEY);
> +        if (apiKey != null) {
> +            Prefs.getInstance().setMozApiKey(apiKey);

What happens if we start scanning below and /never/ see a non-null API key?

@@ +222,5 @@
> +    }
> +
> +    @Override
> +    public IBinder onBind(Intent intent) {
> +        mIsBound = true;

Explain in the class comment what you're doing with the binding lifecycle.  Why would you be unbound?  How would you use the mIsBound flag whilst unbound?  I see a stopScanning method (uncalled in Fennec!) that might care; how is that intended to be invoked?  Also, can you not get the bind state from the service and/or the binder?

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/uploadthread/AsyncUploader.java
@@ +12,5 @@
> +import org.mozilla.mozstumbler.service.AppGlobals;
> +import org.mozilla.mozstumbler.service.stumblerthread.datahandling.DataStorageManager;
> +import org.mozilla.mozstumbler.service.utils.NetworkUtils;
> +
> +/* Only one at a time may be uploading. If executed while another upload is in progress

Why not actually make it so, by making this a singleton and enforcing your invariant?  Because at the moment, you have an unsynchronized static boolean guard that races.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/AbstractCommunicator.java
@@ +17,5 @@
> +import java.net.HttpURLConnection;
> +import java.net.MalformedURLException;
> +import java.net.URL;
> +
> +public abstract class AbstractCommunicator {

This needs a nice class comment explaining what it's intended to accomplish (providing a uniform interface for Fennec and MozStumbler), how it came into being, and why it's not an actual interface.  Why is that, again?

@@ +54,5 @@
> +        // Zero is no error, for HTTP error cases, set this code to the error
> +        public int errorCode = -1;
> +    }
> +
> +    /* Return non-zero for error, http error code if available */

This returns a NetworkSendResult, not an integer.

@@ +57,5 @@
> +
> +    /* Return non-zero for error, http error code if available */
> +    public abstract NetworkSendResult cleanSend(byte[] data);
> +
> +    public String getNickname() {

This looks like it has something to do with MozStumbler?  Why is this here, if it's not used in Fennec?

@@ +69,5 @@
> +    }
> +
> +    private void openConnectionAndSetHeaders() {
> +        try {
> +            if (sMozApiKey == null) {

Shouldn't this do something if the API key is null even after fetching it?  Ah, I see that getMozApiKey() returns a special string in that case.  This special handling would be clearer if you just queried Prefs all the time, and cached (as necessary) over there.  You're passing the API key via the intent, but assuming you'll only get one API key.  That's true now, but fragile.

@@ +101,5 @@
> +        return output;
> +    }
> +
> +    public boolean isCorrectResponse(int httpCode) {
> +        return httpCode/100 == 2;

WAT.  This is a great way to convince people that 200 is all you accept, when actually you'll take 2xx.  I nominate this for pointless trickery of the month.  If you really don't want to change it, rename it "isSuccessResponse" and explain what is happening.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/DateTimeUtils.java
@@ +40,5 @@
> +    public static String formatTime(long time) {
> +        return formatDate(new Date(time));
> +    }
> +
> +    public static String formatTimeForLocale(long time) {

This is unused in Fennec.  Is it really needed?  (Locale-specific stuff is dangerous, 'cuz the system can change locales at will, and we do let the user change locale-like l10n settings that might change this is unpredictable ways (that won't be noticed by this static pattern)).

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/NetworkUtils.java
@@ +12,5 @@
> +
> +public final class NetworkUtils {
> +    private static final String LOG_TAG = AppGlobals.LOG_PREFIX + NetworkUtils.class.getSimpleName();
> +
> +    ConnectivityManager mConnectivityManager;

I know this code pre-dates your involvement, but bare variables are almost certainly wrong.  This can be final, too, if we make a protected constructor.

@@ +16,5 @@
> +    ConnectivityManager mConnectivityManager;
> +    static NetworkUtils sInstance;
> +
> +    /* Created at startup by app, or service, using a context. */
> +    static public void createGlobalInstance(Context context) {

This kind of long-lived access to an Android Context raises red-flags.  This almost certainly needs to be the application context -- anything else might be too short.  I see this created by the StumblerService, which you are trying to make persistent, but I don't think you can guarantee anything: Android will kill your service (and possibly your Context?) whenever it cares to.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/PersistentIntentService.java
@@ +22,5 @@
> +import android.os.HandlerThread;
> +import android.os.IBinder;
> +import android.os.Looper;
> +import android.os.Message;
> +

This whole thing has a terrible code smell that I don't understand well enough to articulate.  I just don't get it, but rnewman seems okay with it and I won't block on it.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/Zipper.java
@@ +1,1 @@
> +package org.mozilla.mozstumbler.service.utils;

Licenses, throughout.  I'm not going to note them all.

@@ +11,5 @@
> +public class Zipper {
> +    public static byte[] zipData(byte[] data) throws IOException {
> +        final ByteArrayOutputStream os = new ByteArrayOutputStream();
> +        GZIPOutputStream gstream = new GZIPOutputStream(os);
> +        if (gstream == null) {

This can't happen.  Either you get a new object, or you throw.

@@ +39,5 @@
> +            InputStreamReader reader = new InputStreamReader(gstream);
> +            BufferedReader in = new BufferedReader(reader);
> +            String read;
> +            while ((read = in.readLine()) != null) {
> +                result += read;

This is not used in this part of the code base, but is inefficient due to the string concatenations.  Prefer http://developer.android.com/reference/java/lang/StringBuilder.html.

Comment 78

[:garvan]

Thanks Nick, these updates should be no problem to get in.

> but I spent two hours reading this
> and am still not clear on the lifecycle of the services.

This is the greatest concern to me. We need readers to understand the code, as this is a community contributed project. We already have 3rd parties asking to integrate the stumbler lib into their work. 

The stumblerthread.StumblerService is the 'main' class, it then creates the classes that listen for cell, gps, and wifi (through the ScanManager), aggregates this data in 'bundles' (the Reporter), and save this to disk (DataStorageManager). This work is synchronous on this thread. I suspect you know all this already. 
The lifetime you speak of is the StumblerService lifetime. It must first receive a Fennec start intent (and an API key is part of that 'valid' first-time start intent), this is based on the state of the opt-in pref. (Without having received the aforementioned, it cannot be started.)
After that, start intents happen on boot, and on Fennec start and pause. It is a sticky service, so when killed, it starts up again.
 
That said, it is currently still used in MozStumbler as a bound service, there is more work on that side to be done to have it used exactly like Fennec, but it will happen. My personal MozStumbler build runs exactly like it does in Fennec.

> :::
> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/mainthread/
> PassiveServiceReceiver.java
> @@ +13,5 @@
> > +import org.mozilla.mozstumbler.service.stumblerthread.StumblerService;
> > +
> > +/* Starts the stumblerthread.StumblerService.
> > + * Registered as a receiver in manifest. Starts the StumblerService in passive listening mode.
> > + * Using GPS_* event changes during development, switch to using the existing permissions for a
> 
> The tense of "using" is misleading.  Do you mean, "while this code was under
> development, we used GPS_*"?  Or something else?

Sorry, these were notes for my debugging use, I'll take them out.
 
> @@ +15,5 @@
> > +/* Starts the stumblerthread.StumblerService.
> > + * Registered as a receiver in manifest. Starts the StumblerService in passive listening mode.
> > + * Using GPS_* event changes during development, switch to using the existing permissions for a
> > + * service on Fennec.
> > + <meta-data name="MOZILLA_API_KEY" value="aValue">
> 
> This is either wrong or likely to be wrong very quickly.  I'd prefer a
> reference to the right place to look for this information, if we care at all.

Also, debug notes that I use, I'll take it out, it is all working well at this point, there isn't anything to debug here.

> @@ +28,5 @@
> > +    static final String LOG_TAG = AppGlobals.LOG_PREFIX + PassiveServiceReceiver.class.getSimpleName();
> > +
> > +    @Override
> > +    public void onReceive(Context context, Intent intent) {
> > +        String action = intent.getAction();
> 
> You should verify that your intent is not-null (we see null intents a lot --
> it's Android!), and you should verify that it's really the intent you want. 
> You get kicked by at least 3 intents.

Good point, will do.

> @@ +31,5 @@
> > +    public void onReceive(Context context, Intent intent) {
> > +        String action = intent.getAction();
> > +        String mozApiKey = null;
> > +
> > +        if (action.contains(".STUMBLER_PREF")) {
> 
> I'd be careful that action is non-null too.  It's Android!

Good catch, will do.

> @@ +46,5 @@
> > +        Log.d(LOG_TAG, "Stumbler: Sending passive start message | isDebug:" + AppGlobals.isDebug);
> > +
> > +        Intent startServiceIntent = new Intent(context, StumblerService.class);
> > +        startServiceIntent.putExtra(StumblerService.ACTION_START_PASSIVE, true);
> > +        startServiceIntent.putExtra(StumblerService.ACTION_EXTRA_MOZ_API_KEY, mozApiKey);
> 
> This seems racy.  

As in a race-condition? This receiver is a main thread receiver. 

You could get a valid API key from STUMBLER_PREF, and then
> a null API key from a startup/SD card event.  Are null API keys filtered at
> the Prefs level?  If so, explain why this works! 

This can only be started by Fennec, with valid values, and those values are stored. StumblerService.onHandleIntent() has checks related to this.

> it's wrong if you get a non-STUMBLER_PREF intent before you
> hear from Fennec with the API key. 

By code contract, Fennec has started the service with the data it needs to run. The service stores these values. And further Intents to start the service are great, the more the merrier, it keeps the service alive (and on each start intent, we use that as a cue to check if any data needs uploading). 

> :::
> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/
> StumblerService.java
> @@ +25,5 @@
> > +import org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver;
> > +import org.mozilla.mozstumbler.service.utils.NetworkUtils;
> > +import org.mozilla.mozstumbler.service.utils.PersistentIntentService;
> > +
> > +// Created from PassiveServiceReceiver
> 
> Explain to me what this is used for and how it interacts with the other
> Services.

The PassiveServiceReceiver is the BroadcastReceiver for the Intent to start the the StumblerService. It is not used by active stumbling in MozStumbler (where the GPS is polled) hence the not-so-super name.

> 
> @@ +90,5 @@
> > +    public void checkPrefs() {
> > +        mScanManager.checkPrefs();
> > +    }
> > +
> > +    public int getLocationCount() {
> 
> None of these helpers are used in Fennec.  Are they necessary?

MozStumbler uses them. I did pull large chunks of code out (for now) of the service. Things like nickname, isGeofenced, some of the small accessors you mentioned, I have no plans to pull out of the library. If stumbling in Fennec is successful, and we decide to do an about:stumbling page that provides a lot of the info on the MozStumbler status screen, then this code will get used, and a bunch of code I pulled out will have to go back in. The code I pulled was not just because it wasn't used, it was because the code would take too long to fix up. The work there is not the coding per-se, it is me finding out the history of why things were done a certain way, in order to make the right decisions going forward.

> @@ +157,5 @@
> > +    @Override
> > +    public void onDestroy() {
> > +        super.onDestroy();
> > +
> > +        // Used to move these disk I/O ops off the calling thread. The current operations here are synchronized,
> 
> current?  Do you mean "concurrent", or do you mean "The operations currently
> here...".

Will reword, could have just said 'The operations in the AsyncTask that follows'. 

> @@ +171,5 @@
> > +                if (sFirefoxStumblingEnabled.get() == false) {
> > +                    Prefs.getInstance().setFirefoxScanEnabled(false);
> > +                }
> > +
> > +                if (DataStorageManager.getInstance() != null) {
> 
> This pattern is racy.  Prefer saving to a temporary, then null check and
> access the temporary copy.  (In this case, I think you're okay, because
> singleton nonsense.)

I think we agree there is no race condition here, but I would be interested to know the risk you see, I can address this in a future code update.

> @@ +174,5 @@
> > +
> > +                if (DataStorageManager.getInstance() != null) {
> > +                    try {
> > +                        DataStorageManager.getInstance().saveCurrentReportsToDisk();
> > +                    } catch (IOException ex) {
> 
> Is this really the only error you fear?  OOM, bad gravy, anything?  I'd
> catch Exception and log at least, just to make sure we don't die on a
> background thread.

The code is saving 5-10 KB of data (that would be a lot, most times I am getting 1-3 KB), I can't say I have any concerns. But I will follow your suggestion of switching to Exception and logging.

> @@ +213,5 @@
> > +        Prefs.getInstance().setFirefoxScanEnabled(true);
> > +
> > +        String apiKey = intent.getStringExtra(ACTION_EXTRA_MOZ_API_KEY);
> > +        if (apiKey != null) {
> > +            Prefs.getInstance().setMozApiKey(apiKey);
> 
> What happens if we start scanning below and /never/ see a non-null API key?

This is part of what I mentioned earlier. It is set by code contract or stumbling could never have started. I'll add a comment that clarifies this.

> @@ +222,5 @@
> > +    }
> > +
> > +    @Override
> > +    public IBinder onBind(Intent intent) {
> > +        mIsBound = true;
> 
> Explain in the class comment what you're doing with the binding lifecycle. 
> Why would you be unbound?  How would you use the mIsBound flag whilst
> unbound?  I see a stopScanning method (uncalled in Fennec!) that might care;
> how is that intended to be invoked?  Also, can you not get the bind state
> from the service and/or the binder?

I will comment this. Bound service is what MozStumbler release version is still using.

> :::
> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/uploadthread/
> AsyncUploader.java
> @@ +12,5 @@
> > +import org.mozilla.mozstumbler.service.AppGlobals;
> > +import org.mozilla.mozstumbler.service.stumblerthread.datahandling.DataStorageManager;
> > +import org.mozilla.mozstumbler.service.utils.NetworkUtils;
> > +
> > +/* Only one at a time may be uploading. If executed while another upload is in progress
> 
> Why not actually make it so, by making this a singleton and enforcing your
> invariant?  Because at the moment, you have an unsynchronized static boolean
> guard that races.

Will switch, I like your suggestion. The class that instantiates this is synchronous on the main thread, I don't actually see the race condition.

> :::
> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/
> AbstractCommunicator.java
> @@ +17,5 @@
> > +import java.net.HttpURLConnection;
> > +import java.net.MalformedURLException;
> > +import java.net.URL;
> > +
> > +public abstract class AbstractCommunicator {
> 
> This needs a nice class comment explaining what it's intended to accomplish
> (providing a uniform interface for Fennec and MozStumbler), how it came into
> being, and why it's not an actual interface.  Why is that, again?

I don't like this class, not my code either. But is acceptable to me. It is just a utility class for uploading. My beef is that it requires use by inheritance, a utility class should be used by composition, or maybe not require any OO-technique for use. It should be easy to use anywhere and in more than one context in a given class.

> @@ +54,5 @@
> > +        // Zero is no error, for HTTP error cases, set this code to the error
> > +        public int errorCode = -1;
> > +    }
> > +
> > +    /* Return non-zero for error, http error code if available */
> 
> This returns a NetworkSendResult, not an integer.

Thanks will fix comment.

> @@ +57,5 @@
> > +
> > +    /* Return non-zero for error, http error code if available */
> > +    public abstract NetworkSendResult cleanSend(byte[] data);
> > +
> > +    public String getNickname() {
> 
> This looks like it has something to do with MozStumbler?  Why is this here,
> if it's not used in Fennec?

Used in MozStumbler. 

> @@ +69,5 @@
> > +    }
> > +
> > +    private void openConnectionAndSetHeaders() {
> > +        try {
> > +            if (sMozApiKey == null) {
> 
> Shouldn't this do something if the API key is null even after fetching it? 
> Ah, I see that getMozApiKey() returns a special string in that case.  This
> special handling would be clearer if you just queried Prefs all the time,
> and cached (as necessary) over there.  You're passing the API key via the
> intent, but assuming you'll only get one API key.  That's true now, but
> fragile.

Agreed, will change.

> 
> @@ +101,5 @@
> > +        return output;
> > +    }
> > +
> > +    public boolean isCorrectResponse(int httpCode) {
> > +        return httpCode/100 == 2;
> 
> WAT.  This is a great way to convince people that 200 is all you accept,
> when actually you'll take 2xx. 

Bows. My name is on that one :).
I didn't realize the word 'correct' meant 200, I thought word 'ok' meant 200. 
Will change to isSuccessResponse(). 

> :::
> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/
> DateTimeUtils.java
> @@ +40,5 @@
> > +    public static String formatTime(long time) {
> > +        return formatDate(new Date(time));
> > +    }
> > +
> > +    public static String formatTimeForLocale(long time) {
> 
> This is unused in Fennec.  Is it really needed?  (Locale-specific stuff is
> dangerous, 'cuz the system can change locales at will, and we do let the
> user change locale-like l10n settings that might change this is
> unpredictable ways (that won't be noticed by this static pattern)).

Will remove this class. Thanks for catching this. No longer used by the service, only be the client side. 

> :::
> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/
> NetworkUtils.java
> @@ +12,5 @@
> > +
> > +public final class NetworkUtils {
> > +    private static final String LOG_TAG = AppGlobals.LOG_PREFIX + NetworkUtils.class.getSimpleName();
> > +
> > +    ConnectivityManager mConnectivityManager;
> 
> I know this code pre-dates your involvement, but bare variables are almost
> certainly wrong.  This can be final, too, if we make a protected constructor.

Will fix, I think I uglified this during one of my refactors.

> @@ +16,5 @@
> > +    ConnectivityManager mConnectivityManager;
> > +    static NetworkUtils sInstance;
> > +
> > +    /* Created at startup by app, or service, using a context. */
> > +    static public void createGlobalInstance(Context context) {
> 
> This kind of long-lived access to an Android Context raises red-flags.  This
> almost certainly needs to be the application context -- anything else might
> be too short.  I see this created by the StumblerService, which you are
> trying to make persistent, but I don't think you can guarantee anything:
> Android will kill your service (and possibly your Context?) whenever it
> cares to.

Will do. I tried to be careful with any classes that have a context member (in this code it is only classes that are workers of StumblerService that have this, alternatively they could have a back reference to this class). 
But I have *not* been careful to get the application context when passing a context into a function where the context is only used as an accessor to a system service (that is, the context is not used beyond a single function scope).
Context lifetime risk bothers me, so I'll go over the code and insert application context any where I wasn't doing this.
 
> 
> :::
> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/
> PersistentIntentService.java
> @@ +22,5 @@
> > +import android.os.HandlerThread;
> > +import android.os.IBinder;
> > +import android.os.Looper;
> > +import android.os.Message;
> > +
> 
> This whole thing has a terrible code smell that I don't understand well
> enough to articulate.  I just don't get it, but rnewman seems okay with it
> and I won't block on it.

I don't mind answering questions about it. Or "did you consider another approach?" questions. 
IntentService just doesn't work in its original form for long running tasks. 

> :::
> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/utils/Zipper.
> java
> @@ +1,1 @@
> > +package org.mozilla.mozstumbler.service.utils;
> 
> Licenses, throughout.  I'm not going to note them all.

Ok, will look for those.

> @@ +11,5 @@
> > +public class Zipper {
> > +    public static byte[] zipData(byte[] data) throws IOException {
> > +        final ByteArrayOutputStream os = new ByteArrayOutputStream();
> > +        GZIPOutputStream gstream = new GZIPOutputStream(os);
> > +        if (gstream == null) {
> 
> This can't happen.  Either you get a new object, or you throw.

Will remove null check.

> @@ +39,5 @@
> > +            InputStreamReader reader = new InputStreamReader(gstream);
> > +            BufferedReader in = new BufferedReader(reader);
> > +            String read;
> > +            while ((read = in.readLine()) != null) {
> > +                result += read;
> 

Yes, StringBuilder it should be.

Comment 79

[:garvan]

Nick: I am going to cleanup the StumblerService start operation and see if I can get this into a state where the behaviour is obvious. During a previous review I removed an startup state enum, that I found useful to reflect the state, and now I am confused reading the startup code logic.

Comment 80

[:garvan]

Attachment: Step 1: service_java_code_to_6f9955.patch

Addressed nearly everything in the review.

- Tried to clarify the role of the PassiveServiceReceiver and the StumblerService lifecycle, though some class comments, and clearer if-conditions. I had accidentally removed a conditional in PassiveServiceReceiver in my prev commit which is in fact needed to match my description of the lifecycle.

- AsyncUploader is not a singleton because AsyncTasks are not restartable. To restart you must create a new task.

- As I only see the IOExceptions are thrown from this code path, this is unchanged
try { DataStorageManager.getInstance().saveCurrentReportsToDisk();
} catch (IOException ex) {

Comment 81

[:garvan]

Attachment: Part 3: stumbler_manifest_and_mozbuild.patch

Carrying over the r+. 
I removed one line from the build for DateTimeUtils.java, this file was removed as part of the review.

Comment 82

[:garvan]

Comment on attachment 8474945 [details] [diff] [review]
Step 1: service_java_code_to_6f9955.patch

Review of attachment 8474945 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/mainthread/PassiveServiceReceiver.java
@@ +17,5 @@
> + * Starts the StumblerService in passive listening mode.
> + *
> + * The received intent serves these purposes:
> + * 1) The host application enables (or disables) the StumblerService.
> + *    Enabling requires passing in the upload API key. Both the enabled state, and the API key are stored in prefs.

Hmm, this wording is wrong. The upload API key should be passed for proper enabling in Fennec release mode, so that the debug API key isn't being used. The API Key is optional, it is not sent when this class is used in MozStumbler. In Fennec, the API Key is sent along with the intent to enable the service. 

I see how this is unclear, I will change this code to enforce that enabling the service requires passing an API key. On Fennec, someone might touch the start Intent code, decide the API key seems like an optional extra, and break the release build.

Comment 83

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8474945 [details] [diff] [review]
Step 1: service_java_code_to_6f9955.patch

Review of attachment 8474945 [details] [diff] [review]:
-----------------------------------------------------------------

OK, I give up trying to read this at a fine detail level.  It's too big, too inconsistent, and just doesn't follow best practices.  We'll live with that.

Instead, I read for these outcomes:

1) be sure this doesn't upload too frequently.

I understand the receiver/intent lifecycle at this point -- see long comment below -- so I think we're going to upload at a reasonable pace in good situations.

I'm worried that we're not hard rate-limiting to avoid error situations.  For example, where's our paranoia in the following situation: we get a background crash in a service that gets restarted, triggering Fennec to restart in a loop.  (Rare, but not unheard of.)  At the moment, we'll upload every Fennec start.  (We could have data every second with an App like Strava running.)  We currently have Fennec sync clients that Sync 300 times a day (rate-limited), and desktop Sync clients that sync 28000 times a day (not rate-limited).  We should be rate limiting to once every 30 minutes or something.

2) be sure this doesn't upload too much data.

It's not clear to me that we'll drop records if we're getting data faster than we can upload.  I do see some mechanism to not store too much on disk; will that cover us?  What if we make a mistake in our purging code, and we try to upload a 3meg payload?  Will it be dropped on failure?

3) be sure this doesn't write to disk too frequently.

I can see we're batching records in memory, but we're not dropping records if we get records really fast.  (That is, why can't we get 50 records in a second?  This might be reasonable if you're running an app like Strava.)

4) be sure this doesn't write too much data to disk.

Same as above.  We could hit the disk every second if we get records fast enough.

In toto: I'm basically OK that this patch does what it says it does, re: batching, writing records to disk, and infrequently uploading them.  That is, I think our record-set limiting is reasonable.  That's good.

I'm not convinced that our total-data-set limiting is reasonable.  I think we need to anticipate getting deluged with data (run trackers; route mappers) and be confident that we won't kill battery via disk access in such situations.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/StumblerService.java
@@ +218,5 @@
> +                    try {
> +                        DataStorageManager.getInstance().saveCurrentReportsToDisk();
> +                    } catch (IOException ex) {
> +                        AppGlobals.guiLogInfo(ex.toString());
> +                        Log.e(LOG_TAG, "Exception in onDestroy saving reports" + ex.toString());

Previously, this logged the exception with ", e);".  Why change (to something I think is wrong)?

@@ +248,5 @@
> +        }
> +
> +        if (!DataStorageManager.getInstance().isDirEmpty()) {
> +            // non-empty on startup, schedule an upload
> +            // This is the only upload trigger in Firefox mode

This comment is extremely important.  This is the key to understanding why we won't destroy a user's upload quota.

Find the correct place -- class comment here, UploadAlarmReceiver, in Fennec itself -- to explain how an upload gets triggered and why it won't be frequent.  For the record: it's my belief that this is because an upload is triggered by:

* starting Fennec;
* changing the pref;
* boot;
* SD card;

and nothing else.  (That is, collecting data never triggers an upload.)  If I'm wrong, please correct me.

@@ +252,5 @@
> +            // 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 int secondsToWait = 2;
> +            UploadAlarmReceiver.scheduleAlarm(this, secondsToWait, false /* no repeat*/);

Lift secondsToWait to a constant in the global constants, and comment on what it is intended for.

@@ +270,5 @@
> +            startScanning();
> +        }
> +    }
> +
> +    // Note that in passive mode, having data isn't an upload trigger, it is triggered by the start intent

This method name suggests a predicate.  Change to |setStorageIsEmpty| or |notifyStorageEmptyState| or similar.

@@ +275,5 @@
> +    public void storageIsEmpty(boolean isEmpty) {
> +        if (isEmpty) {
> +            UploadAlarmReceiver.cancelAlarm(this, !mScanManager.isPassiveMode());
> +        } else if (!mScanManager.isPassiveMode()) {
> +            int secondsToWait = 5 * 60;

Use the factored-out constant from above.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/datahandling/DataStorageManager.java
@@ +38,5 @@
> + * when the service is destroyed.
> + */
> +public class DataStorageManager {
> +    private static final String LOG_TAG = AppGlobals.LOG_PREFIX + DataStorageManager.class.getSimpleName();
> +    private static final int MAX_REPORTS_IN_MEMORY = 50;

Please comment the MAX_REPORTS_IN_MEMORY, MAX_BYTES_*, and MAX_WEEKS constants.  When others come to understand this behaviour, that's what they're going to need to read.

@@ +228,5 @@
> +        mFileList = new ReportFileList();
> +        mFileList.update(mReportsDir);
> +    }
> +
> +    public void setMaxStorageOnDisk(long maxBytes) {

rnewman raised this as well: why are these mutable?

I don't care to prolong this agony any further, but I would have much preferred to extract a simple storage interface that was implemented with all these layers of configurable nonsense for MozStumbler and implemented using dead-simple best practices for Fennec.  Looking deeper, the interface would have been about 4-5 methods, and simplified the concurrency issues.  Follow-up territory.

Comment 84

[Richard Newman [:rnewman]]

> Previously, this logged the exception with ", e);".  Why change (to
> something I think is wrong)?

I mentioned in one place (not sure if here) to use `+ e` to minimize the impact of logspam. Sometimes the stack is unimportant, but avoiding 20 lines of logging is polite.

Comment 85

[:garvan]

> Instead, I read for these outcomes:
> 
> 1) be sure this doesn't upload too frequently.> 
> I'm worried that we're not hard rate-limiting to avoid error situations. 
> For example, where's our paranoia in the following situation: we get a
> background crash in a service that gets restarted, triggering Fennec to
> restart in a loop.  (Rare, but not unheard of.)  At the moment, we'll upload
> every Fennec start. 

Added a guard against this in this commit:
https://github.com/garvankeeley/MozStumbler/commit/21ce350dfc6829e9179d5c5af3ff8f389d904390

Exact line is this one:
https://github.com/garvankeeley/MozStumbler/commit/21ce350dfc6829e9179d5c5af3ff8f389d904390#diff-2e283fd24b98cb72d625d6eef3e01a0dR207

> 2) be sure this doesn't upload too much data.
> 
> It's not clear to me that we'll drop records if we're getting data faster
> than we can upload.  I do see some mechanism to not store too much on disk;
> will that cover us?  

This is saveToDisk(..) function:
https://github.com/garvankeeley/MozStumbler/blob/fennec-code-standard/src/org/mozilla/mozstumbler/service/stumblerthread/datahandling/DataStorageManager.java#L390

This guard condition covers the disk is full (250KB). All saving operations go through this saveToDisk() function, and it will immediately return if we have exceeded that size.
Note it is pretty basic, it doesn't shut down any other part of the system. The in-memory report buffer keeps getting reports, and it gets cleared every 50 reports regardless of whether disk I/O succeeded or failed. 

>What if we make a mistake in our purging code, and we
> try to upload a 3meg payload?  Will it be dropped on failure?

We can't get a multi-MB payload in memory, unless the underlying wifi and scanner system went haywire and reported thousands of wifis and cells. Actually, just saw this can't happen, the code already had guards on the number of WIFIs+Cells per GPS location. 

I renamed the constants to be clearer to me:
https://github.com/garvankeeley/MozStumbler/commit/f5b838bda7dedd972127ebb7f09cd2d8e7a7e52f#diff-4297557d0a754023d400d83db99618abR127

As for getting multi-MB on disk, the system will never write to disk if there is 250KB stored.
ASIDE: not 250KB exactly, that is a precondition on the write operation, the last write will exceed this, but we have limited the size of 50 records to around 5KB ...and there are in-memory buffers to consider (those can be sent without ever going to disk), so ~260KB is the true limit. 

> 3) be sure this doesn't write to disk too frequently.
> 
> I can see we're batching records in memory, but we're not dropping records
> if we get records really fast.  (That is, why can't we get 50 records in a
> second?  This might be reasonable if you're running an app like Strava.)

GPSScanner has the guard regarding how often collection occurs, I updated a bit:
https://github.com/garvankeeley/MozStumbler/commit/0961391157923a75e68ed5a2fc766c7a5842cf4c

If requires 3 seconds OR 30 meters between GPS locations (travelling 100km/hr or greater in order to get 1 GPS per second). I just added an additional guard in case GPS arrives faster than 1Hz. Note it is possible to buy external GPS units that are >1Hz, although I am not aware of any that can pair with a phone.

As the in-memory buffer is 50 GPS reports, it could maximally write to disk every ~50 sec, under normal mobile hardware when moving at >100km/hr.

> 4) be sure this doesn't write too much data to disk.
> 
> Same as above.  We could hit the disk every second if we get records fast
> enough.

Already addressed in above comment.
 
> @@ +248,5 @@
> > +        }
> > +
> > +        if (!DataStorageManager.getInstance().isDirEmpty()) {
> > +            // non-empty on startup, schedule an upload
> > +            // This is the only upload trigger in Firefox mode
> 
> This comment is extremely important.  This is the key to understanding why
> we won't destroy a user's upload quota.
> 
> Find the correct place -- class comment here, UploadAlarmReceiver, in Fennec
> itself 

Updated the class comment for the UploadAlarmReceiver:
https://github.com/garvankeeley/MozStumbler/commit/71057230dd8cde997de3639f4a159d5ea5d9ef23

> @@ +252,5 @@
> > +            // 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 int secondsToWait = 2;
> > +            UploadAlarmReceiver.scheduleAlarm(this, secondsToWait, false /* no repeat*/);
> 
> Lift secondsToWait to a constant in the global constants, and comment on
> what it is intended for.

Hopefully this is better:
https://github.com/garvankeeley/MozStumbler/commit/21ce350dfc6829e9179d5c5af3ff8f389d904390
 
> @@ +270,5 @@
> > +            startScanning();
> > +        }
> > +    }
> > +
> > +    // Note that in passive mode, having data isn't an upload trigger, it is triggered by the start intent
> 
> This method name suggests a predicate.  Change to |setStorageIsEmpty| or
> |notifyStorageEmptyState| or similar.

Good idea, renamed that function here:
https://github.com/garvankeeley/MozStumbler/commit/9c6d8b070fa0834e5c52297cf87026d23ce3a4a9

> :::
> mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/
> datahandling/DataStorageManager.java
> @@ +38,5 @@
> > + * when the service is destroyed.
> > + */
> > +public class DataStorageManager {
> > +    private static final String LOG_TAG = AppGlobals.LOG_PREFIX + DataStorageManager.class.getSimpleName();
> > +    private static final int MAX_REPORTS_IN_MEMORY = 50;
> 
> Please comment the MAX_REPORTS_IN_MEMORY, MAX_BYTES_*, and MAX_WEEKS
> constants.  When others come to understand this behaviour, that's what
> they're going to need to read.

Done here for DataStorageManager.java:
https://github.com/garvankeeley/MozStumbler/commit/f05ea1726f2c032081eec8407771eade003518f5
 
> @@ +228,5 @@
> > +        mFileList = new ReportFileList();
> > +        mFileList.update(mReportsDir);
> > +    }
> > +
> > +    public void setMaxStorageOnDisk(long maxBytes) {
> 
> rnewman raised this as well: why are these mutable?

This is gone, these variables are now final on the class, refer to the above commit for the new service-side implementation. 

The long version of this commit -if you are interested- which shows the client-side implementation is here:
https://github.com/garvankeeley/MozStumbler/commit/f60aa8642a51090f64f2e3dea111564b95240378

Comment 86

[:garvan]

Comment on attachment 8473827 [details] [diff] [review]
Step 2: Prefs support for checking stumbler enabled and starting service

Review of attachment 8473827 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +870,5 @@
> +       Intent intent = new Intent(PREFS_STUMBLER_ENABLED)
> +                .putExtra("pref", PREFS_GEO_REPORTING)
> +                .putExtra("branch", GeckoSharedPrefs.APP_PREFS_NAME)
> +                .putExtra("enabled", value)
> +                .putExtra("moz_mozilla_api_key", AppConstants.MOZ_STUMBLER_API_KEY);

Also:
 .putExtra("host_app_name", <fennec's app name>)
 .putExtra("host_app_version", <fennec's app version string>)

These can be used as part of the user-agent in stumbler.

Comment 87

[:garvan]

Comment on attachment 8474949 [details] [diff] [review]
Part 3: stumbler_manifest_and_mozbuild.patch

Review of attachment 8474949 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/stumbler/stumbler_sources.mozbuild
@@ +13,5 @@
> +    'java/org/mozilla/mozstumbler/service/stumblerthread/blocklist/WifiBlockListInterface.java',
> +    'java/org/mozilla/mozstumbler/service/stumblerthread/datahandling/DataStorageContract.java',
> +    'java/org/mozilla/mozstumbler/service/stumblerthread/datahandling/DataStorageManager.java',
> +    'java/org/mozilla/mozstumbler/service/stumblerthread/datahandling/StumblerBundle.java',
> +    'java/org/mozilla/mozstumbler/service/stumblerthread/datahandling/StumblerBundleReceiver.java',

Reminder: StumblerBundleReceiver.java is removed, update this.

Comment 88

[:garvan]

> Also:
>  .putExtra("host_app_name", <fennec's app name>)
>  .putExtra("host_app_version", <fennec's app version string>)
> 
> These can be used as part of the user-agent in stumbler.

Aha, this is what i need to add GeckoAppShell.getGeckoInterface().getDefaultUAString()

Comment 89

[:garvan]

After stumbling with my latest change to GPSScanner, decided that my previous logic for when to keep a GPS location was better:

https://github.com/garvankeeley/MozStumbler/commit/90982f95342847e9f339b5a213fd4b6db0c9adc4

When a GPS location arrives, if it is less than 3 seconds from the previous, or insufficient movement has taken place, ignore it.

Comment 90

[:garvan]

Attachment: step 2: Prefs support for checking stumbler enabled and starting service

Can you review the one-line changed in broadcastStumblerPref:
.putExtra("user_agent", GeckoAppShell.getGeckoInterface().getDefaultUAString());

[Also, this patch is rebased on today's m-c]

Comment 91

[:garvan]

Attachment: Part 3: stumbler manifest and mozbuild

Carrying over previous r+, removed one line for a java file no longer used.

Comment 92

[:garvan]

Changes for handling user-agent string from Fennec:
https://github.com/garvankeeley/MozStumbler/commit/87cdf32a362ea7cea639f77b94956f360cd00026

This is the stumbler-side code for Comment 90

Comment 93

[:garvan]

Comment on attachment 8474945 [details] [diff] [review]
Step 1: service_java_code_to_6f9955.patch

This patch semi-obsolete since for the final review steps we are pointing to the github diffs. Once the github diffs are ok'd, I'll repost this patch with the final code for landing.

Ach! I should have done comment 85 as an attachment so I could mark it r+. Anyhoo, can I get feedback on comment 85?

Comment 94

[:garvan]

Attachment: bug1038843-step2-prefs_changes_to_java_and_build_files.patch

Same as comment 90, updated patch as it had a stray ';'

Comment 95

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8480742 [details] [diff] [review]
bug1038843-step2-prefs_changes_to_java_and_build_files.patch

Review of attachment 8480742 [details] [diff] [review]:
-----------------------------------------------------------------

WFM.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +859,5 @@
> +                .putExtra("pref", PREFS_GEO_REPORTING)
> +                .putExtra("branch", GeckoSharedPrefs.APP_PREFS_NAME)
> +                .putExtra("enabled", value)
> +                .putExtra("moz_mozilla_api_key", AppConstants.MOZ_STUMBLER_API_KEY)
> +                .putExtra("user_agent", GeckoAppShell.getGeckoInterface().getDefaultUAString());

Let's null check |getGeckoInterface|, crazy though that sounds.

Comment 96

[:garvan]

Attachment: Step 2: Prefs support for checking stumbler enabled and starting service

Updated to check for getGeckoInterface returning null, carry over r+

Comment 97

[Nick Alexander :nalexander [he/him]]

OK, I think this is ready to rock-and-roll.  Please upload all the patches, tidy commit messages, etc.  I'd like to apply locally, sanity check, and push them myself.

Comment 98

[:garvan]

Attachment: Part 1: stumbler java code to 5b14c1f

r+ from Nick A. as part of  comment 97.

Comment 99

[:garvan]

Nick for your testing, I recommend the following: 
- turn it on of course: https://bugzilla.mozilla.org/show_bug.cgi?id=1054520
- run with adb logcat | grep -i stumbler
- you can monitor this directory for stumbler files:
 /storage/emulated/legacy/Android/data/org.mozilla.fennec/files/mozstumbler/reports
(in debug, it is placed in the above public location, in non-debug it places files in private app data)
- the directory above the reports dir, has a text file with the upload stats

- keep in mind it is tricky to collect data indoors (because we require pure GPS locations), and without moving (30 meters between GPS points).

Comment 100

[Richard Newman [:rnewman]]

Relevant for testing:

https://developer.android.com/training/location/location-testing.html

You can enable the mock location setting in Developer Options, then use a spoofer to generate location changes.

Comment 101

[Nick Alexander :nalexander [he/him]]

Smoketest looks fine locally:

I GeckoPreferences(2454)      Changed app.geo.reportdata = true
D Telemetry(2454)             SendUIEvent: event = edit.1 method = settings timestamp = 1409332320 extras = app.geo.reportdata
D Stumbler:PassiveServiceReceiver(2454)                   Stumbler: Sending passive start message | isDebug:true
D AbsListView(2454)           unregisterIRListener() is called 
D Stumbler:ScanManager(2454)  Scanning started...
I GeckoPreferences(2454)      Changed app.geo.reportdata = false
D Telemetry(2454)             SendUIEvent: event = edit.1 method = settings timestamp = 1409352812 extras = app.geo.reportdata
D AbsListView(2454)           unregisterIRListener() is called 
D Stumbler:Reporter(2454)     shutdown
D Stumbler:ScanManager(2454)  Scanning stopped
D Stumbler:StumblerService(2454)                          onDestroy

Comment 102

[Nick Alexander :nalexander [he/him]]

I folded 3rd patch into the 1st, so we landed in two pieces:

https://hg.mozilla.org/integration/fx-team/rev/0239fbe8a0c8
https://hg.mozilla.org/integration/fx-team/rev/e7075da2b9d8

Fingers crossed!

Comment 103

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/0239fbe8a0c8
https://hg.mozilla.org/mozilla-central/rev/e7075da2b9d8

Comment 104

[:garvan]

sorry, wrong bug window