Bug 1340542 - Move the scalar ID checks to the public API. r=gfritzsche
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Wed, 22 Feb 2017 11:57:04 +0100
changeset 392048 578d891011c317df2bdfa315791f765394c9302c
parent 392047 f8194edb8d411e9caab1d26cd283a9791c978c61
child 392049 093c7e787c9b1523b3364fdaac7a4481106002ca
push id7198
push userjlorenzo@mozilla.com
push dateTue, 18 Apr 2017 12:07:49 +0000
treeherdermozilla-beta@d57aa49c3948 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgfritzsche
bugs1340542
milestone54.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 1340542 - Move the scalar ID checks to the public API. r=gfritzsche MozReview-Commit-ID: 1tDkVKFdaeU
toolkit/components/telemetry/TelemetryScalar.cpp
toolkit/components/telemetry/tests/gtest/TestScalars.cpp
--- 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
+}