Bug 1408975 - Allow to register new addon events for an existing category. r=dexter

toolkit/components/telemetry/TelemetryEvent.cpp

@@ -168,11 +168,6 @@ enum class RecordEventResult {
   WrongProcess,
 };
 
-enum class RegisterEventResult {
-  Ok,
-  AlreadyRegistered,
-};
-
 typedef nsTArray<EventExtraEntry> ExtraArray;
 
 class EventRecord {
@@ -520,35 +515,38 @@ ShouldRecordChildEvent(const StaticMutexAutoLock& lock, const nsACString& catego
   return RecordEventResult::Ok;
 }
 
-RegisterEventResult
+void
 RegisterEvents(const StaticMutexAutoLock& lock, const nsACString& category,
                const nsTArray<DynamicEventInfo>& eventInfos,
                const nsTArray<bool>& eventExpired)
 {
   MOZ_ASSERT(eventInfos.Length() == eventExpired.Length(), "Event data array sizes should match.");
 
-  // Check that none of the events are already registered.
-  for (auto& info : eventInfos) {
-    if (gEventNameIDMap.Get(UniqueEventName(info))) {
-      return RegisterEventResult::AlreadyRegistered;
-    }
-  }
-
   // Register the new events.
   if (!gDynamicEventInfo) {
     gDynamicEventInfo = new nsTArray<DynamicEventInfo>();
   }
 
   for (uint32_t i = 0, len = eventInfos.Length(); i < len; ++i) {
+    const nsCString& eventName = UniqueEventName(eventInfos[i]);
+
+    // Re-registering events can happen when add-ons update, so we don't print warnings.
+    // We don't support changing their definition, but the expiry might have changed.
+    EventKey* existing = nullptr;
+    if (gEventNameIDMap.Get(eventName, &existing)) {
+      if (eventExpired[i]) {
+        existing->id = kExpiredEventId;
+      }
+      continue;
+    }
+
     gDynamicEventInfo->AppendElement(eventInfos[i]);
     uint32_t eventId = eventExpired[i] ? kExpiredEventId : gDynamicEventInfo->Length() - 1;
-    gEventNameIDMap.Put(UniqueEventName(eventInfos[i]), new EventKey{eventId, true});
+    gEventNameIDMap.Put(eventName, new EventKey{eventId, true});
   }
 
   // Now after successful registration enable recording for this category.
   gEnabledCategories.PutEntry(category);
-
-  return RegisterEventResult::Ok;
 }
 
 } // anonymous namespace
@@ -1080,18 +1078,9 @@ TelemetryEvent::RegisterEvents(const nsACString& aCategory,
     }
   }
 
-  RegisterEventResult res = RegisterEventResult::Ok;
   {
     StaticMutexAutoLock locker(gTelemetryEventsMutex);
-    res = ::RegisterEvents(locker, aCategory, newEventInfos, newEventExpired);
-  }
-
-  switch (res) {
-    case RegisterEventResult::AlreadyRegistered:
-      JS_ReportErrorASCII(cx, "Attempt to register event that is already registered.");
-      return NS_ERROR_INVALID_ARG;
-    default:
-      break;
+    RegisterEvents(locker, aCategory, newEventInfos, newEventExpired);
   }
 
   return NS_OK;

toolkit/components/telemetry/docs/collection/events.rst

@@ -197,6 +197,8 @@ After registration, the events can be recorded through the ``recordEvent()`` fun
 
 New events registered here are subject to the same limitations as the ones registered through ``Events.yaml``, although the naming was in parts updated to recent policy changes.
 
+When add-ons are updated, they may re-register all of their events. In that case, any changes to events that are already registered are ignored. The only exception is expiry; an event that is re-registered with ``expired: true`` will not be recorded anymore.
+
 Example:
 
 .. code-block:: js
@@ -227,3 +229,4 @@ Version History
 - Firefox 53: Event recording disabled by default (`bug 1329139 <https://bugzilla.mozilla.org/show_bug.cgi?id=1329139>`_).
 - Firefox 54: Added child process events (`bug 1313326 <https://bugzilla.mozilla.org/show_bug.cgi?id=1313326>`_).
 - Firefox 56: Added support for recording new probes from add-ons (`bug 1302681 <bug https://bugzilla.mozilla.org/show_bug.cgi?id=1302681>`_).
