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 371928 02d37af6e76fc7f8b13f62f9a7ddf82ccb8c5d45
parent 371927 28c47fbedac4b808eb93a05fe55a80317336bc40
child 371929 473bef59dcfa065110ec044a8326a3ccd78affaa
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgfritzsche
bugs1315906
milestone53.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 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]);
 });