Bug 1370221 - Don't try to serialize ExtendedJSONObject r=nalexander draft
authorGrigory Kruglov <gkruglov@mozilla.com>
Tue, 06 Jun 2017 14:15:31 -0400
changeset 589878 483dca7fe06acf1fbdcccfe9b9282041eeab2bf7
parent 589877 c838bec2e2c6ae67c9baccb083af90530a872d58
child 590597 c7335b82b19e947b95e908faab93520933f17c26
push id62554
push userbmo:gkruglov@mozilla.com
push dateWed, 07 Jun 2017 01:06:43 +0000
reviewersnalexander
bugs1370221
milestone55.0a1
Bug 1370221 - Don't try to serialize ExtendedJSONObject r=nalexander MozReview-Commit-ID: 3Q4rD2Ljfc
mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetrySyncPingBuilder.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/telemetry/TelemetryCollector.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/telemetry/TelemetryCollectorTest.java
--- a/mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetrySyncPingBuilder.java
+++ b/mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetrySyncPingBuilder.java
@@ -6,16 +6,17 @@
 
 package org.mozilla.gecko.telemetry.pingbuilders;
 
 import android.os.Bundle;
 import android.os.Parcelable;
 import android.support.annotation.NonNull;
 
 import org.json.simple.JSONArray;
+import org.json.simple.JSONObject;
 import org.mozilla.gecko.sync.ExtendedJSONObject;
 import org.mozilla.gecko.sync.telemetry.TelemetryContract;
 import org.mozilla.gecko.sync.telemetry.TelemetryStageCollector;
 import org.mozilla.gecko.telemetry.TelemetryLocalPing;
 
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -126,17 +127,17 @@ public class TelemetrySyncPingBuilder ex
 
         if (devicesJSON.size() > 0) {
             payload.put("devices", devicesJSON);
         }
         return this;
     }
 
     public TelemetrySyncPingBuilder setError(@NonNull Serializable error) {
-        payload.put("failureReason", castErrorObject(error));
+        payload.put("failureReason", new ExtendedJSONObject((JSONObject) error));
         return this;
     }
 
     public TelemetrySyncPingBuilder setTook(long took) {
         payload.put("took", took);
         return this;
     }
 
@@ -154,13 +155,9 @@ public class TelemetrySyncPingBuilder ex
     /**
      * We broadcast this data via LocalBroadcastManager and control both sides of this code, so it
      * is acceptable to do an unchecked cast.
      */
     @SuppressWarnings("unchecked")
     private static HashMap<String, TelemetryStageCollector> castSyncData(final Serializable data) {
         return (HashMap<String, TelemetryStageCollector>) data;
     }
-
-    private static ExtendedJSONObject castErrorObject(final Serializable error) {
-        return (ExtendedJSONObject) error;
-    }
 }
--- 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
@@ -2,30 +2,28 @@
  * 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.telemetry;
 
 import android.os.Bundle;
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
-import android.support.annotation.VisibleForTesting;
 import android.util.Log;
 
 import org.mozilla.gecko.sync.CollectionConcurrentModificationException;
 import org.mozilla.gecko.sync.ExtendedJSONObject;
 import org.mozilla.gecko.sync.HTTPFailureException;
 import org.mozilla.gecko.sync.SyncDeadlineReachedException;
 import org.mozilla.gecko.sync.Utils;
 import org.mozilla.gecko.sync.net.SyncStorageResponse;
 import org.mozilla.gecko.sync.repositories.FetchFailedException;
 import org.mozilla.gecko.sync.repositories.StoreFailedException;
 import org.mozilla.gecko.sync.repositories.domain.ClientRecord;
 
-import java.io.Serializable;
 import java.io.UnsupportedEncodingException;
 import java.security.NoSuchAlgorithmException;
 import java.util.ArrayList;
 import java.util.HashMap;
 
 /**
  * Gathers telemetry about a single run of sync.
  * In light of sync restarts, rarely a "single sync" will actually include more than one sync.
@@ -41,17 +39,17 @@ public class TelemetryCollector {
 
     // Telemetry collected by individual stages is aggregated here. Stages run sequentially,
     // and only access their own collectors.
     private final HashMap<String, TelemetryStageCollector> stageCollectors = new HashMap<>();
 
     // Data which is not specific to a single stage is aggregated in this object.
     // It's possible that these fields are read/written from different threads.
     // Volatile is used to ensure memory visibility.
-    @VisibleForTesting protected volatile ExtendedJSONObject error;
+    private volatile ExtendedJSONObject error;
     private volatile String hashedUID;
     private volatile String hashedDeviceID;
     private final ArrayList<Bundle> devices = new ArrayList<>();
 
     @Nullable private volatile Long started;
     @Nullable private volatile Long finished;
 
     private volatile boolean didRestart = false;
@@ -148,17 +146,19 @@ public class TelemetryCollector {
 
         final long took = this.finished - this.started;
 
         final Bundle telemetry = new Bundle();
         telemetry.putString(TelemetryContract.KEY_LOCAL_UID, this.hashedUID);
         telemetry.putString(TelemetryContract.KEY_LOCAL_DEVICE_ID, this.hashedDeviceID);
         telemetry.putParcelableArrayList(TelemetryContract.KEY_DEVICES, this.devices);
         telemetry.putLong(TelemetryContract.KEY_TOOK, took);
-        telemetry.putSerializable(TelemetryContract.KEY_ERROR, (Serializable) this.error);
+        if (this.error != null) {
+            telemetry.putSerializable(TelemetryContract.KEY_ERROR, this.error.object);
+        }
         telemetry.putSerializable(TelemetryContract.KEY_STAGES, this.stageCollectors);
         if (this.didRestart) {
             telemetry.putBoolean(TelemetryContract.KEY_RESTARTED, true);
         }
         return telemetry;
     }
 
     /**
--- 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
@@ -1,30 +1,32 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 package org.mozilla.gecko.sync.telemetry;
 
 import android.os.Bundle;
 
+import org.json.simple.JSONObject;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mozilla.gecko.background.testhelpers.TestRunner;
 import org.mozilla.gecko.sync.CollectionConcurrentModificationException;
 import org.mozilla.gecko.sync.ExtendedJSONObject;
 import org.mozilla.gecko.sync.HTTPFailureException;
 import org.mozilla.gecko.sync.SyncDeadlineReachedException;
 import org.mozilla.gecko.sync.Utils;
 import org.mozilla.gecko.sync.net.SyncStorageResponse;
 import org.mozilla.gecko.sync.repositories.FetchFailedException;
 import org.mozilla.gecko.sync.repositories.StoreFailedException;
 import org.mozilla.gecko.sync.repositories.domain.ClientRecord;
 
 import java.util.ArrayList;
+import java.util.ConcurrentModificationException;
 
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.doReturn;
 
 import static org.junit.Assert.*;
 
 @RunWith(TestRunner.class)
 public class TelemetryCollectorTest {
@@ -126,19 +128,57 @@ public class TelemetryCollectorTest {
     @Test(expected = IllegalStateException.class)
     public void testDurationMissingFinished() throws Exception {
         collector.setStarted(10L);
         collector.build();
     }
 
     @Test
     public void testError() throws Exception {
+        collector.setStarted(1L);
+        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");
-        assertEquals("testError", collector.error.getString("name"));
-        assertEquals("unexpectedStuff", collector.error.getString("error"));
+        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");
+        assertEquals("exceptionTest", errorJson.get("name"));
+        assertEquals("IllegalArgumentException", errorJson.get("error"));
+
+        // Details and exception.
+        collector.setError("anotherTest", "Error details", new ConcurrentModificationException());
+        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();