Bug 890040 - Protect against orphans in Android health report uploader. r=rnewman

This does two main things.  First, it tracks sets of obsolete document
IDs and retries obsoletes and deletes.  Second, it tracks document IDs
that are attempted to be written to the server for obsolescence or
deletion without waiting for a server response to prevent orphaning
documents on successful upload and failed response.

The ObsoleteDocumentTracker uses a JSON map to maintain obsolete
document IDs and priority values.  That's not the right data
structure; Bug 894194 tracks improving it.

mobile/android/base/android-services-files.mk

@@ -35,6 +35,7 @@ SYNC_JAVA_FILES := \
   background/common/log/writers/StringLogWriter.java \
   background/common/log/writers/TagLogWriter.java \
   background/common/log/writers/ThreadLocalTagLogWriter.java \
+  background/datareporting/TelemetryRecorder.java \
   background/db/CursorDumper.java \
   background/db/Tab.java \
   background/healthreport/Environment.java \
@@ -51,6 +52,7 @@ SYNC_JAVA_FILES := \
   background/healthreport/upload/HealthReportBroadcastService.java \
   background/healthreport/upload/HealthReportUploadService.java \
   background/healthreport/upload/HealthReportUploadStartReceiver.java \
+  background/healthreport/upload/ObsoleteDocumentTracker.java \
   background/healthreport/upload/SubmissionClient.java \
   background/healthreport/upload/SubmissionPolicy.java \
   sync/AlreadySyncingException.java \

mobile/android/base/background/bagheera/BagheeraClient.java

@@ -7,10 +7,12 @@ package org.mozilla.gecko.background.bagheera;
 import java.io.IOException;
 import java.net.URISyntaxException;
 import java.security.GeneralSecurityException;
+import java.util.Collection;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Executors;
 import java.util.regex.Pattern;
 
+import org.mozilla.gecko.sync.Utils;
 import org.mozilla.gecko.sync.net.BaseResource;
 import org.mozilla.gecko.sync.net.BaseResourceDelegate;
 import org.mozilla.gecko.sync.net.Resource;
