Bug 1460400 - Don't enable dynamic builtin events on registration. r?chutten draft
authorJan-Erik Rediger <jrediger@mozilla.com>
Mon, 14 May 2018 12:31:30 +0200
changeset 794677 f09f0c23857a07d487c097055274285de83ef04c
parent 794618 87cc179438941649616131ed9bb0c6f0766c8028
push id109759
push userbmo:jrediger@mozilla.com
push dateMon, 14 May 2018 10:39:14 +0000
reviewerschutten
bugs1460400
milestone62.0a1
Bug 1460400 - Don't enable dynamic builtin events on registration. r?chutten This makes sure dynamic builtin events follow the same semantics as static builtin events. On registration of the event the category is stored, but not enabled. For fully-dynamic events, e.g. those registered by addons, the category is enabled immediately (and can't be disabled). This removes now-unused type definitions and switches from a map to a simple set to store the category names. The value stored in the map previously was not used at all. In theory the map was effectively immutable after initialization, but the check was only forced in debug anyway. Now the set is mutable, but is only mutated in exactly 2 places. MozReview-Commit-ID: 8tLEVXzHuHw
toolkit/components/telemetry/TelemetryEvent.cpp
toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js
--- a/toolkit/components/telemetry/TelemetryEvent.cpp
+++ b/toolkit/components/telemetry/TelemetryEvent.cpp
@@ -114,19 +114,16 @@ const uint32_t kMaxExtraValueByteLength 
 const uint32_t kMaxMethodNameByteLength = 20;
 // Maximum length of dynamic object names, in UTF8 byte sequence length.
 const uint32_t kMaxObjectNameByteLength = 20;
 // Maximum length of extra key names, in UTF8 byte sequence length.
 const uint32_t kMaxExtraKeyNameByteLength = 15;
 // The maximum number of valid extra keys for an event.
 const uint32_t kMaxExtraKeyCount = 10;
 
-typedef nsDataHashtable<nsCStringHashKey, uint32_t> StringUintMap;
-typedef nsClassHashtable<nsCStringHashKey, nsCString> StringMap;
-
 struct EventKey {
   uint32_t id;
   bool dynamic;
 };
 
 struct DynamicEventInfo {
   DynamicEventInfo(const nsACString& category, const nsACString& method,
                    const nsACString& object, nsTArray<nsCString>& extra_keys,
@@ -307,18 +304,18 @@ namespace {
 bool gInitDone = false;
 
 bool gCanRecordBase;
 bool gCanRecordExtended;
 
 // The EventName -> EventKey cache map.
 nsClassHashtable<nsCStringHashKey, EventKey> gEventNameIDMap(kEventCount);
 
-// The CategoryName -> CategoryID cache map.
-StringUintMap gCategoryNameIDMap;
+// The CategoryName set.
+nsTHashtable<nsCStringHashKey> gCategoryNames;
 
 // This tracks the IDs of the categories for which recording is enabled.
 nsTHashtable<nsCStringHashKey> gEnabledCategories;
 
 // The main event storage. Events are inserted here, keyed by process id and
 // in recording order.
 typedef nsUint32HashKey ProcessIDHashKey;
 typedef nsTArray<EventRecord> EventRecordArray;
@@ -541,17 +538,17 @@ ShouldRecordChildEvent(const StaticMutex
   }
 
   return RecordEventResult::Ok;
 }
 
 void
 RegisterEvents(const StaticMutexAutoLock& lock, const nsACString& category,
                const nsTArray<DynamicEventInfo>& eventInfos,
-               const nsTArray<bool>& eventExpired)
+               const nsTArray<bool>& eventExpired, bool aBuiltin)
 {
   MOZ_ASSERT(eventInfos.Length() == eventExpired.Length(), "Event data array sizes should match.");
 
   // Register the new events.
   if (!gDynamicEventInfo) {
     gDynamicEventInfo = new nsTArray<DynamicEventInfo>();
   }
 
@@ -568,18 +565,26 @@ RegisterEvents(const StaticMutexAutoLock
       continue;
     }
 
     gDynamicEventInfo->AppendElement(eventInfos[i]);
     uint32_t eventId = eventExpired[i] ? kExpiredEventId : gDynamicEventInfo->Length() - 1;
     gEventNameIDMap.Put(eventName, new EventKey{eventId, true});
   }
 
-  // Now after successful registration enable recording for this category.
-  gEnabledCategories.PutEntry(category);
+  // If it is a builtin, add the category name in order to enable it later.
+  if (aBuiltin) {
+    gCategoryNames.PutEntry(category);
+  }
+
+  if (!aBuiltin) {
+    // Now after successful registration enable recording for this category
+    // (if not a dynamic builtin).
+    gEnabledCategories.PutEntry(category);
+  }
 }
 
 } // anonymous namespace
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
 //
 // PRIVATE: thread-unsafe helpers for event handling.
