diff --git a/toolkit/components/telemetry/TelemetryEvent.cpp b/toolkit/components/telemetry/TelemetryEvent.cpp index 668336c4ddc6..efc9f8d86a75 100644 --- a/toolkit/components/telemetry/TelemetryEvent.cpp +++ b/toolkit/components/telemetry/TelemetryEvent.cpp @@ -168,11 +168,6 @@ enum class RecordEventResult { WrongProcess, }; -enum class RegisterEventResult { - Ok, - AlreadyRegistered, -}; - typedef nsTArray 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& eventInfos, const nsTArray& 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(); } 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; diff --git a/toolkit/components/telemetry/docs/collection/events.rst b/toolkit/components/telemetry/docs/collection/events.rst index a5beb47d87ce..5141e2bd650f 100644 --- a/toolkit/components/telemetry/docs/collection/events.rst +++ b/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 `_). - Firefox 54: Added child process events (`bug 1313326 `_). - Firefox 56: Added support for recording new probes from add-ons (`bug 1302681 `_). +- Firefox 58: Ignore re-registering existing events for a category instead of failing (`bug 1408975 `_). diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js index d1fa4c8e583e..4445dc67910f 100644 --- a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js +++ b/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); });