@@ -102,8 +104,8 @@ public class BagheeraClient {
    *          the document ID, which is typically a UUID.
    * @param payload
    *          a document, typically JSON-encoded.
-   * @param oldID
-   *          an optional ID which denotes a document to supersede. Can be null.
+   * @param oldIDs
+   *          an optional collection of IDs which denote documents to supersede. Can be null or empty.
    * @param delegate
    *          the delegate whose methods should be invoked on success or
    *          failure.
@@ -111,7 +113,7 @@ public class BagheeraClient {
   public void uploadJSONDocument(final String namespace,
                                  final String id,
                                  final String payload,
-                                 final String oldID,
+                                 Collection<String> oldIDs,
                                  final BagheeraRequestDelegate delegate) throws URISyntaxException {
     if (namespace == null) {
       throw new IllegalArgumentException("Must provide namespace.");
@@ -126,7 +128,7 @@ public class BagheeraClient {
     final BaseResource resource = makeResource(namespace, id);
     final HttpEntity deflatedBody = DeflateHelper.deflateBody(payload);
 
-    resource.delegate = new BagheeraUploadResourceDelegate(resource, namespace, id, oldID, delegate);
+    resource.delegate = new BagheeraUploadResourceDelegate(resource, namespace, id, oldIDs, delegate);
     resource.post(deflatedBody);
   }
 
@@ -228,23 +230,23 @@ public class BagheeraClient {
   public final class BagheeraUploadResourceDelegate extends BagheeraResourceDelegate {
     private static final String HEADER_OBSOLETE_DOCUMENT = "X-Obsolete-Document";
     private static final String COMPRESSED_CONTENT_TYPE = "application/json+zlib; charset=utf-8";
-    protected String obsoleteDocumentID;
+    protected final Collection<String> obsoleteDocumentIDs;
 
     public BagheeraUploadResourceDelegate(Resource resource,
         String namespace,
         String id,
-        String obsoleteDocumentID,
+        Collection<String> obsoleteDocumentIDs,
         BagheeraRequestDelegate delegate) {
       super(resource, namespace, id, delegate);
-      this.obsoleteDocumentID = obsoleteDocumentID;
+      this.obsoleteDocumentIDs = obsoleteDocumentIDs;
     }
 
     @Override
     public void addHeaders(HttpRequestBase request, DefaultHttpClient client) {
       super.addHeaders(request, client);
       request.setHeader(HTTP.CONTENT_TYPE, COMPRESSED_CONTENT_TYPE);
-      if (this.obsoleteDocumentID != null) {
-        request.addHeader(HEADER_OBSOLETE_DOCUMENT, this.obsoleteDocumentID);
+      if (this.obsoleteDocumentIDs != null && this.obsoleteDocumentIDs.size() > 0) {
+        request.addHeader(HEADER_OBSOLETE_DOCUMENT, Utils.toCommaSeparatedString(this.obsoleteDocumentIDs));
       }
     }
   }

mobile/android/base/background/healthreport/HealthReportConstants.java.in

@@ -61,8 +61,26 @@ public class HealthReportConstants {
 
   public static final String PREF_OBSOLETE_DOCUMENT_IDS_TO_DELETION_ATTEMPTS_REMAINING = "healthreport_obsolete_document_ids_to_deletions_remaining";
 
-  public static final String PREF_DELETION_ATTEMPTS_PER_OBSOLETE_DOCUMENT_ID = "healthreport_deletion_attempts_per_obsolete_document_id";
-  public static final long DEFAULT_DELETION_ATTEMPTS_PER_OBSOLETE_DOCUMENT_ID = 5;
+  // We don't want to try to delete forever, but we also don't want to orphan
+  // obsolete document IDs from devices that fail to reach the server for a few
+  // days.  This tries to delete document IDs for at least one week (of upload
+  // failures).  Note that if the device is really offline, no upload is
+  // performed and our count of attempts is not altered.
+  public static final long DELETION_ATTEMPTS_PER_OBSOLETE_DOCUMENT_ID = (DEFAULT_MAXIMUM_FAILURES_PER_DAY + 1) * 7;
+
+  // If we absolutely know that a document ID reached the server, we really
+  // don't want to orphan it.  This tries to delete document IDs that will
+  // definitely be orphaned for at least six weeks (of upload failures).  Note
+  // that if the device is really offline, no upload is performed and our count
+  // of attempts is not altered.
+  public static final long DELETION_ATTEMPTS_PER_KNOWN_TO_BE_ON_SERVER_DOCUMENT_ID = (DEFAULT_MAXIMUM_FAILURES_PER_DAY + 1) * 7 * 6;
+
+  // We don't want to allocate unbounded storage for obsolete IDs, but we also
+  // don't want to orphan obsolete document IDs from devices that fail to delete
+  // for a few days.  This stores as many IDs as are expected to be generated in
+  // a month.  Note that if the device is really offline, no upload is performed
+  // and our count of attempts is not altered.
+  public static final long MAXIMUM_STORED_OBSOLETE_DOCUMENT_IDS = (DEFAULT_MAXIMUM_FAILURES_PER_DAY + 1) * 30;
 
   // Forensic.
   public static final String PREF_LAST_DELETE_REQUESTED = "healthreport_last_delete_requested";
@@ -78,4 +96,12 @@ public class HealthReportConstants {
 
   public static final String PREF_DOCUMENT_SERVER_NAMESPACE = "healthreport_document_server_namespace";
   public static final String DEFAULT_DOCUMENT_SERVER_NAMESPACE = "metrics";
+
+  // One UUID is 36 characters (like e56542e0-e4d2-11e2-a28f-0800200c9a66), so
+  // we limit the number of obsolete IDs passed so that each request is not a
+  // large upload (and therefore more likely to fail).  We also don't want to
+  // push Bagheera to make too many deletes, since we don't know how the cluster
+  // will handle such API usage.  This obsoletes 2 days worth of old documents
+  // at a time.
+  public static final int MAXIMUM_DELETIONS_PER_POST = ((int) DEFAULT_MAXIMUM_FAILURES_PER_DAY + 1) * 2;
 }

mobile/android/base/background/healthreport/HealthReportUtils.java

@@ -12,16 +12,14 @@ import java.util.Set;
 import java.util.SortedSet;
 import java.util.TimeZone;
 import java.util.TreeSet;
+import java.util.UUID;
 
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
 import org.mozilla.apache.commons.codec.digest.DigestUtils;
-import org.mozilla.gecko.background.common.log.Logger;
-import org.mozilla.gecko.sync.ExtendedJSONObject;
 
 import android.content.ContentUris;
-import android.content.SharedPreferences;
 import android.net.Uri;
 
 public class HealthReportUtils {
@@ -149,30 +147,7 @@ public class HealthReportUtils {
     dest.put(value, dest.optInt(value, 0) + 1);
   }
 
-  public static ExtendedJSONObject getObsoleteIds(SharedPreferences sharedPrefs) {
-    String s = sharedPrefs.getString(HealthReportConstants.PREF_OBSOLETE_DOCUMENT_IDS_TO_DELETION_ATTEMPTS_REMAINING, null);
-    if (s == null) {
-      return new ExtendedJSONObject();
-    }
-    try {
-      return ExtendedJSONObject.parseJSONObject(s);
-    } catch (Exception e) {
-      Logger.warn(LOG_TAG, "Got exception getting obsolete ids.", e);
-      return new ExtendedJSONObject();
-    }
-  }
-
-  /**
-   * Write obsolete ids to disk.
-   *
-   * @param sharedPrefs to write to.
-   * @param ids to write.
-   * @return editor.
-   */
-  public static void setObsoleteIds(SharedPreferences sharedPrefs, ExtendedJSONObject ids) {
-    sharedPrefs
-      .edit()
-      .putString(HealthReportConstants.PREF_OBSOLETE_DOCUMENT_IDS_TO_DELETION_ATTEMPTS_REMAINING, ids.toString())
-      .commit();
+  public static String generateDocumentId() {
+    return UUID.randomUUID().toString();
   }
 }

mobile/android/base/background/healthreport/upload/AndroidSubmissionClient.java

@@ -5,7 +5,7 @@
 package org.mozilla.gecko.background.healthreport.upload;
 
 import java.io.IOException;
-import java.util.UUID;
+import java.util.Collection;
 
 import org.json.JSONObject;
 import org.mozilla.gecko.background.bagheera.BagheeraClient;
@@ -15,6 +15,7 @@ import org.mozilla.gecko.background.healthreport.EnvironmentBuilder;
 import org.mozilla.gecko.background.healthreport.HealthReportConstants;
 import org.mozilla.gecko.background.healthreport.HealthReportDatabaseStorage;
 import org.mozilla.gecko.background.healthreport.HealthReportGenerator;
+import org.mozilla.gecko.sync.net.BaseResource;
 
 import android.content.ContentProviderClient;
 import android.content.Context;
@@ -61,24 +62,25 @@ public class AndroidSubmissionClient implements SubmissionClient {
       .commit();
   }
 
-  protected void uploadPayload(String payload, BagheeraRequestDelegate uploadDelegate) {
+  protected void uploadPayload(String id, String payload, Collection<String> oldIds, BagheeraRequestDelegate uploadDelegate) {
     final BagheeraClient client = new BagheeraClient(getDocumentServerURI());
 
-    final String id = UUID.randomUUID().toString();
-    final String lastId = getLastUploadDocumentId();
-
     Logger.pii(LOG_TAG, "New health report has id " + id +
-        (lastId == null ? "." : " and obsoletes id " + lastId + "."));
+        "and obsoletes " + (oldIds != null ? Integer.toString(oldIds.size()) : "no") + " old ids.");
 
     try {
-      client.uploadJSONDocument(getDocumentServerNamespace(), id, payload, lastId, uploadDelegate);
+      client.uploadJSONDocument(getDocumentServerNamespace(),
+          id,
+          payload,
+          oldIds,
+          uploadDelegate);
     } catch (Exception e) {
       uploadDelegate.handleError(e);
     }
   }
 
   @Override
-  public void upload(long localTime, Delegate delegate) {
+  public void upload(long localTime, String id, Collection<String> oldIds, Delegate delegate) {
     // We abuse the life-cycle of an Android ContentProvider slightly by holding
     // onto a ContentProviderClient while we generate a payload. This keeps our
     // database storage alive, and may also allow us to share a database
@@ -116,8 +118,8 @@ public class AndroidSubmissionClient implements SubmissionClient {
         return;
       }
 
-      BagheeraRequestDelegate uploadDelegate = new RequestDelegate(delegate, localTime, true, null);
-      this.uploadPayload(document.toString(), uploadDelegate);
+      BagheeraRequestDelegate uploadDelegate = new RequestDelegate(delegate, localTime, true, id);
+      this.uploadPayload(id, document.toString(), oldIds, uploadDelegate);
     } catch (Exception e) {
       Logger.warn(LOG_TAG, "Got exception generating document.", e);
       delegate.onHardFailure(localTime, null, "Got exception uploading.", e);
@@ -153,11 +155,12 @@ public class AndroidSubmissionClient implements SubmissionClient {
       this.localTime = localTime;
       this.isUpload = isUpload;
       this.methodString = this.isUpload ? "upload" : "delete";
-      this.id = this.isUpload ? null : id; // id is known for deletions only.
+      this.id = id;
     }
 
     @Override
     public void handleSuccess(int status, String namespace, String id, HttpResponse response) {
+      BaseResource.consumeEntity(response);
       if (isUpload) {
         setLastUploadLocalTimeAndDocumentId(localTime, id);
       }
@@ -176,6 +179,7 @@ public class AndroidSubmissionClient implements SubmissionClient {
      */
     @Override
     public void handleFailure(int status, String namespace, HttpResponse response) {
+      BaseResource.consumeEntity(response);
       Logger.debug(LOG_TAG, "Failed " + methodString + " at " + localTime + ".");
       if (status >= 500) {
         delegate.onSoftFailure(localTime, id, "Got status " + status + " from server.", null);

mobile/android/base/background/healthreport/upload/HealthReportBroadcastService.java

@@ -8,8 +8,6 @@ import org.mozilla.gecko.background.BackgroundService;
 import org.mozilla.gecko.background.common.GlobalConstants;
 import org.mozilla.gecko.background.common.log.Logger;
 import org.mozilla.gecko.background.healthreport.HealthReportConstants;
-import org.mozilla.gecko.background.healthreport.HealthReportUtils;
-import org.mozilla.gecko.sync.ExtendedJSONObject;
 
 import android.app.AlarmManager;
 import android.app.PendingIntent;
@@ -43,10 +41,6 @@ public class HealthReportBroadcastService extends BackgroundService {
     getSharedPreferences().edit().putLong(HealthReportConstants.PREF_SUBMISSION_INTENT_INTERVAL_MSEC, interval).commit();
   }
 
-  public long getDeletionAttemptsPerObsoleteDocumentId() {
-    return getSharedPreferences().getLong(HealthReportConstants.PREF_DELETION_ATTEMPTS_PER_OBSOLETE_DOCUMENT_ID, HealthReportConstants.DEFAULT_DELETION_ATTEMPTS_PER_OBSOLETE_DOCUMENT_ID);
-  }
-
   /**
    * Set or cancel an alarm to submit data for a profile.
    *
@@ -143,22 +137,16 @@ public class HealthReportBroadcastService extends BackgroundService {
     Logger.pii(LOG_TAG, "Updating health report alarm for profile " + profileName + " at " + profilePath + ".");
 
     final SharedPreferences sharedPrefs = getSharedPreferences();
-
-    ExtendedJSONObject obsoleteIds = HealthReportUtils.getObsoleteIds(getSharedPreferences());
+    final ObsoleteDocumentTracker tracker = new ObsoleteDocumentTracker(sharedPrefs);
+    final boolean hasObsoleteIds = tracker.hasObsoleteIds();
 
     if (!enabled) {
-      String lastId = sharedPrefs.getString(HealthReportConstants.PREF_LAST_UPLOAD_DOCUMENT_ID, null);
       final Editor editor = sharedPrefs.edit();
       editor.remove(HealthReportConstants.PREF_LAST_UPLOAD_DOCUMENT_ID);
 
-      if (lastId != null) {
-        try {
-          obsoleteIds.put(lastId, getDeletionAttemptsPerObsoleteDocumentId());
-        } catch (Exception e) {
-          Logger.warn(LOG_TAG, "Got exception updating obsolete ids JSON.", e);
-        }
-        HealthReportUtils.setObsoleteIds(getSharedPreferences(), obsoleteIds);
-        Logger.debug(LOG_TAG, "New health report to obsolete; scheduling deletion of " + obsoleteIds.size() + " documents.");
+      if (hasObsoleteIds) {
+        Logger.debug(LOG_TAG, "Health report upload disabled; scheduling deletion of " + tracker.numberOfObsoleteIds() + " documents.");
+        tracker.limitObsoleteIds();
       } else {
         // Primarily intended for debugging and testing.
         Logger.debug(LOG_TAG, "Health report upload disabled and no deletes to schedule: clearing prefs.");
@@ -171,7 +159,7 @@ public class HealthReportBroadcastService extends BackgroundService {
 
     // The user can toggle us off or on, or we can have obsolete documents to
     // remove.
-    final boolean serviceEnabled = (obsoleteIds.size() > 0) || enabled;
+    final boolean serviceEnabled = hasObsoleteIds || enabled;
     toggleAlarm(this, profileName, profilePath, enabled, serviceEnabled);
   }
 }

mobile/android/base/background/healthreport/upload/HealthReportUploadService.java

@@ -73,8 +73,9 @@ public class HealthReportUploadService extends BackgroundService {
     Logger.pii(LOG_TAG, "Ticking policy for profile " + profileName + " at " + profilePath + ".");
 
     final SharedPreferences sharedPrefs = getSharedPreferences();
+    final ObsoleteDocumentTracker tracker = new ObsoleteDocumentTracker(sharedPrefs);
     SubmissionClient client = new AndroidSubmissionClient(this, sharedPrefs, profilePath);
-    SubmissionPolicy policy = new SubmissionPolicy(sharedPrefs, client, uploadEnabled);
+    SubmissionPolicy policy = new SubmissionPolicy(sharedPrefs, client, tracker, uploadEnabled);
 
     final long now = System.currentTimeMillis();
     policy.tick(now);

mobile/android/base/background/healthreport/upload/ObsoleteDocumentTracker.java

@@ -0,0 +1,245 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.gecko.background.healthreport.upload;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map.Entry;
+import java.util.Set;
+
+import org.mozilla.gecko.background.common.log.Logger;
+import org.mozilla.gecko.background.healthreport.HealthReportConstants;
+import org.mozilla.gecko.sync.ExtendedJSONObject;
+
+import android.content.SharedPreferences;
+
+public class ObsoleteDocumentTracker {
+  public static final String LOG_TAG = ObsoleteDocumentTracker.class.getSimpleName();
+
+  protected final SharedPreferences sharedPrefs;
+
+  public ObsoleteDocumentTracker(SharedPreferences sharedPrefs) {
+    this.sharedPrefs = sharedPrefs;
+  }
+
+  protected ExtendedJSONObject getObsoleteIds() {
+    String s = sharedPrefs.getString(HealthReportConstants.PREF_OBSOLETE_DOCUMENT_IDS_TO_DELETION_ATTEMPTS_REMAINING, null);
+    if (s == null) {
+      // It's possible we're migrating an old profile forward.
+      String lastId = sharedPrefs.getString(HealthReportConstants.PREF_LAST_UPLOAD_DOCUMENT_ID, null);
+      if (lastId == null) {
+        return new ExtendedJSONObject();
+      }
+      ExtendedJSONObject ids = new ExtendedJSONObject();
+      ids.put(lastId, HealthReportConstants.DELETION_ATTEMPTS_PER_OBSOLETE_DOCUMENT_ID);
+      setObsoleteIds(ids);
+      return ids;
+    }
+    try {
+      return ExtendedJSONObject.parseJSONObject(s);
+    } catch (Exception e) {
+      Logger.warn(LOG_TAG, "Got exception getting obsolete ids.", e);
+      return new ExtendedJSONObject();
+    }
+  }
+
+  /**
+   * Write obsolete ids to disk.
+   *
+   * @param ids to write.
+   */
+  protected void setObsoleteIds(ExtendedJSONObject ids) {
+    sharedPrefs
+      .edit()
+      .putString(HealthReportConstants.PREF_OBSOLETE_DOCUMENT_IDS_TO_DELETION_ATTEMPTS_REMAINING, ids.toString())
+      .commit();
+  }
+
+  /**
+   * Remove id from set of obsolete document ids tracked for deletion.
+   *
+   * Public for testing.
+   *
+   * @param id to stop tracking.
+   */
+  public void removeObsoleteId(String id) {
+    ExtendedJSONObject ids = getObsoleteIds();
+    ids.remove(id);
+    setObsoleteIds(ids);
+  }
+
+  protected void decrementObsoleteId(ExtendedJSONObject ids, String id) {
+    if (!ids.containsKey(id)) {
+      return;
+    }
+    try {
+      Long attempts = ids.getLong(id);
+      if (attempts == null || --attempts < 1) {
+        ids.remove(id);
+      } else {
+        ids.put(id, attempts);
+      }
+    } catch (ClassCastException e) {
+      ids.remove(id);
+      Logger.info(LOG_TAG, "Got exception decrementing obsolete ids counter.", e);
+    }
+  }
+
+  /**
+   * Decrement attempts remaining for id in set of obsolete document ids tracked
+   * for deletion.
+   *
+   * Public for testing.
+   *
+   * @param id to decrement attempts.
+   */
+  public void decrementObsoleteIdAttempts(String id) {
+    ExtendedJSONObject ids = getObsoleteIds();
+    decrementObsoleteId(ids, id);
+    setObsoleteIds(ids);
+  }
+
+  public void purgeObsoleteIds(Collection<String> oldIds) {
+    ExtendedJSONObject ids = getObsoleteIds();
+    for (String oldId : oldIds) {
+      ids.remove(oldId);
+    }
+    setObsoleteIds(ids);
+  }
+
+  public void decrementObsoleteIdAttempts(Collection<String> oldIds) {
+    ExtendedJSONObject ids = getObsoleteIds();
+    for (String oldId : oldIds) {
+      decrementObsoleteId(ids, oldId);
+    }
+    setObsoleteIds(ids);
+  }
+
+  /**
+   * Sort Longs in decreasing order, moving null and non-Longs to the front.
+   *
+   * Public for testing only.
+   */
+  public static class PairComparator implements Comparator<Entry<String, Object>> {
+    @Override
+    public int compare(Entry<String, Object> lhs, Entry<String, Object> rhs) {
+      Object l = lhs.getValue();
+      Object r = rhs.getValue();
+      if (l == null || !(l instanceof Long)) {
+        if (r == null || !(r instanceof Long)) {
+          return 0;
+        }
+        return -1;
+      }
+      if (r == null || !(r instanceof Long)) {
+        return 1;
+      }
+      return ((Long) r).compareTo((Long) l);
+    }
+  }
+
+  /**
+   * Return a batch of obsolete document IDs that should be deleted next.
+   *
+   * Document IDs are long and sending too many in a single request might
+   * increase the likelihood of POST failures, so we delete a (deterministic)
+   * subset here.
+   *
+   * @return a non-null collection.
+   */
+  public Collection<String> getBatchOfObsoleteIds() {
+    ExtendedJSONObject ids = getObsoleteIds();
+    // Sort by increasing order of key values.
+    List<Entry<String, Object>> pairs = new ArrayList<Entry<String,Object>>(ids.entrySet());
+    Collections.sort(pairs, new PairComparator());
+    List<String> batch = new ArrayList<String>(HealthReportConstants.MAXIMUM_DELETIONS_PER_POST);
+    int i = 0;
+    while (batch.size() < HealthReportConstants.MAXIMUM_DELETIONS_PER_POST && i < pairs.size()) {
+      batch.add(pairs.get(i++).getKey());
+    }
+    return batch;
+  }
+
+  /**
+   * Track the given document ID for eventual obsolescence and deletion.
+   * Obsolete IDs are not known to have been uploaded to the server, so we just
+   * give a best effort attempt at deleting them
+   *
+   * @param id to eventually delete.
+   */
+  public void addObsoleteId(String id) {
+    ExtendedJSONObject ids = getObsoleteIds();
+    if (ids.size() >= HealthReportConstants.MAXIMUM_STORED_OBSOLETE_DOCUMENT_IDS) {
+      // Remove the one that's been tried the most and is least likely to be
+      // known to be on the server. Since the comparator orders in decreasing
+      // order, we take the max.
+      ids.remove(Collections.max(ids.entrySet(), new PairComparator()).getKey());
+    }
+    ids.put(id, HealthReportConstants.DELETION_ATTEMPTS_PER_OBSOLETE_DOCUMENT_ID);
+    setObsoleteIds(ids);
+  }
+
+  /**
+   * Track the given document ID for eventual obsolescence and deletion, and
+   * give it priority since we know this ID has made it to the server, and we
+   * definitely don't want to orphan it.
+   *
+   * @param id to eventually delete.
+   */
+  public void markIdAsUploaded(String id) {
+    ExtendedJSONObject ids = getObsoleteIds();
+    ids.put(id, HealthReportConstants.DELETION_ATTEMPTS_PER_KNOWN_TO_BE_ON_SERVER_DOCUMENT_ID);
+    setObsoleteIds(ids);
+  }
+
+  public boolean hasObsoleteIds() {
+    return getObsoleteIds().size() > 0;
+  }
+
+  public int numberOfObsoleteIds() {
+    return getObsoleteIds().size();
+  }
+
+  public String getNextObsoleteId() {
+    ExtendedJSONObject ids = getObsoleteIds();
+    if (ids.size() < 1) {
+      return null;
+    }
+    try {
+      // Delete the one that's most likely to be known to be on the server, and
+      // that's not been tried as much. Since the comparator orders in
+      // decreasing order, we take the min.
+      return Collections.min(ids.entrySet(), new PairComparator()).getKey();
+    } catch (Exception e) {
+      Logger.warn(LOG_TAG, "Got exception picking obsolete id to delete.", e);
+      return null;
+    }
+  }
+
+  /**
+   * We want cleaning up documents on the server to be best effort. Purge badly
+   * formed IDs and cap the number of times we try to delete so that the queue
+   * doesn't take too long.
+   */
+  public void limitObsoleteIds() {
+    ExtendedJSONObject ids = getObsoleteIds();
+
+    Set<String> keys = new HashSet<String>(ids.keySet()); // Avoid invalidating an iterator.
+    for (String key : keys) {
+      Object o = ids.get(key);
+      if (!(o instanceof Long)) {
+        continue;
+      }
+      if (((Long) o).longValue() > HealthReportConstants.DELETION_ATTEMPTS_PER_OBSOLETE_DOCUMENT_ID) {
+        ids.put(key, HealthReportConstants.DELETION_ATTEMPTS_PER_OBSOLETE_DOCUMENT_ID);
+      }
+    }
+    setObsoleteIds(ids);
+  }
+}

mobile/android/base/background/healthreport/upload/SubmissionClient.java

@@ -4,6 +4,8 @@
 
 package org.mozilla.gecko.background.healthreport.upload;
 
+import java.util.Collection;
+
 public interface SubmissionClient {
   public interface Delegate {
     /**
@@ -35,6 +37,6 @@ public interface SubmissionClient {
     public void onSuccess(long localTime, String id);
   }
 
-  public void upload(long localTime, Delegate delegate);
+  public void upload(long localTime, String id, Collection<String> oldIds, Delegate delegate);
   public void delete(long localTime, String id, Delegate delegate);
 }

mobile/android/base/background/healthreport/upload/SubmissionPolicy.java

@@ -4,13 +4,15 @@
 
 package org.mozilla.gecko.background.healthreport.upload;
 
-import java.util.Collections;
+import java.net.MalformedURLException;
+import java.net.SocketException;
+import java.net.UnknownHostException;
+import java.util.Collection;
 
 import org.mozilla.gecko.background.common.log.Logger;
 import org.mozilla.gecko.background.healthreport.HealthReportConstants;
 import org.mozilla.gecko.background.healthreport.HealthReportUtils;
 import org.mozilla.gecko.background.healthreport.upload.SubmissionClient.Delegate;
-import org.mozilla.gecko.sync.ExtendedJSONObject;
 
 import android.content.SharedPreferences;
 
@@ -47,10 +49,18 @@ public class SubmissionPolicy {
   protected final SharedPreferences sharedPreferences;
   protected final SubmissionClient client;
   protected final boolean uploadEnabled;
+  protected final ObsoleteDocumentTracker tracker;
 
-  public SubmissionPolicy(final SharedPreferences sharedPreferences, final SubmissionClient client, boolean uploadEnabled) {
+  public SubmissionPolicy(final SharedPreferences sharedPreferences,
+      final SubmissionClient client,
+      final ObsoleteDocumentTracker tracker,
+      boolean uploadEnabled) {
+    if (sharedPreferences == null) {
+      throw new IllegalArgumentException("sharedPreferences must not be null");
+    }
     this.sharedPreferences = sharedPreferences;
     this.client = client;
+    this.tracker = tracker;
     this.uploadEnabled = uploadEnabled;
   }
 
@@ -84,26 +94,15 @@ public class SubmissionPolicy {
       return false;
     }
 
-    ExtendedJSONObject ids = getObsoleteIds();
-    if (ids.size() > 0) {
-      // Deleting obsolete documents takes precedence over everything else. We
-      // try to delete aggressively, since the volume of deletes should be very
-      // low. But we don't want to send too many delete requests at the same
-      // time, so we process these one at a time. In the future (Bug 872756), we
-      // will be able to delete multiple documents with one request.
-      String obsoleteId;
-      try {
-        // We don't care what the order is, but let's make testing easier by
-        // being deterministic. Deleting in random order might avoid failing too
-        // many times in succession, but we expect only a single pending delete
-        // in practice.
-        obsoleteId = Collections.min(ids.keySet());
-      } catch (Exception e) {
-        Logger.warn(LOG_TAG, "Got exception picking obsolete id to delete.", e);
-        return false;
-      }
+    if (!uploadEnabled) {
+      // We only delete (rather than mark as obsolete during upload) when
+      // uploading is disabled. We try to delete aggressively, since the volume
+      // of deletes should be very low. But we don't want to send too many
+      // delete requests at the same time, so we process these one at a time. In
+      // the future (Bug 872756), we will be able to delete multiple documents
+      // with one request.
+      final String obsoleteId = tracker.getNextObsoleteId();
       if (obsoleteId == null) {
-        Logger.error(LOG_TAG, "Next obsolete id to delete is null?");
         return false;
       }
 
@@ -113,12 +112,6 @@ public class SubmissionPolicy {
       return true;
     }
 
-    // If we delete all obsolete ids, we could fall through to this point, and
-    // we don't want to upload.
-    if (!uploadEnabled) {
-      return false;
-    }
-
     long firstRun = getFirstRunLocalTime();
     if (firstRun < 0) {
       firstRun = localTime;
@@ -135,37 +128,67 @@ public class SubmissionPolicy {
       return false;
     }
 
+    String id = HealthReportUtils.generateDocumentId();
+    Collection<String> oldIds = tracker.getBatchOfObsoleteIds();
+    tracker.addObsoleteId(id);
+
     Editor editor = editor();
     editor.setLastUploadRequested(localTime); // Write committed by delegate.
-    client.upload(localTime, new UploadDelegate(editor));
+    client.upload(localTime, id, oldIds, new UploadDelegate(editor, oldIds));
     return true;
   }
 
+  /**
+   * Return true if the upload that produced <code>e</code> definitely did not
+   * produce a new record on the remote server.
+   *
+   * @param e
+   *          <code>Exception</code> that upload produced.
+   * @return true if the server could not have a new record.
+   */
+  protected boolean isLocalException(Exception e) {
+    return (e instanceof MalformedURLException) ||
+           (e instanceof SocketException) ||
+           (e instanceof UnknownHostException);
+  }
+
   protected class UploadDelegate implements Delegate {
     protected final Editor editor;
+    protected final Collection<String> oldIds;
 
-    public UploadDelegate(Editor editor) {
+    public UploadDelegate(Editor editor, Collection<String> oldIds) {
       this.editor = editor;
+      this.oldIds = oldIds;
     }
 
     @Override
     public void onSuccess(long localTime, String id) {
       long next = localTime + getMinimumTimeBetweenUploads();
+      tracker.markIdAsUploaded(id);
+      tracker.purgeObsoleteIds(oldIds);
       editor
         .setNextSubmission(next)
         .setLastUploadSucceeded(localTime)
         .setCurrentDayFailureCount(0)
         .commit();
       if (Logger.LOG_PERSONAL_INFORMATION) {
-        Logger.pii(LOG_TAG, "Successful upload with id " + id + " reported at " + localTime + "; next upload at " + next + ".");
+        Logger.pii(LOG_TAG, "Successful upload with id " + id + " obsoleting "
+            + oldIds.size() + " old records reported at " + localTime + "; next upload at " + next + ".");
       } else {
-        Logger.info(LOG_TAG, "Successful upload reported at " + localTime + "; next upload at " + next + ".");
+        Logger.info(LOG_TAG, "Successful upload obsoleting " + oldIds.size()
+            + " old records reported at " + localTime + "; next upload at " + next + ".");
       }
     }
 
     @Override
     public void onHardFailure(long localTime, String id, String reason, Exception e) {
       long next = localTime + getMinimumTimeBetweenUploads();
+      if (isLocalException(e)) {
+        Logger.info(LOG_TAG, "Hard failure caused by local exception; not tracking id and not decrementing attempts.");
+        tracker.removeObsoleteId(id);
+      } else {
+        tracker.decrementObsoleteIdAttempts(oldIds);
+      }
       editor
         .setNextSubmission(next)
         .setLastUploadFailed(localTime)
@@ -177,14 +200,20 @@ public class SubmissionPolicy {
     @Override
     public void onSoftFailure(long localTime, String id, String reason, Exception e) {
       int failuresToday = getCurrentDayFailureCount();
-      Logger.warn(LOG_TAG, "Soft failure reported at " + localTime + ": " + reason + " Previously failed " + failuresToday + " today.");
+      Logger.warn(LOG_TAG, "Soft failure reported at " + localTime + ": " + reason + " Previously failed " + failuresToday + " time(s) today.");
 
       if (failuresToday >= getMaximumFailuresPerDay()) {
-        onHardFailure(localTime, id, "Reached the limit of daily upload attempts.", null);
+        onHardFailure(localTime, id, "Reached the limit of daily upload attempts: " + failuresToday, e);
         return;
       }
 
       long next = localTime + getMinimumTimeAfterFailure();
+      if (isLocalException(e)) {
+        Logger.info(LOG_TAG, "Soft failure caused by local exception; not tracking id and not decrementing attempts.");
+        tracker.removeObsoleteId(id);
+      } else {
+        tracker.decrementObsoleteIdAttempts(oldIds);
+      }
       editor
         .setNextSubmission(next)
         .setLastUploadFailed(localTime)
@@ -204,7 +233,11 @@ public class SubmissionPolicy {
     @Override
     public void onSoftFailure(final long localTime, String id, String reason, Exception e) {
       long next = localTime + getMinimumTimeBetweenDeletes();
-      decrementObsoleteIdAttempts(id);
+      if (isLocalException(e)) {
+        Logger.info(LOG_TAG, "Soft failure caused by local exception; not decrementing attempts.");
+      } else {
+        tracker.decrementObsoleteIdAttempts(id);
+      }
       editor
         .setNextSubmission(next)
         .setLastDeleteFailed(localTime)
@@ -221,7 +254,7 @@ public class SubmissionPolicy {
     public void onHardFailure(final long localTime, String id, String reason, Exception e) {
       // We're never going to be able to delete this id, so don't keep trying.
       long next = localTime + getMinimumTimeBetweenDeletes();
-      removeObsoleteId(id);
+      tracker.removeObsoleteId(id);
       editor
         .setNextSubmission(next)
         .setLastDeleteFailed(localTime)
@@ -237,7 +270,7 @@ public class SubmissionPolicy {
     @Override
     public void onSuccess(final long localTime, String id) {
       long next = localTime + getMinimumTimeBetweenDeletes();
-      removeObsoleteId(id);
+      tracker.removeObsoleteId(id);
       editor
         .setNextSubmission(next)
         .setLastDeleteSucceeded(localTime)
@@ -393,53 +426,4 @@ public class SubmissionPolicy {
   public long getMinimumTimeBetweenDeletes() {
     return getSharedPreferences().getLong(HealthReportConstants.PREF_MINIMUM_TIME_BETWEEN_DELETES, HealthReportConstants.DEFAULT_MINIMUM_TIME_BETWEEN_DELETES);
   }
-
-  public ExtendedJSONObject getObsoleteIds() {
-    return HealthReportUtils.getObsoleteIds(getSharedPreferences());
-  }
-
-  public void setObsoleteIds(ExtendedJSONObject ids) {
-    HealthReportUtils.setObsoleteIds(getSharedPreferences(), ids);
-  }
-
-  /**
-   * Remove id from set of obsolete document ids tracked for deletion.
-   *
-   * Public for testing.
-   *
-   * @param id to stop tracking.
-   */
-  public void removeObsoleteId(String id) {
-    ExtendedJSONObject ids = HealthReportUtils.getObsoleteIds(getSharedPreferences());
-    ids.remove(id);
-    setObsoleteIds(ids);
-  }
-
-  /**
-   * Decrement attempts remaining for id in set of obsolete document ids tracked
-   * for deletion.
-   *
-   * Public for testing.
-   *
-   * @param id to decrement attempts.
-   */
-  public void decrementObsoleteIdAttempts(String id) {
-    ExtendedJSONObject ids = HealthReportUtils.getObsoleteIds(getSharedPreferences());
-
-    if (!ids.containsKey(id)) {
-      return;
-    }
-    try {
-      Long attempts = ids.getLong(id);
-      if (attempts == null || --attempts < 1) {
-        ids.remove(id);
-      } else {
-        ids.put(id, attempts);
-      }
-    } catch (ClassCastException e) {
-      Logger.info(LOG_TAG, "Got exception decrementing obsolete ids counter.", e);
-    }
-
-    setObsoleteIds(ids);
-  }
 }

mobile/android/base/sync/CollectionKeys.java

@@ -121,7 +121,7 @@ public class CollectionKeys {
 
     ExtendedJSONObject collections = cleartext.getObject("collections");
     HashMap<String, KeyBundle> collectionKeys = new HashMap<String, KeyBundle>();
-    for (Entry<String, Object> pair : collections.entryIterable()) {
+    for (Entry<String, Object> pair : collections.entrySet()) {
       KeyBundle bundle = arrayToKeyBundle((JSONArray) pair.getValue());
       collectionKeys.put(pair.getKey(), bundle);
     }

mobile/android/base/sync/ExtendedJSONObject.java

@@ -285,7 +285,7 @@ public class ExtendedJSONObject {
   }
 
   @SuppressWarnings("unchecked")
-  public Iterable<Entry<String, Object>> entryIterable() {
+  public Set<Entry<String, Object>> entrySet() {
     return this.object.entrySet();
   }
 

mobile/android/base/sync/SyncConfiguration.java

@@ -340,7 +340,7 @@ public class SyncConfiguration implements CredentialsSource {
     try {
       ExtendedJSONObject o = ExtendedJSONObject.parseJSONObject(json);
       Map<String, Boolean> map = new HashMap<String, Boolean>();
-      for (Entry<String, Object> e : o.entryIterable()) {
+      for (Entry<String, Object> e : o.entrySet()) {
         String key = e.getKey();
         Boolean value = (Boolean) e.getValue();
         map.put(key, value);

mobile/android/services/java-sources.mn

@@ -22,6 +22,7 @@ background/common/log/writers/SimpleTagLogWriter.java
 background/common/log/writers/StringLogWriter.java
 background/common/log/writers/TagLogWriter.java
 background/common/log/writers/ThreadLocalTagLogWriter.java
+background/datareporting/TelemetryRecorder.java
 background/db/CursorDumper.java
 background/db/Tab.java
 background/healthreport/Environment.java
@@ -38,6 +39,7 @@ background/healthreport/upload/HealthReportBroadcastReceiver.java
 background/healthreport/upload/HealthReportBroadcastService.java
 background/healthreport/upload/HealthReportUploadService.java
 background/healthreport/upload/HealthReportUploadStartReceiver.java
+background/healthreport/upload/ObsoleteDocumentTracker.java
 background/healthreport/upload/SubmissionClient.java
 background/healthreport/upload/SubmissionPolicy.java
 sync/AlreadySyncingException.java