@@ -727,39 +732,33 @@ TelemetryEvent::InitializeGlobalState(bo
     // If this event is expired or not recorded in this process, mark it with
     // a special event id.
     // This avoids doing repeated checks at runtime.
     if (IsExpiredVersion(info.common_info.expiration_version().get())) {
       eventId = kExpiredEventId;
     }
 
     gEventNameIDMap.Put(UniqueEventName(info), new EventKey{eventId, false});
-    if (!gCategoryNameIDMap.Contains(info.common_info.category())) {
-      gCategoryNameIDMap.Put(info.common_info.category(),
-                             info.common_info.category_offset);
-    }
+    gCategoryNames.PutEntry(info.common_info.category());
   }
 
-#ifdef DEBUG
-  gCategoryNameIDMap.MarkImmutable();
-#endif
   gInitDone = true;
 }
 
 void
 TelemetryEvent::DeInitializeGlobalState()
 {
   StaticMutexAutoLock locker(gTelemetryEventsMutex);
   MOZ_ASSERT(gInitDone);
 
   gCanRecordBase = false;
   gCanRecordExtended = false;
 
   gEventNameIDMap.Clear();
-  gCategoryNameIDMap.Clear();
+  gCategoryNames.Clear();
   gEnabledCategories.Clear();
   gEventRecords.Clear();
   gBuiltinEventRecords.Clear();
 
   gDynamicEventInfo = nullptr;
 
   gInitDone = false;
 }
@@ -1108,17 +1107,17 @@ TelemetryEvent::RegisterEvents(const nsA
         newEventInfos.AppendElement(info);
         newEventExpired.AppendElement(expired);
       }
     }
   }
 
   {
     StaticMutexAutoLock locker(gTelemetryEventsMutex);
-    RegisterEvents(locker, aCategory, newEventInfos, newEventExpired);
+    RegisterEvents(locker, aCategory, newEventInfos, newEventExpired, aBuiltin);
   }
 
   return NS_OK;
 }
 
 nsresult
 TelemetryEvent::CreateSnapshots(uint32_t aDataset, bool aClear, JSContext* cx,
                                 uint8_t optional_argc, JS::MutableHandleValue aResult)
