Bug 1386027 - Simplify handleError interfaces for SessionCallback and TelemetryCollector r=rnewman draft
authorGrigory Kruglov <gkruglov@mozilla.com>
Tue, 08 Aug 2017 13:45:29 -0400
changeset 642752 19bbd2c98776b32b803d7febb55549bc430cbc3e
parent 642518 a921bfb8a2cf3db4d9edebe9b35799a3f9d035da
child 725083 da956550f692566f8db148ff8cebe9eee8c77a5f
push id72848
push userbmo:gkruglov@mozilla.com
push dateTue, 08 Aug 2017 17:46:37 +0000
reviewersrnewman
bugs1386027
milestone57.0a1
Bug 1386027 - Simplify handleError interfaces for SessionCallback and TelemetryCollector r=rnewman Error reporting, and especially the split between per-stage and global session errors, are a bit of a mess; at the very least, this patch unifies things a little bit, and ensures we're not passing around nulls unexpectedly. MozReview-Commit-ID: JTSp7GuOji0
mobile/android/base/android-services.mozbuild
mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/BackoffException.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/CryptoKeysChangedException.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/GlobalSession.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/OutdatedStorageVersionException.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/delegates/GlobalSessionCallback.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/EnsureCrypto5KeysStage.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/FetchMetaGlobalStage.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/telemetry/TelemetryCollector.java
mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/sync/TestResetting.java
mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/testhelpers/DefaultGlobalSessionCallback.java
mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/TestResetCommands.java
mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/helpers/MockGlobalSessionCallback.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/testhelpers/DefaultGlobalSessionCallback.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/telemetry/TelemetryCollectorTest.java
--- a/mobile/android/base/android-services.mozbuild
+++ b/mobile/android/base/android-services.mozbuild
@@ -859,32 +859,34 @@ sync_java_files = [TOPSRCDIR + '/mobile/
     'fxa/sync/FxAccountSyncStatusHelper.java',
     'fxa/sync/SchedulePolicy.java',
     'fxa/SyncStatusListener.java',
     'push/autopush/AutopushClient.java',
     'push/autopush/AutopushClientException.java',
     'push/RegisterUserAgentResponse.java',
     'push/SubscribeChannelResponse.java',
     'sync/AlreadySyncingException.java',
+    'sync/BackoffException.java',
     'sync/BackoffHandler.java',
     'sync/BadRequiredFieldJSONException.java',
     'sync/CollectionConcurrentModificationException.java',
     'sync/CollectionKeys.java',
     'sync/CommandProcessor.java',
     'sync/CommandRunner.java',
     'sync/CredentialException.java',
     'sync/crypto/CryptoException.java',
     'sync/crypto/CryptoInfo.java',
     'sync/crypto/HKDF.java',
     'sync/crypto/HMACVerificationException.java',
     'sync/crypto/KeyBundle.java',
     'sync/crypto/MissingCryptoInputException.java',
     'sync/crypto/NoKeyBundleException.java',
     'sync/crypto/PBKDF2.java',
     'sync/crypto/PersistedCrypto5Keys.java',
+    'sync/CryptoKeysChangedException.java',
     'sync/CryptoRecord.java',
     'sync/DelayedWorkTracker.java',
     'sync/delegates/ClientsDataDelegate.java',
     'sync/delegates/FreshStartDelegate.java',
     'sync/delegates/GlobalSessionCallback.java',
     'sync/delegates/JSONRecordFetchDelegate.java',
     'sync/delegates/KeyUploadDelegate.java',
     'sync/delegates/MetaGlobalDelegate.java',
@@ -938,16 +940,17 @@ sync_java_files = [TOPSRCDIR + '/mobile/
     'sync/net/TLSSocketFactory.java',
     'sync/net/WBOCollectionRequestDelegate.java',
     'sync/net/WBORequestDelegate.java',
     'sync/NoCollectionKeysSetException.java',
     'sync/NodeAuthenticationException.java',
     'sync/NonArrayJSONException.java',
     'sync/NonObjectJSONException.java',
     'sync/NullClusterURLException.java',
+    'sync/OutdatedStorageVersionException.java',
     'sync/PersistedMetaGlobal.java',
     'sync/PrefsBackoffHandler.java',
     'sync/ReflowIsNecessaryException.java',
     'sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java',
     'sync/repositories/android/AndroidBrowserBookmarksRepository.java',
     'sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java',
     'sync/repositories/android/AndroidBrowserHistoryDataAccessor.java',
     'sync/repositories/android/AndroidBrowserHistoryRepository.java',
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java
@@ -28,16 +28,17 @@ import org.mozilla.gecko.fxa.authenticat
 import org.mozilla.gecko.fxa.authenticator.AndroidFxAccount;
 import org.mozilla.gecko.fxa.authenticator.FxADefaultLoginStateMachineDelegate;
 import org.mozilla.gecko.fxa.authenticator.FxAccountAuthenticator;
 import org.mozilla.gecko.fxa.login.FxAccountLoginStateMachine;
 import org.mozilla.gecko.fxa.login.Married;
 import org.mozilla.gecko.fxa.login.State;
 import org.mozilla.gecko.fxa.login.State.StateLabel;
 import org.mozilla.gecko.fxa.sync.FxAccountSyncDelegate.Result;
+import org.mozilla.gecko.sync.BackoffException;
 import org.mozilla.gecko.sync.BackoffHandler;
 import org.mozilla.gecko.sync.GlobalSession;
 import org.mozilla.gecko.sync.PrefsBackoffHandler;
 import org.mozilla.gecko.sync.SharedPreferencesClientsDataDelegate;
 import org.mozilla.gecko.sync.SyncConfiguration;
 import org.mozilla.gecko.sync.ThreadPool;
 import org.mozilla.gecko.sync.Utils;
 import org.mozilla.gecko.sync.crypto.KeyBundle;
@@ -171,38 +172,28 @@ public class FxAccountSyncAdapter extend
       recordTelemetry();
     }
 
     @Override
     public void handleError(GlobalSession globalSession, Exception ex, String reason) {
       super.handleError(globalSession, ex, reason);
       // If an error hasn't been set downstream, record what we know at this point.
       if (!telemetryCollector.hasError()) {
-        telemetryCollector.setError(TelemetryCollector.KEY_ERROR_INTERNAL, reason, ex);
-      }
-      recordTelemetry();
-    }
-
-    @Override
-    public void handleError(GlobalSession globalSession, Exception ex) {
-      super.handleError(globalSession, ex);
-      // If an error hasn't been set downstream, record what we know at this point.
-      if (!telemetryCollector.hasError()) {
-        telemetryCollector.setError(TelemetryCollector.KEY_ERROR_INTERNAL, ex);
+        telemetryCollector.setError(TelemetryCollector.KEY_ERROR_INTERNAL, ex, reason);
       }
       recordTelemetry();
     }
 
     @Override
     public void handleAborted(GlobalSession globalSession, String reason) {
       super.handleAborted(globalSession, reason);
       // Note to future maintainers: while there are reasons, other than 'backoff', this method
       // might be called, in practice that _is_ the only reason it gets called at the moment of
       // writing this. If this changes, please do expand this telemetry handling.
-      this.telemetryCollector.setError(TelemetryCollector.KEY_ERROR_INTERNAL, "backoff");
+      this.telemetryCollector.setError(TelemetryCollector.KEY_ERROR_INTERNAL, new BackoffException(), reason);
       recordTelemetry();
     }
 
     private void recordTelemetry() {
       telemetryCollector.setFinished(SystemClock.elapsedRealtime());
       final Intent telemetryIntent = new Intent();
       telemetryIntent.setAction(ACTION_BACKGROUND_TELEMETRY);
       telemetryIntent.putExtra(TelemetryContract.KEY_TYPE, TelemetryContract.KEY_TYPE_SYNC);
@@ -288,23 +279,18 @@ public class FxAccountSyncAdapter extend
       } finally {
         // Continue with the usual success flow.
         syncDelegate.handleSuccess();
       }
     }
 
     @Override
     public void handleError(GlobalSession globalSession, Exception ex, String reason) {
-      this.handleError(globalSession, ex);
-    }
-
-    @Override
-    public void handleError(GlobalSession globalSession, Exception e) {
       Logger.warn(LOG_TAG, "Global session failed."); // Exception will be dumped by delegate below.
-      syncDelegate.handleError(e);
+      syncDelegate.handleError(ex);
       // TODO: should we reduce the periodic sync interval?
     }
 
     @Override
     public void handleAborted(GlobalSession globalSession, String reason) {
       Logger.warn(LOG_TAG, "Global session aborted: " + reason);
       syncDelegate.handleError(null);
       // TODO: should we reduce the periodic sync interval?
@@ -423,50 +409,43 @@ public class FxAccountSyncAdapter extend
           Collection<String> knownStageNames = SyncConfiguration.validEngineNames();
           syncConfig.stagesToSync = Utils.getStagesToSyncFromBundle(knownStageNames, extras);
           syncConfig.setClusterURL(storageServerURI);
 
           globalSession = new GlobalSession(syncConfig, callback, context, clientsDataDelegate, callback.getCollector());
           callback.getCollector().setIDs(token.hashedFxaUid, clientsDataDelegate.getAccountGUID());
           globalSession.start(syncDeadline);
         } catch (Exception e) {
-          callback.handleError(globalSession, e);
-          return;
+          callback.handleError(globalSession, e, "Unexpected error while starting a sync");
         }
       }
 
       @Override
       public void handleFailure(TokenServerException e) {
         Logger.error(LOG_TAG, "Failed to get token.", e);
         try {
           // We should only get here *after* we're locked into the married state.
           State state = fxAccount.getState();
           if (state.getStateLabel() == StateLabel.Married) {
             Married married = (Married) state;
             fxAccount.setState(married.makeCohabitingState());
           }
         } finally {
           fxAccount.releaseSharedAccountStateLock();
         }
-        callback.getCollector().setError(
-                TelemetryCollector.KEY_ERROR_TOKEN,
-                e.getClass().getSimpleName()
-        );
-        callback.handleError(null, e);
+        callback.getCollector().setError(TelemetryCollector.KEY_ERROR_TOKEN, e);
+        callback.handleError(null, e, "Failure processing a token");
       }
 
       @Override
       public void handleError(Exception e) {
         Logger.error(LOG_TAG, "Failed to get token.", e);
         fxAccount.releaseSharedAccountStateLock();
-        callback.getCollector().setError(
-                TelemetryCollector.KEY_ERROR_TOKEN,
-                e.getClass().getSimpleName()
-        );
-        callback.handleError(null, e);
+        callback.getCollector().setError(TelemetryCollector.KEY_ERROR_TOKEN, e);
+        callback.handleError(null, e, "Error getting a token");
       }
 
       @Override
       public void handleBackoff(int backoffSeconds) {
         // This is the token server telling us to back off.
         Logger.info(LOG_TAG, "Token server requesting backoff of " + backoffSeconds + "s. Backoff handler: " + tokenBackoffHandler);
         didReceiveBackoff = true;
 
new file mode 100644
--- /dev/null
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/BackoffException.java
@@ -0,0 +1,9 @@
+/* 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.sync;
+
+public class BackoffException extends SyncException {
+    private static final long serialVersionUID = 2966836737310897884L;
+}
new file mode 100644
--- /dev/null
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/CryptoKeysChangedException.java
@@ -0,0 +1,9 @@
+/* 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.sync;
+
+public class CryptoKeysChangedException extends SyncException {
+    private static final long serialVersionUID = 5729127849933343128L;
+}
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/GlobalSession.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/GlobalSession.java
@@ -1,16 +1,18 @@
 /* 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.sync;
 
 import android.content.Context;
 import android.os.SystemClock;
+import android.support.annotation.NonNull;
+import android.support.annotation.Nullable;
 import android.support.annotation.VisibleForTesting;
 
 import org.json.simple.JSONArray;
 import org.mozilla.gecko.background.common.log.Logger;
 import org.mozilla.gecko.sync.crypto.CryptoException;
 import org.mozilla.gecko.sync.crypto.KeyBundle;
 import org.mozilla.gecko.sync.delegates.ClientsDataDelegate;
 import org.mozilla.gecko.sync.delegates.FreshStartDelegate;
@@ -272,17 +274,17 @@ public class GlobalSession implements Ht
 
   /**
    * Move to the next stage in the syncing process.
    */
   public void advance() {
     // If we have a backoff, request a backoff and don't advance to next stage.
     long existingBackoff = largestBackoffObserved.get();
     if (existingBackoff > 0) {
-      this.abort(null, "Aborting sync because of backoff of " + existingBackoff + " milliseconds.");
+      this.abort(new BackoffException(), "Aborting sync because of backoff of " + existingBackoff + " milliseconds.");
       return;
     }
 
     this.callback.handleStageCompleted(this.currentState, this);
     Stage next = nextStage(this.currentState);
     GlobalSyncStage nextStage;
     try {
       nextStage = this.getSyncStageByName(next);
@@ -507,17 +509,16 @@ public class GlobalSession implements Ht
         Logger.warn(LOG_TAG, "Got exception trying to upload updated meta/global record; ignoring.", e);
         synchronized (monitor) {
           monitor.notify();
         }
       }
     };
   }
 
