Bug 1340542 - Move the scalar ID checks to the public API. r=gfritzsche draft
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Wed, 22 Feb 2017 11:57:04 +0100
changeset 490916 4889ea309fc068b8f9eee2d6f72a954c56509830
parent 487877 9a9db410f20781076424307265decbc5de1e94cc
child 547420 b36cf738dbe171dee9f9377e38039461f0c3adc2
push id47273
push useralessio.placitelli@gmail.com
push dateWed, 01 Mar 2017 14:54:46 +0000
reviewersgfritzsche
bugs1340542
milestone54.0a1
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
+}