Bug 1315906 - Change JS histogram add functions so that it doesn't throw. r=gfritzsche
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Tue, 20 Dec 2016 08:32:00 +0100
changeset 452467 02d37af6e76fc7f8b13f62f9a7ddf82ccb8c5d45
parent 452466 28c47fbedac4b808eb93a05fe55a80317336bc40
child 452468 473bef59dcfa065110ec044a8326a3ccd78affaa
push id39415
push usermozilla@noorenberghe.ca
push dateWed, 21 Dec 2016 20:49:14 +0000
reviewersgfritzsche
bugs1315906
milestone53.0a1
Bug 1315906 - Change JS histogram add functions so that it doesn't throw. r=gfritzsche
toolkit/components/telemetry/TelemetryHistogram.cpp
toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -35,16 +35,17 @@ using base::BooleanHistogram;
 using base::CountHistogram;
 using base::FlagHistogram;
 using base::LinearHistogram;
 using mozilla::StaticMutex;
 using mozilla::StaticMutexAutoLock;
 using mozilla::StaticAutoPtr;
 using mozilla::Telemetry::Accumulation;
 using mozilla::Telemetry::KeyedAccumulation;
+using mozilla::Telemetry::Common::LogToBrowserConsole;
 
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
 //
 // Naming: there are two kinds of functions in this file:
 //
 // * Functions named internal_*: these can only be reached via an
@@ -1549,16 +1550,19 @@ internal_JSHistogram_Add(JSContext *cx, 
     return false;
   }
 
   Histogram *h = static_cast<Histogram*>(JS_GetPrivate(obj));
   MOZ_ASSERT(h);
   Histogram::ClassType type = h->histogram_type();
 
   JS::CallArgs args = CallArgsFromVp(argc, vp);
