bug 1438335 - Clamp the samples accumulated to histograms. r?Dexter draft
authorChris H-C <chutten@mozilla.com>
Thu, 15 Feb 2018 13:32:13 -0500
changeset 757283 697e02fd8011cb553701a7e7f9f8f1fb3fcf9fec
parent 755533 9b69cc60e5848f2f8802c911fd00771b50eed41f
child 757284 a6ef7737c24e523c328ccbdd5d96e028f08ba48a
push id99742
push userbmo:chutten@mozilla.com
push dateTue, 20 Feb 2018 17:04:55 +0000
reviewersDexter
bugs1438335
milestone60.0a1
bug 1438335 - Clamp the samples accumulated to histograms. r?Dexter Internally Histograms use int, which is signed. Our API uses uint32_t, which is not. Our JS API uses ToUint32, which takes provided values modulo UINT32_MAX. This patch consolidates behaviour so that all values greater than INT_MAX will be correctly recorded in the highest bucket of a Histogram... instead of having values between INT_MAX and UINT32_MAX end up as 0s, and values above UINT32_MAX being taken modulo UINT32_MAX, but only from the JS API. MozReview-Commit-ID: CzaaNZkPEji
toolkit/components/telemetry/TelemetryHistogram.cpp
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -590,30 +590,37 @@ internal_CreateHistogramInstance(const H
   }
 
   return h;
 }
 
 nsresult
 internal_HistogramAdd(Histogram& histogram,
                       const HistogramID id,
-                      int32_t value,
+                      uint32_t value,
                       ProcessID aProcessType)
 {
   // Check if we are allowed to record the data.
   bool canRecordDataset = CanRecordDataset(gHistogramInfos[id].dataset,
                                            internal_CanRecordBase(),
                                            internal_CanRecordExtended());
   // If `histogram` is a non-parent-process histogram, then recording-enabled
   // has been checked in its owner process.
   if (!canRecordDataset ||
     (aProcessType == ProcessID::Parent && !internal_IsRecordingEnabled(id))) {
     return NS_OK;
   }
 
+  // The internal representation of a base::Histogram's buckets uses `int`.
+  // Clamp large values of `value` to be INT_MAX so they continue to be treated
+  // as large values (instead of negative ones).
+  if (value > INT_MAX) {
+    value = INT_MAX;
+  }
+
   // It is safe to add to the histogram now: the subsession histogram was already
   // cloned from this so we won't add the sample twice.
   histogram.Add(value);
 
   return NS_OK;
 }
 
 } // namespace
@@ -1175,17 +1182,24 @@ internal_JSHistogram_Add(JSContext *cx, 
       return true;
     }
 
     if (!(args[0].isNumber() || args[0].isBoolean())) {
       LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Not a number"));
       return true;
     }
 
-    if (!JS::ToUint32(cx, args[0], &value)) {
+    if (args[0].isNumber() && args[0].toNumber() > UINT32_MAX) {
+      // Clamp large numerical arguments to value's acceptable values.
+      // JS::ToUint32 will take arg[0] modulo 2^32 before returning it, which
+      // may result in a smaller final value.
+      value = UINT32_MAX;
+      LogToBrowserConsole(nsIScriptError::errorFlag,
+        NS_LITERAL_STRING("Clamped larged numeric value."));
+    } else if (!JS::ToUint32(cx, args[0], &value)) {
       LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Failed to convert argument"));
       return true;
     }
   }
 
   {
     StaticMutexAutoLock locker(gTelemetryHistogramMutex);
     internal_Accumulate(id, value);