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
--- 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);