+  // This function should always return |undefined| and never fail but
+  // rather report failures using the console.
+  args.rval().setUndefined();
 
   if (!internal_CanRecordBase()) {
     return true;
   }
 
   uint32_t value = 0;
   mozilla::Telemetry::ID id;
   if ((type == base::CountHistogram::COUNT_HISTOGRAM) && (args.length() == 0)) {
@@ -1567,40 +1571,41 @@ internal_JSHistogram_Add(JSContext *cx, 
     value = 1;
   } else if (type == base::LinearHistogram::LINEAR_HISTOGRAM &&
       (args.length() > 0) && args[0].isString() &&
       NS_SUCCEEDED(internal_GetHistogramEnumId(h->histogram_name().c_str(), &id)) &&
       gHistograms[id].histogramType == nsITelemetry::HISTOGRAM_CATEGORICAL) {
     // For categorical histograms we allow passing a string argument that specifies the label.
     nsAutoJSString label;
     if (!label.init(cx, args[0])) {
-      JS_ReportErrorASCII(cx, "Invalid string parameter");
-      return false;
+      LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Invalid string parameter"));
+      return true;
     }
 
     nsresult rv = gHistograms[id].label_id(NS_ConvertUTF16toUTF8(label).get(), &value);
     if (NS_FAILED(rv)) {
-      JS_ReportErrorASCII(cx, "Unknown label for categorical histogram");
-      return false;
+      LogToBrowserConsole(nsIScriptError::errorFlag,
+                          NS_LITERAL_STRING("Unknown label for categorical histogram"));
+      return true;
     }
   } else {
     // All other accumulations expect one numerical argument.
     if (!args.length()) {
-      JS_ReportErrorASCII(cx, "Expected one argument");
-      return false;
+      LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Expected one argument"));
+      return true;
     }
 
     if (!(args[0].isNumber() || args[0].isBoolean())) {
-      JS_ReportErrorASCII(cx, "Not a number");
-      return false;
+      LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Not a number"));
+      return true;
     }
 
     if (!JS::ToUint32(cx, args[0], &value)) {
-      JS_ReportErrorASCII(cx, "Failed to convert argument");
-      return false;
+      LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Failed to convert argument"));
+      return true;
     }
   }
 
   {
     StaticMutexAutoLock locker(gTelemetryHistogramMutex);
     internal_Accumulate(*h, value);
   }
   return true;
@@ -1813,45 +1818,50 @@ internal_JSKeyedHistogram_Add(JSContext 
   }
 
   KeyedHistogram* keyed = static_cast<KeyedHistogram*>(JS_GetPrivate(obj));
   if (!keyed) {
     return false;
   }
 
   JS::CallArgs args = CallArgsFromVp(argc, vp);
+  // This function should always return |undefined| and never fail but
+  // rather report failures using the console.
+  args.rval().setUndefined();
   if (args.length() < 1) {
-    JS_ReportErrorASCII(cx, "Expected one argument");
-    return false;
+    LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Expected one argument"));
+    return true;
   }
 
   nsAutoJSString key;
   if (!args[0].isString() || !key.init(cx, args[0])) {
-    JS_ReportErrorASCII(cx, "Not a string");
-    return false;
+    LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Not a string"));
+    return true;
   }
 
   const uint32_t type = keyed->GetHistogramType();
 
   // If we don't have an argument for the count histogram, assume an increment of 1.
   // Otherwise, make sure to run some sanity checks on the argument.
   int32_t value = 1;
   if ((type != base::CountHistogram::COUNT_HISTOGRAM) || (args.length() == 2)) {
     if (args.length() < 2) {
-      JS_ReportErrorASCII(cx, "Expected two arguments for this histogram type");
-      return false;
+      LogToBrowserConsole(nsIScriptError::errorFlag,
+                          NS_LITERAL_STRING("Expected two arguments for this histogram type"));
+      return true;
     }
 
     if (!(args[1].isNumber() || args[1].isBoolean())) {
-      JS_ReportErrorASCII(cx, "Not a number");
-      return false;
+      LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Not a number"));
+      return true;
     }
 
     if (!JS::ToInt32(cx, args[1], &value)) {
-      return false;
+      LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Failed to convert argument"));
+      return true;
     }
   }
 
   {
     StaticMutexAutoLock locker(gTelemetryHistogramMutex);
     internal_Accumulate(*keyed, NS_ConvertUTF16toUTF8(key), value);
   }
   return true;
--- a/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
+++ b/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ -202,30 +202,34 @@ add_task(function* test_count_histogram(
 
 add_task(function* test_categorical_histogram()
 {
   let h1 = Telemetry.getHistogramById("TELEMETRY_TEST_CATEGORICAL");
   for (let v of ["CommonLabel", "Label2", "Label3", "Label3", 0, 0, 1]) {
     h1.add(v);
   }
   for (let s of ["", "Label4", "1234"]) {
-    Assert.throws(() => h1.add(s));
+    // The |add| method should not throw for unexpected values, but rather
+    // print an error message in the console.
+    h1.add(s);
   }
 
   let snapshot = h1.snapshot();
   Assert.equal(snapshot.sum, 6);
   Assert.deepEqual(snapshot.ranges, [0, 1, 2, 3]);
   Assert.deepEqual(snapshot.counts, [3, 2, 2, 0]);
 
   let h2 = Telemetry.getHistogramById("TELEMETRY_TEST_CATEGORICAL_OPTOUT");
   for (let v of ["CommonLabel", "CommonLabel", "Label4", "Label5", "Label6", 0, 1]) {
     h2.add(v);
   }
   for (let s of ["", "Label3", "1234"]) {
-    Assert.throws(() => h2.add(s));
+    // The |add| method should not throw for unexpected values, but rather
+    // print an error message in the console.
+    h2.add(s);
   }
 
   snapshot = h2.snapshot();
   Assert.equal(snapshot.sum, 7);
   Assert.deepEqual(snapshot.ranges, [0, 1, 2, 3, 4]);
   Assert.deepEqual(snapshot.counts, [3, 2, 1, 1, 0]);
 });