commit 0b832d6a4b1b2c5231376456f1f616a99f503d7e
parent dbe1fb5b33518694e8e4a230a0898c8064575828
Author: Jens Stutte <jstutte@mozilla.com>
Date: Sun, 5 Oct 2025 16:43:59 +0000
Bug 1991831 - Telemetry objects should track their observers. r=toolkit-telemetry-reviewers,chutten
At least in xpcshell-tests, the order of initialization between different objects that are also observers is non-obvious.
The easiest way to cope with this is to track the observer registration status per object.
Differential Revision: https://phabricator.services.mozilla.com/D266959
Diffstat:
6 files changed, 80 insertions(+), 39 deletions(-)
diff --git a/toolkit/components/telemetry/app/TelemetryReportingPolicy.sys.mjs b/toolkit/components/telemetry/app/TelemetryReportingPolicy.sys.mjs
@@ -250,6 +250,8 @@ var TelemetryReportingPolicyImpl = {
// reporting policy tab / infobar
_notificationType: null,
+ _observerRegistered: false,
+
get _log() {
if (!this._logger) {
this._logger = Log.repository.getLoggerWithMessagePrefix(
@@ -725,7 +727,10 @@ var TelemetryReportingPolicyImpl = {
Services.prefs.addObserver(TOU_ACCEPTED_VERSION_PREF, this);
// Add the event observers.
- Services.obs.addObserver(this, "sessionstore-windows-restored");
+ if (!this._observerRegistered) {
+ Services.obs.addObserver(this, "sessionstore-windows-restored");
+ this._observerRegistered = true;
+ }
},
/**
@@ -743,7 +748,10 @@ var TelemetryReportingPolicyImpl = {
* Detach the observers that were attached during setup.
*/
_detachObservers() {
- Services.obs.removeObserver(this, "sessionstore-windows-restored");
+ if (this._observerRegistered) {
+ Services.obs.removeObserver(this, "sessionstore-windows-restored");
+ this._observerRegistered = false;
+ }
Services.prefs.removeObserver(TOU_ACCEPTED_DATE_PREF, this);
Services.prefs.removeObserver(TOU_ACCEPTED_VERSION_PREF, this);
},
diff --git a/toolkit/components/telemetry/app/TelemetryScheduler.sys.mjs b/toolkit/components/telemetry/app/TelemetryScheduler.sys.mjs
@@ -85,6 +85,7 @@ export var TelemetryScheduler = {
_schedulerInterval: 0,
_shuttingDown: true,
_isUserIdle: false,
+ _observerRegistered: false,
/**
* Initialises the scheduler and schedules the first daily/aborted session pings.
@@ -107,7 +108,10 @@ export var TelemetryScheduler = {
this._rescheduleTimeout();
lazy.idleService.addIdleObserver(this, IDLE_TIMEOUT_SECONDS);
- Services.obs.addObserver(this, "wake_notification");
+ if (!this._observerRegistered) {
+ Services.obs.addObserver(this, "wake_notification");
+ this._observerRegistered = true;
+ }
},
/**
@@ -130,7 +134,10 @@ export var TelemetryScheduler = {
}
lazy.idleService.removeIdleObserver(this, IDLE_TIMEOUT_SECONDS);
- Services.obs.removeObserver(this, "wake_notification");
+ if (this._observerRegistered) {
+ Services.obs.removeObserver(this, "wake_notification");
+ this._observerRegistered = false;
+ }
this._shuttingDown = true;
},
diff --git a/toolkit/components/telemetry/app/TelemetrySend.sys.mjs b/toolkit/components/telemetry/app/TelemetrySend.sys.mjs
@@ -738,13 +738,8 @@ export var TelemetrySendImpl = {
_tooLateToSend: false,
// Array of {url, path} awaiting flushPingSenderBatch().
_pingSenderBatch: [],
-
- OBSERVER_TOPICS: [
- TOPIC_IDLE_DAILY,
- TOPIC_QUIT_APPLICATION_GRANTED,
- TOPIC_QUIT_APPLICATION_FORCED,
- TOPIC_PROFILE_CHANGE_NET_TEARDOWN,
- ],
+ _quitObserverRegistered: false,
+ _observerRegistered: false,
OBSERVED_PREFERENCES: [
TelemetryUtils.Preferences.TelemetryEnabled,
@@ -790,8 +785,11 @@ export var TelemetrySendImpl = {
// Install the observer to detect OS shutdown early enough, so
// that we catch this before the delayed setup happens.
- Services.obs.addObserver(this, TOPIC_QUIT_APPLICATION_FORCED);
- Services.obs.addObserver(this, TOPIC_QUIT_APPLICATION_GRANTED);
+ if (!this._quitObserverRegistered) {
+ Services.obs.addObserver(this, TOPIC_QUIT_APPLICATION_FORCED);
+ Services.obs.addObserver(this, TOPIC_QUIT_APPLICATION_GRANTED);
+ this._quitObserverRegistered = true;
+ }
},
QueryInterface: ChromeUtils.generateQI(["nsISupportsWeakReference"]),
@@ -801,8 +799,11 @@ export var TelemetrySendImpl = {
this._testMode = testing;
- Services.obs.addObserver(this, TOPIC_IDLE_DAILY);
- Services.obs.addObserver(this, TOPIC_PROFILE_CHANGE_NET_TEARDOWN);
+ if (!this._observerRegistered) {
+ Services.obs.addObserver(this, TOPIC_IDLE_DAILY);
+ Services.obs.addObserver(this, TOPIC_PROFILE_CHANGE_NET_TEARDOWN);
+ this._observerRegistered = true;
+ }
this._server = Services.prefs.getStringPref(
TelemetryUtils.Preferences.Server,
@@ -910,15 +911,15 @@ export var TelemetrySendImpl = {
Services.prefs.removeObserver(pref, this);
}
- for (let topic of this.OBSERVER_TOPICS) {
- try {
- Services.obs.removeObserver(this, topic);
- } catch (ex) {
- this._log.error(
- "shutdown - failed to remove observer for " + topic,
- ex
- );
- }
+ if (this._observerRegistered) {
+ Services.obs.removeObserver(this, TOPIC_IDLE_DAILY);
+ Services.obs.removeObserver(this, TOPIC_PROFILE_CHANGE_NET_TEARDOWN);
+ this._observerRegistered = false;
+ }
+ if (this._quitObserverRegistered) {
+ Services.obs.removeObserver(this, TOPIC_QUIT_APPLICATION_FORCED);
+ Services.obs.removeObserver(this, TOPIC_QUIT_APPLICATION_GRANTED);
+ this._quitObserverRegistered = false;
}
// We can't send anymore now.
diff --git a/toolkit/components/telemetry/pings/EventPing.sys.mjs b/toolkit/components/telemetry/pings/EventPing.sys.mjs
@@ -59,6 +59,8 @@ export var TelemetryEventPing = {
_processStartTimestamp: 0,
+ _observerRegistered: false,
+
get dataset() {
return Services.telemetry.canRecordPrereleaseData
? Ci.nsITelemetry.DATASET_PRERELEASE_CHANNELS
@@ -74,7 +76,10 @@ export var TelemetryEventPing = {
(Date.now() - TelemetryUtils.monotonicNow()) / MS_IN_A_MINUTE
) * MS_IN_A_MINUTE;
- Services.obs.addObserver(this, EVENT_LIMIT_REACHED_TOPIC);
+ if (!this._observerRegistered) {
+ Services.obs.addObserver(this, EVENT_LIMIT_REACHED_TOPIC);
+ this._observerRegistered = true;
+ }
XPCOMUtils.defineLazyPreferenceGetter(
this,
@@ -96,7 +101,10 @@ export var TelemetryEventPing = {
this._log.trace("Shutting down.");
// removeObserver may throw, which could interrupt shutdown.
try {
- Services.obs.removeObserver(this, EVENT_LIMIT_REACHED_TOPIC);
+ if (this._observerRegistered) {
+ Services.obs.removeObserver(this, EVENT_LIMIT_REACHED_TOPIC);
+ this._observerRegistered = false;
+ }
} catch (ex) {}
this._submitPing(this.Reason.SHUTDOWN, true /* discardLeftovers */);
diff --git a/toolkit/components/telemetry/pings/TelemetrySession.sys.mjs b/toolkit/components/telemetry/pings/TelemetrySession.sys.mjs
@@ -340,6 +340,8 @@ var Impl = {
_newProfilePingSent: false,
// Keep track of the active observers
_observedTopics: new Set(),
+ _earlyObserversRegistered: false,
+ _delayedObserversRegistered: false,
addObserver(aTopic) {
Services.obs.addObserver(this, aTopic);
@@ -804,15 +806,18 @@ var Impl = {
* chrome process.
*/
attachEarlyObservers() {
- this.addObserver("sessionstore-windows-restored");
- if (AppConstants.platform === "android") {
- this.addObserver("application-background");
- }
- this.addObserver("xul-window-visible");
+ if (!this._earlyObserversRegistered) {
+ this.addObserver("sessionstore-windows-restored");
+ if (AppConstants.platform === "android") {
+ this.addObserver("application-background");
+ }
+ this.addObserver("xul-window-visible");
- // Attach the active-ticks related observers.
- this.addObserver("user-interaction-active");
- this.addObserver("user-interaction-inactive");
+ // Attach the active-ticks related observers.
+ this.addObserver("user-interaction-active");
+ this.addObserver("user-interaction-inactive");
+ this._earlyObserversRegistered = true;
+ }
},
/**
@@ -888,7 +893,10 @@ var Impl = {
this._getSessionDataObject()
);
- this.addObserver("idle-daily");
+ if (!this._delayedObserversRegistered) {
+ this.addObserver("idle-daily");
+ this._delayedObserversRegistered = true;
+ }
await Services.telemetry.gatherMemory();
Services.telemetry.asyncFetchTelemetryData(function () {});
@@ -1063,6 +1071,8 @@ var Impl = {
this._log.warn("uninstall - Failed to remove " + topic, e);
}
}
+ this._earlyObserversRegistered = false;
+ this._delayedObserversRegistered = false;
},
getPayload: function getPayload(reason, clearSubsession) {
diff --git a/toolkit/components/telemetry/pings/UpdatePing.sys.mjs b/toolkit/components/telemetry/pings/UpdatePing.sys.mjs
@@ -25,6 +25,7 @@ const UPDATE_STAGED_TOPIC = "update-staged";
*/
export var UpdatePing = {
_enabled: false,
+ _observerRegistered: false,
earlyInit() {
this._log = Log.repository.getLoggerWithMessagePrefix(
@@ -42,8 +43,11 @@ export var UpdatePing = {
return;
}
- Services.obs.addObserver(this, UPDATE_DOWNLOADED_TOPIC);
- Services.obs.addObserver(this, UPDATE_STAGED_TOPIC);
+ if (!this._observerRegistered) {
+ Services.obs.addObserver(this, UPDATE_DOWNLOADED_TOPIC);
+ Services.obs.addObserver(this, UPDATE_STAGED_TOPIC);
+ this._observerRegistered = true;
+ }
},
/**
@@ -186,7 +190,10 @@ export var UpdatePing = {
if (!this._enabled) {
return;
}
- Services.obs.removeObserver(this, UPDATE_DOWNLOADED_TOPIC);
- Services.obs.removeObserver(this, UPDATE_STAGED_TOPIC);
+ if (this._observerRegistered) {
+ Services.obs.removeObserver(this, UPDATE_DOWNLOADED_TOPIC);
+ Services.obs.removeObserver(this, UPDATE_STAGED_TOPIC);
+ this._observerRegistered = false;
+ }
},
};
