Bug 1340542 - Move the scalar ID checks to the public API. r=gfritzsche
MozReview-Commit-ID: 1tDkVKFdaeU
--- a/toolkit/components/telemetry/TelemetryScalar.cpp
+++ b/toolkit/components/telemetry/TelemetryScalar.cpp
@@ -1399,16 +1399,21 @@ TelemetryScalar::Add(const nsACString& a
* Adds the value to the given scalar.
*
* @param aId The scalar enum id.
* @param aVal The numeric value to add to the scalar.
*/
void
TelemetryScalar::Add(mozilla::Telemetry::ScalarID aId, uint32_t aValue)
{
+ if (NS_WARN_IF(!IsValidEnumId(aId))) {
+ MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids.");
+ return;
+ }
+
StaticMutexAutoLock locker(gTelemetryScalarsMutex);
if (internal_CanRecordScalar(aId, false) != ScalarResult::Ok) {
// We can't record this scalar. Bail out.
return;
}
// Accumulate in the child process if needed.
@@ -1433,16 +1438,21 @@ TelemetryScalar::Add(mozilla::Telemetry:
* @param aId The scalar enum id.
* @param aKey The key name.
* @param aVal The numeric value to add to the scalar.
*/
void
TelemetryScalar::Add(mozilla::Telemetry::ScalarID aId, const nsAString& aKey,
uint32_t aValue)
{
+ if (NS_WARN_IF(!IsValidEnumId(aId))) {
+ MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids.");
+ return;
+ }
+
StaticMutexAutoLock locker(gTelemetryScalarsMutex);
if (internal_CanRecordScalar(aId, true) != ScalarResult::Ok) {
// We can't record this scalar. Bail out.
return;
}
// Accumulate in the child process if needed.
@@ -1537,16 +1547,21 @@ TelemetryScalar::Set(const nsACString& a
* Sets the scalar to the given numeric value.
*
* @param aId The scalar enum id.
* @param aValue The numeric, unsigned value to set the scalar to.
*/
void
TelemetryScalar::Set(mozilla::Telemetry::ScalarID aId, uint32_t aValue)
{
+ if (NS_WARN_IF(!IsValidEnumId(aId))) {
+ MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids.");
+ return;
+ }
+
StaticMutexAutoLock locker(gTelemetryScalarsMutex);
if (internal_CanRecordScalar(aId, false) != ScalarResult::Ok) {
// We can't record this scalar. Bail out.
return;
}
// Accumulate in the child process if needed.
@@ -1569,16 +1584,21 @@ TelemetryScalar::Set(mozilla::Telemetry:
* Sets the scalar to the given string value.
*
* @param aId The scalar enum id.
* @param aValue The string value to set the scalar to.
*/
void
TelemetryScalar::Set(mozilla::Telemetry::ScalarID aId, const nsAString& aValue)
{
+ if (NS_WARN_IF(!IsValidEnumId(aId))) {
+ MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids.");
+ return;
+ }
+
StaticMutexAutoLock locker(gTelemetryScalarsMutex);
if (internal_CanRecordScalar(aId, false) != ScalarResult::Ok) {
// We can't record this scalar. Bail out.
return;
}
// Accumulate in the child process if needed.
@@ -1601,16 +1621,21 @@ TelemetryScalar::Set(mozilla::Telemetry:
* Sets the scalar to the given boolean value.
*
* @param aId The scalar enum id.
* @param aValue The boolean value to set the scalar to.
*/
void
TelemetryScalar::Set(mozilla::Telemetry::ScalarID aId, bool aValue)
{
+ if (NS_WARN_IF(!IsValidEnumId(aId))) {
+ MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids.");
+ return;
+ }
+
StaticMutexAutoLock locker(gTelemetryScalarsMutex);
if (internal_CanRecordScalar(aId, false) != ScalarResult::Ok) {
// We can't record this scalar. Bail out.
return;
}
// Accumulate in the child process if needed.
@@ -1635,16 +1660,21 @@ TelemetryScalar::Set(mozilla::Telemetry:
* @param aId The scalar enum id.
* @param aKey The scalar key.
* @param aValue The numeric, unsigned value to set the scalar to.
*/
void
TelemetryScalar::Set(mozilla::Telemetry::ScalarID aId, const nsAString& aKey,
uint32_t aValue)
{
+ if (NS_WARN_IF(!IsValidEnumId(aId))) {
+ MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids.");
+ return;
+ }
+
StaticMutexAutoLock locker(gTelemetryScalarsMutex);
if (internal_CanRecordScalar(aId, true) != ScalarResult::Ok) {
// We can't record this scalar. Bail out.
return;
}
// Accumulate in the child process if needed.
@@ -1669,16 +1699,21 @@ TelemetryScalar::Set(mozilla::Telemetry:
* @param aId The scalar enum id.
* @param aKey The scalar key.
* @param aValue The boolean value to set the scalar to.
*/
void
TelemetryScalar::Set(mozilla::Telemetry::ScalarID aId, const nsAString& aKey,
bool aValue)
{
+ if (NS_WARN_IF(!IsValidEnumId(aId))) {
+ MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids.");
+ return;
+ }
+
StaticMutexAutoLock locker(gTelemetryScalarsMutex);
if (internal_CanRecordScalar(aId, true) != ScalarResult::Ok) {
// We can't record this scalar. Bail out.
return;
}
// Accumulate in the child process if needed.
@@ -1773,16 +1808,21 @@ TelemetryScalar::SetMaximum(const nsACSt
* Sets the scalar to the maximum of the current and the passed value.
*
* @param aId The scalar enum id.
* @param aValue The numeric value to set the scalar to.
*/
void
TelemetryScalar::SetMaximum(mozilla::Telemetry::ScalarID aId, uint32_t aValue)
{
+ if (NS_WARN_IF(!IsValidEnumId(aId))) {
+ MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids.");
+ return;
+ }
+
StaticMutexAutoLock locker(gTelemetryScalarsMutex);
if (internal_CanRecordScalar(aId, false) != ScalarResult::Ok) {
// We can't record this scalar. Bail out.
return;
}
// Accumulate in the child process if needed.
@@ -1807,16 +1847,21 @@ TelemetryScalar::SetMaximum(mozilla::Tel
* @param aId The scalar enum id.
* @param aKey The key name.
* @param aValue The numeric value to set the scalar to.
*/
void
TelemetryScalar::SetMaximum(mozilla::Telemetry::ScalarID aId, const nsAString& aKey,
uint32_t aValue)
{
+ if (NS_WARN_IF(!IsValidEnumId(aId))) {
+ MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids.");
+ return;
+ }
+
StaticMutexAutoLock locker(gTelemetryScalarsMutex);
if (internal_CanRecordScalar(aId, true) != ScalarResult::Ok) {
// We can't record this scalar. Bail out.
return;
}
// Accumulate in the child process if needed.
@@ -2115,16 +2160,21 @@ TelemetryScalar::UpdateChildData(GeckoPr
MOZ_ASSERT(XRE_IsParentProcess(),
"The stored child processes scalar data must be updated from the parent process.");
StaticMutexAutoLock locker(gTelemetryScalarsMutex);
if (!internal_CanRecordBase()) {
return;
}
for (auto& upd : aScalarActions) {
+ if (NS_WARN_IF(!IsValidEnumId(upd.mId))) {
+ MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids.");
+ continue;
+ }
+
if (internal_IsKeyedScalar(upd.mId)) {
continue;
}
// Are we allowed to record this scalar? We don't need to check for
// allowed processes here, that's taken care of when recording
// in child processes.
if (!internal_CanRecordForScalarID(upd.mId)) {
@@ -2201,16 +2251,21 @@ TelemetryScalar::UpdateChildKeyedData(Ge
MOZ_ASSERT(XRE_IsParentProcess(),
"The stored child processes keyed scalar data must be updated from the parent process.");
StaticMutexAutoLock locker(gTelemetryScalarsMutex);
if (!internal_CanRecordBase()) {
return;
}
for (auto& upd : aScalarActions) {
+ if (NS_WARN_IF(!IsValidEnumId(upd.mId))) {
+ MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids.");
+ continue;
+ }
+
if (!internal_IsKeyedScalar(upd.mId)) {
continue;
}
// Are we allowed to record this scalar? We don't need to check for
// allowed processes here, that's taken care of when recording
// in child processes.
if (!internal_CanRecordForScalarID(upd.mId)) {
--- a/toolkit/components/telemetry/tests/gtest/TestScalars.cpp
+++ b/toolkit/components/telemetry/tests/gtest/TestScalars.cpp
@@ -314,8 +314,49 @@ TEST_F(TelemetryTestFixture, NonMainThre
// Shutdown the thread. This also waits for the runnable to complete.
testingThread->Shutdown();
// Check the recorded value.
JS::RootedValue scalarsSnapshot(cx.GetJSContext());
GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot);
CheckUintScalar("telemetry.test.unsigned_int_kind", cx.GetJSContext(), scalarsSnapshot, 37);
}
+
+TEST_F(TelemetryTestFixture, ScalarUnknownID) {
+ AutoJSContextWithGlobal cx(mCleanGlobal);
+
+ // Make sure we don't get scalars from other tests.
+ Unused << mTelemetry->ClearScalars();
+
+// Don't run this part in debug builds as that intentionally asserts.
+#ifndef DEBUG
+ const uint32_t kTestFakeIds[] = {
+ static_cast<uint32_t>(Telemetry::ScalarID::ScalarCount),
+ static_cast<uint32_t>(Telemetry::ScalarID::ScalarCount) + 378537,
+ std::numeric_limits<uint32_t>::max()
+ };
+
+ for (auto id : kTestFakeIds) {
+ Telemetry::ScalarID scalarId = static_cast<Telemetry::ScalarID>(id);
+ Telemetry::ScalarSet(scalarId, static_cast<uint32_t>(1));
+ Telemetry::ScalarSet(scalarId, true);
+ Telemetry::ScalarSet(scalarId, NS_LITERAL_STRING("test"));
+ Telemetry::ScalarAdd(scalarId, 1);
+ Telemetry::ScalarSetMaximum(scalarId, 1);
+
+ // Make sure that nothing was recorded in the plain scalars.
+ JS::RootedValue scalarsSnapshot(cx.GetJSContext());
+ GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot);
+ ASSERT_TRUE(scalarsSnapshot.isUndefined()) << "No scalar must be recorded";
+
+ // Same for the keyed scalars.
+ Telemetry::ScalarSet(scalarId, NS_LITERAL_STRING("key1"), static_cast<uint32_t>(1));
+ Telemetry::ScalarSet(scalarId, NS_LITERAL_STRING("key1"), true);
+ Telemetry::ScalarAdd(scalarId, NS_LITERAL_STRING("key1"), 1);
+ Telemetry::ScalarSetMaximum(scalarId, NS_LITERAL_STRING("key1"), 1);
+
+ // Make sure that nothing was recorded in the keyed scalars.
+ JS::RootedValue keyedSnapshot(cx.GetJSContext());
+ GetScalarsSnapshot(true, cx.GetJSContext(), &keyedSnapshot);
+ ASSERT_TRUE(keyedSnapshot.isUndefined()) << "No keyed scalar must be recorded";
+ }
+#endif
+}