Bug 1370221 - Don't try to serialize ExtendedJSONObject r=nalexander
authorGrigory Kruglov <gkruglov@mozilla.com>
Tue, 06 Jun 2017 14:15:31 -0400
changeset 410894 61f5e3d8517c96ce70ec0cb5539dfd3b03f80a02
parent 410893 afcfa17be8acdd3d1b88b2bf8d6deffb53c52af1
child 410895 0fbdafd2214dee63b4bf31c7891ed8c075c23cb5
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander
bugs1370221
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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();