@@ -1216,18 +1215,17 @@ TelemetryEvent::ClearEvents()
   gBuiltinEventRecords.Clear();
 }
 
 void
 TelemetryEvent::SetEventRecordingEnabled(const nsACString& category, bool enabled)
 {
   StaticMutexAutoLock locker(gTelemetryEventsMutex);
 
-  uint32_t categoryId;
-  if (!gCategoryNameIDMap.Get(category, &categoryId)) {
+  if (!gCategoryNames.Contains(category)) {
     LogToBrowserConsole(nsIScriptError::warningFlag,
                         NS_LITERAL_STRING("Unkown category for SetEventRecordingEnabled."));
     return;
   }
 
   if (enabled) {
     gEnabledCategories.PutEntry(category);
   } else {
@@ -1259,21 +1257,17 @@ TelemetryEvent::SizeOfIncludingThis(mozi
   n += getSizeOfRecords(gEventRecords);
   n += getSizeOfRecords(gBuiltinEventRecords);
 
   n += gEventNameIDMap.ShallowSizeOfExcludingThis(aMallocSizeOf);
   for (auto iter = gEventNameIDMap.ConstIter(); !iter.Done(); iter.Next()) {
     n += iter.Key().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
   }
 
-  n += gCategoryNameIDMap.ShallowSizeOfExcludingThis(aMallocSizeOf);
-  for (auto iter = gCategoryNameIDMap.ConstIter(); !iter.Done(); iter.Next()) {
-    n += iter.Key().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
-  }
-
+  n += gCategoryNames.ShallowSizeOfExcludingThis(aMallocSizeOf);
   n += gEnabledCategories.ShallowSizeOfExcludingThis(aMallocSizeOf);
 
   if (gDynamicEventInfo) {
     n += gDynamicEventInfo->ShallowSizeOfIncludingThis(aMallocSizeOf);
     for (auto& info : *gDynamicEventInfo) {
       n += info.SizeOfExcludingThis(aMallocSizeOf);
     }
   }
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js
@@ -73,16 +73,17 @@ add_task({
   await CommonUtils.writeJSON(DYNAMIC_EVENT_SPEC, FILE_PATH);
 
   // Start TelemetryController to trigger loading the specs.
   await TelemetryController.testReset();
   await TelemetryController.testPromiseJsProbeRegistration();
 
   // Record the events
   const TEST_EVENT_NAME = "telemetry.test.builtin";
+  Telemetry.setEventRecordingEnabled(TEST_EVENT_NAME, true);
   Telemetry.recordEvent(TEST_EVENT_NAME, "test1", "object1");
   Telemetry.recordEvent(TEST_EVENT_NAME, "test2", "object1", null,
                         {"key1": "foo", "key2": "bar"});
   Telemetry.recordEvent(TEST_EVENT_NAME, "test2", "object2", null,
                         {"key2": "bar"});
 
   // Check the values we tried to store.
   const snapshot =
@@ -123,16 +124,17 @@ add_task(async function test_dynamicBuil
     "test2": {
       methods: ["test2", "test2b"],
       objects: ["object1", "object2"],
       extra_keys: ["key1", "key2"],
     },
   });
 
   // Record some events.
+  Telemetry.setEventRecordingEnabled(TEST_EVENT_NAME, true);
   Telemetry.recordEvent(TEST_EVENT_NAME, "test1", "object1");
   Telemetry.recordEvent(TEST_EVENT_NAME, "test2", "object1", null,
                         {"key1": "foo", "key2": "bar"});
   Telemetry.recordEvent(TEST_EVENT_NAME, "test2b", "object2", null,
                         {"key2": "bar"});
   // Now check that the snapshot contains the expected data.
   let snapshot =
     Telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
@@ -145,8 +147,51 @@ add_task(async function test_dynamicBuil
   ];
   let events = snapshot.parent;
   Assert.equal(events.length, expected.length, "Should have recorded the right amount of events.");
   for (let i = 0; i < expected.length; ++i) {
     Assert.deepEqual(events[i].slice(1), expected[i],
                      "Should have recorded the expected event data.");
   }
 });
+
+add_task(async function test_dynamicBuiltinEventsDisabledByDefault() {
+  Telemetry.clearEvents();
+  Telemetry.canRecordExtended = true;
+
+  const TEST_EVENT_NAME = "telemetry.test.offbydefault";
+
+  // Register some dynamic builtin test events.
+  Telemetry.registerBuiltinEvents(TEST_EVENT_NAME, {
+    // Event with only required fields.
+    "test1": {
+      methods: ["test1"],
+      objects: ["object1"],
+    },
+  });
+
+  // Record some events.
+  // Explicitely _don't_ enable the category
+  Telemetry.recordEvent(TEST_EVENT_NAME, "test1", "object1");
+
+  // Now check that the snapshot contains the expected data.
+  let snapshot =
+    Telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
+  Assert.ok(!("parent" in snapshot), "Should not have parent events in the snapshot.");
+
+  // Now enable the category and record again
+  Telemetry.setEventRecordingEnabled(TEST_EVENT_NAME, true);
+  Telemetry.recordEvent(TEST_EVENT_NAME, "test1", "object1");
+
+  snapshot =
+    Telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
+  Assert.ok(("parent" in snapshot), "Should have parent events in the snapshot.");
+
+  let expected = [
+    [TEST_EVENT_NAME, "test1", "object1"],
+  ];
+  let events = snapshot.parent;
+  Assert.equal(events.length, expected.length, "Should have recorded the right amount of events.");
+  for (let i = 0; i < expected.length; ++i) {
+    Assert.deepEqual(events[i].slice(1), expected[i],
+                     "Should have recorded the expected event data.");
+  }
+});