+- Firefox 58: Ignore re-registering existing events for a category instead of failing (`bug 1408975 <https://bugzilla.mozilla.org/show_bug.cgi?id=1408975>`_).

toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js

@@ -519,14 +519,90 @@ add_task(function* test_dynamicEventRegistrationValidation() {
       extra_keys: ["a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a10"],
     },
   });
-
-  // Test registering an event thats already registered through Events.yaml.
-  Assert.throws(() => Telemetry.registerEvents("telemetry.test", {
-      "test1": {
-        methods: ["test1"],
-        objects: ["object1"],
-      },
-    }),
-    /Attempt to register event that is already registered\./,
-    "Should throw when registering event that already was registered.");
+});
+
+// When add-ons update, they may re-register some of the dynamic events.
+// Test through some possible scenarios.
+add_task(function* test_dynamicEventRegisterAgain() {
+  Telemetry.canRecordExtended = true;
+  Telemetry.clearEvents();
+
+  const category = "telemetry.test.register.again";
+  let events = {
+    "test1": {
+      methods: ["test1"],
+      objects: ["object1"],
+    }
+  };
+
+  // First register the initial event and make sure it can be recorded.
+  Telemetry.registerEvents(category, events);
+  let expected = [
+    [category, "test1", "object1"],
+  ];
+  expected.forEach(e => Telemetry.recordEvent(...e));
+
+  let snapshot = Telemetry.snapshotEvents(OPTIN, true);
+  Assert.equal(snapshot.dynamic.length, expected.length,
+               "Should have right number of events in the snapshot.");
+  Assert.deepEqual(snapshot.dynamic.map(e => e.slice(1)), expected);
+
+  // Register the same event again and make sure it can still be recorded.
+  Telemetry.registerEvents(category, events);
+  Telemetry.recordEvent(category, "test1", "object1");
+
+  snapshot = Telemetry.snapshotEvents(OPTIN, true);
+  Assert.equal(snapshot.dynamic.length, expected.length,
+               "Should have right number of events in the snapshot.");
+  Assert.deepEqual(snapshot.dynamic.map(e => e.slice(1)), expected);
+
+  // Now register another event in the same category and make sure both events can be recorded.
+  events["test2"] = {
+    methods: ["test2"],
+    objects: ["object2"],
+  };
+  Telemetry.registerEvents(category, events);
+
+  expected = [
+    [category, "test1", "object1"],
+    [category, "test2", "object2"],
+  ];
+  expected.forEach(e => Telemetry.recordEvent(...e));
+
+  snapshot = Telemetry.snapshotEvents(OPTIN, true);
+  Assert.equal(snapshot.dynamic.length, expected.length,
+               "Should have right number of events in the snapshot.");
+  Assert.deepEqual(snapshot.dynamic.map(e => e.slice(1)), expected);
+
+  // Check that adding a new object to an event entry works.
+  events["test1"].methods = ["test1a"];
+  events["test2"].objects = ["object2", "object2a"];
+  Telemetry.registerEvents(category, events);
+
+  expected = [
+    [category, "test1", "object1"],
+    [category, "test2", "object2"],
+    [category, "test1a", "object1"],
+    [category, "test2", "object2a"],
+  ];
+  expected.forEach(e => Telemetry.recordEvent(...e));
+
+  snapshot = Telemetry.snapshotEvents(OPTIN, true);
+  Assert.equal(snapshot.dynamic.length, expected.length,
+               "Should have right number of events in the snapshot.");
+  Assert.deepEqual(snapshot.dynamic.map(e => e.slice(1)), expected);
+
+  // Make sure that we can expire events that are already registered.
+  events["test2"].expired = true;
+  Telemetry.registerEvents(category, events);
+
+  expected = [
+    [category, "test1", "object1"],
+  ];
+  expected.forEach(e => Telemetry.recordEvent(...e));
+
+  snapshot = Telemetry.snapshotEvents(OPTIN, true);
+  Assert.equal(snapshot.dynamic.length, expected.length,
+               "Should have right number of events in the snapshot.");
+  Assert.deepEqual(snapshot.dynamic.map(e => e.slice(1)), expected);
 });