-
   public void abort(Exception e, String reason) {
     Logger.warn(LOG_TAG, "Aborting sync: " + reason, e);
     cleanUp();
     long existingBackoff = largestBackoffObserved.get();
     if (existingBackoff > 0) {
       callback.requestBackoff(existingBackoff);
     }
     if (!(e instanceof HTTPFailureException)) {
@@ -1132,17 +1133,17 @@ public class GlobalSession implements Ht
 
   /**
    * Suggest that your Sync client needs to be upgraded to work
    * with this server.
    */
   public void requiresUpgrade() {
     Logger.info(LOG_TAG, "Client outdated storage version; requires update.");
     // TODO: notify UI.
-    this.abort(null, "Requires upgrade from " + STORAGE_VERSION);
+    this.abort(new OutdatedStorageVersionException(), "Requires upgrade from " + STORAGE_VERSION);
   }
 
   /**
    * If meta/global is missing or malformed, throws a MetaGlobalException.
    * Otherwise, returns true if there is an entry for this engine in the
    * meta/global "engines" object.
    * <p>
    * This is a global/permanent setting, not a local/temporary setting. For the
new file mode 100644
--- /dev/null
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/OutdatedStorageVersionException.java
@@ -0,0 +1,9 @@
+/* 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.sync;
+
+/* package-private */ class OutdatedStorageVersionException extends SyncException {
+    private static final long serialVersionUID = 1703691318986965545L;
+}
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/delegates/GlobalSessionCallback.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/delegates/GlobalSessionCallback.java
@@ -31,17 +31,16 @@ public interface GlobalSessionCallback {
    * Called when a migration sentinel has been found and processed successfully.
    * <p>
    * This account should stop syncing immediately, and arrange to delete itself.
    */
   void informMigrated(GlobalSession session);
 
   void handleAborted(GlobalSession globalSession, String reason);
   void handleError(GlobalSession globalSession, Exception ex, String reason);
-  void handleError(GlobalSession globalSession, Exception ex);
   void handleSuccess(GlobalSession globalSession);
   void handleStageCompleted(Stage currentState, GlobalSession globalSession);
   void handleIncompleteStage(Stage currentState, GlobalSession globalSession);
   void handleFullSyncNecessary();
 
   /**
    * Called when a {@link GlobalSession} wants to know if it should continue
    * to make storage requests.
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/EnsureCrypto5KeysStage.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/EnsureCrypto5KeysStage.java
@@ -7,16 +7,17 @@ package org.mozilla.gecko.sync.stage;
 import android.os.SystemClock;
 
 import java.net.URISyntaxException;
 import java.util.HashSet;
 import java.util.Set;
 
 import org.mozilla.gecko.background.common.log.Logger;
 import org.mozilla.gecko.sync.CollectionKeys;
+import org.mozilla.gecko.sync.CryptoKeysChangedException;
 import org.mozilla.gecko.sync.CryptoRecord;
 import org.mozilla.gecko.sync.ExtendedJSONObject;
 import org.mozilla.gecko.sync.HTTPFailureException;
 import org.mozilla.gecko.sync.InfoCollections;
 import org.mozilla.gecko.sync.NoCollectionKeysSetException;
 import org.mozilla.gecko.sync.crypto.KeyBundle;
 import org.mozilla.gecko.sync.crypto.PersistedCrypto5Keys;
 import org.mozilla.gecko.sync.net.AuthHeaderProvider;
@@ -45,17 +46,17 @@ implements SyncStorageRequestDelegate {
   @Override
   public void execute() throws NoSuchStageException {
     InfoCollections infoCollections = session.config.infoCollections;
     if (infoCollections == null) {
       telemetryStageCollector.finished = SystemClock.elapsedRealtime();
       telemetryStageCollector.error = new TelemetryCollector
               .StageErrorBuilder(TELEMETRY_ERROR_NAME, TELEMETRY_ERROR_NO_COLLECTIONS)
               .build();
-      session.abort(null, "No info/collections set in EnsureCrypto5KeysStage.");
+      session.abort(new IllegalStateException(), "No info/collections set in EnsureCrypto5KeysStage.");
       return;
     }
 
     PersistedCrypto5Keys pck = session.config.persistedCryptoKeys();
     long lastModified = pck.lastModified();
     if (retrying || !infoCollections.updateNeeded(CRYPTO_COLLECTION, lastModified)) {
       // Try to use our local collection keys for this session.
       Logger.debug(LOG_TAG, "Trying to use persisted collection keys for this session.");
@@ -180,17 +181,17 @@ implements SyncStorageRequestDelegate {
       // New keys, different from old keys.
       Logger.trace(LOG_TAG, "Fetched keys are not the same as persisted keys; " +
           "setting fetched keys for this session before resetting changed engines.");
       setAndPersist(pck, keys, responseTimestamp);
       session.resetStagesByName(changedCollections);
       telemetryStageCollector.error = new TelemetryCollector
               .StageErrorBuilder(TELEMETRY_ERROR_NAME, TELEMETRY_ERROR_KEYS_CHANGED)
               .build();
-      session.abort(null, "crypto/keys changed on server.");
+      session.abort(new CryptoKeysChangedException(), "crypto/keys changed on server.");
       return;
     }
 
     // New keys don't differ from old keys; persist timestamp and move on.
     Logger.trace(LOG_TAG, "Fetched keys are the same as persisted keys; persisting only last modified.");
     session.config.setCollectionKeys(oldKeys);
     pck.persistLastModified(response.normalizedWeaveTimestamp());
     doAdvance();
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/FetchMetaGlobalStage.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/FetchMetaGlobalStage.java
@@ -74,17 +74,17 @@ public class FetchMetaGlobalStage extend
   @Override
   public void execute() throws NoSuchStageException {
     InfoCollections infoCollections = session.config.infoCollections;
     if (infoCollections == null) {
       telemetryStageCollector.finished = SystemClock.elapsedRealtime();
       telemetryStageCollector.error = new TelemetryCollector
               .StageErrorBuilder(TELEMETRY_ERROR_NAME, TELEMETRY_ERROR_NO_INFO_COLLECTIONS)
               .build();
-      session.abort(null, "No info/collections set in FetchMetaGlobalStage.");
+      session.abort(new IllegalStateException(), "No info/collections set in FetchMetaGlobalStage.");
       return;
     }
 
     final long lastModified = session.config.persistedMetaGlobal().lastModified();
     if (!infoCollections.updateNeeded(META_COLLECTION, lastModified)) {
       // Try to use our local collection keys for this session.
       Logger.info(LOG_TAG, "Trying to use persisted meta/global for this session.");
       MetaGlobal global = session.config.persistedMetaGlobal().metaGlobal(session.config.metaURL(), session.getAuthHeaderProvider());
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/telemetry/TelemetryCollector.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/telemetry/TelemetryCollector.java
@@ -77,30 +77,27 @@ public class TelemetryCollector {
                     ));
         } catch (UnsupportedEncodingException | NoSuchAlgorithmException e) {
             // Should not happen.
             Log.e(LOG_TAG, "Either UTF-8 or SHA-256 are not supported", e);
         }
     }
 
     public void setError(@NonNull String name, @NonNull Exception e) {
-        setError(name, e.getClass().getSimpleName());
-    }
-
-    public void setError(@NonNull String name, @NonNull String details) {
-        setError(name, details, null);
+        setError(name, e, null);
     }
 
-    public void setError(@NonNull String name, @NonNull String details, @Nullable Exception e) {
+    public void setError(@NonNull String name, @NonNull Exception e, @Nullable String details) {
         final ExtendedJSONObject error = new ExtendedJSONObject();
         error.put("name", name);
-        if (e != null) {
-            error.put("error", e.getClass().getSimpleName() + ":" + details);
+        final String exceptionName = e.getClass().getSimpleName();
+        if (details != null) {
+            error.put("error", exceptionName + ":" + details);
         } else {
-            error.put("error", details);
+            error.put("error", exceptionName);
         }
         this.error = error;
     }
 
     public boolean hasError() {
         return this.error != null;
     }
 
--- a/mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/sync/TestResetting.java
+++ b/mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/sync/TestResetting.java
@@ -177,17 +177,17 @@ public class TestResetting extends Andro
     return new DefaultGlobalSessionCallback() {
 
       @Override
       public void handleAborted(GlobalSession globalSession, String reason) {
         performNotify(new Exception("Aborted"));
       }
 
       @Override
-      public void handleError(GlobalSession globalSession, Exception ex) {
+      public void handleError(GlobalSession globalSession, Exception ex, String reason) {
         performNotify(ex);
       }
     };
   }
 
   private static void assertConfigTimestampsGreaterThan(SynchronizerConfiguration config, long local, long remote) {
     assertTrue(local <= config.localBundle.getTimestamp());
     assertTrue(remote <= config.remoteBundle.getTimestamp());
--- a/mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/testhelpers/DefaultGlobalSessionCallback.java
+++ b/mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/testhelpers/DefaultGlobalSessionCallback.java
@@ -39,20 +39,16 @@ public class DefaultGlobalSessionCallbac
 
   }
 
   @Override
   public void handleAborted(GlobalSession globalSession, String reason) {
   }
 
   @Override
-  public void handleError(GlobalSession globalSession, Exception ex) {
-  }
-
-  @Override
   public void handleError(GlobalSession globalSession, Exception ex, String reason) {
   }
 
   @Override
   public void handleSuccess(GlobalSession globalSession) {
   }
 
   @Override
--- a/mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/TestResetCommands.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/TestResetCommands.java
@@ -136,17 +136,17 @@ public class TestResetCommands {
     return new DefaultGlobalSessionCallback() {
 
       @Override
       public void handleAborted(GlobalSession globalSession, String reason) {
         performNotify(new Exception("Aborted"));
       }
 
       @Override
-      public void handleError(GlobalSession globalSession, Exception ex) {
+      public void handleError(GlobalSession globalSession, Exception ex, String reason) {
         performNotify(ex);
       }
 
       @Override
       public void handleSuccess(GlobalSession globalSession) {
       }
     };
   }
--- a/mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/helpers/MockGlobalSessionCallback.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/helpers/MockGlobalSessionCallback.java
@@ -49,21 +49,16 @@ public class MockGlobalSessionCallback i
   @Override
   public void handleAborted(GlobalSession globalSession, String reason) {
     this.calledAborted = true;
     this.testWaiter().performNotify();
   }
 
   @Override
   public void handleError(GlobalSession globalSession, Exception ex, String reason) {
-    this.handleError(globalSession, ex);
-  }
-
-  @Override
-  public void handleError(GlobalSession globalSession, Exception ex) {
     this.calledError = true;
     this.calledErrorException = ex;
     this.testWaiter().performNotify();
   }
 
   @Override
   public void handleIncompleteStage(Stage currentState,
                                     GlobalSession globalSession) {
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/testhelpers/DefaultGlobalSessionCallback.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/testhelpers/DefaultGlobalSessionCallback.java
@@ -31,20 +31,16 @@ public class DefaultGlobalSessionCallbac
   public void handleAborted(GlobalSession globalSession, String reason) {
   }
 
   @Override
   public void handleError(GlobalSession globalSession, Exception ex, String reason) {
   }
 
   @Override
-  public void handleError(GlobalSession globalSession, Exception ex) {
-  }
-
-  @Override
   public void handleSuccess(GlobalSession globalSession) {
   }
 
   @Override
   public void handleStageCompleted(Stage currentState,
                                    GlobalSession globalSession) {
   }
 
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/telemetry/TelemetryCollectorTest.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/telemetry/TelemetryCollectorTest.java
@@ -137,48 +137,31 @@ public class TelemetryCollectorTest {
         collector.setFinished(2L);
 
         // Test that we can build without setting an error.
         assertFalse(collector.hasError());
         Bundle data = collector.build();
         assertFalse(data.containsKey("error"));
 
         // Test various ways to set an error.
-        // Just details.
-        collector.setError("testError", "unexpectedStuff");
-        data = collector.build();
-        assertTrue(data.containsKey("error"));
-        JSONObject errorJson = (JSONObject) data.getSerializable("error");
-        assertEquals("testError", errorJson.get("name"));
-        assertEquals("unexpectedStuff", errorJson.get("error"));
-        assertTrue(collector.hasError());
-
         // Just exception.
         collector.setError("exceptionTest", new IllegalArgumentException());
         data = collector.build();
         assertTrue(data.containsKey("error"));
-        errorJson = (JSONObject) data.getSerializable("error");
+        JSONObject errorJson = (JSONObject) data.getSerializable("error");
         assertEquals("exceptionTest", errorJson.get("name"));
         assertEquals("IllegalArgumentException", errorJson.get("error"));
 
         // Details and exception.
-        collector.setError("anotherTest", "Error details", new ConcurrentModificationException());
+        collector.setError("anotherTest", new ConcurrentModificationException(), "Error details");
         data = collector.build();
         assertTrue(data.containsKey("error"));
         errorJson = (JSONObject) data.getSerializable("error");
         assertEquals("anotherTest", errorJson.get("name"));
         assertEquals("ConcurrentModificationException:Error details", errorJson.get("error"));
-
-        // Details and explicit null exception.
-        collector.setError("noExceptionTest", "Error details", null);
-        data = collector.build();
-        assertTrue(data.containsKey("error"));
-        errorJson = (JSONObject) data.getSerializable("error");
-        assertEquals("noExceptionTest", errorJson.get("name"));
-        assertEquals("Error details", errorJson.get("error"));
     }
 
     @Test
     public void testRestarted() throws Exception {
         collector.setStarted(5L);
         collector.setFinished(10L);
 
         Bundle data = collector.build();