Bug 1359801 - Improve keeping track of observer topics in TelemetrySession. r=gfritzsche
authorAlejandro Rodriguez <alexrs95@gmail.com>
Mon, 29 May 2017 22:06:09 +0200
changeset 409374 99a694a8b9a8654474d93e2a15e70e04eca917d0
parent 409373 c95cd85e640081adf0cbe047a41bc45478bbf3d5
child 409375 fa5c6debadd31d7c3bec114849667dcd885a3a9c
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgfritzsche
bugs1359801
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1359801 - Improve keeping track of observer topics in TelemetrySession. r=gfritzsche
toolkit/components/telemetry/TelemetrySession.jsm
--- a/toolkit/components/telemetry/TelemetrySession.jsm
+++ b/toolkit/components/telemetry/TelemetrySession.jsm
@@ -669,19 +669,16 @@ this.TelemetrySession = Object.freeze({
 });
 
 var Impl = {
   _histograms: {},
   _initialized: false,
   _logger: null,
   _prevValues: {},
   _slowSQLStartup: {},
-  _hasWindowRestoredObserver: false,
-  _hasXulWindowVisibleObserver: false,
-  _hasActiveTicksObservers: false,
   // The activity state for the user. If false, don't count the next
   // active tick. Otherwise, increment the active ticks as usual.
   _isUserActive: true,
   _startupIO: {},
   // The previous build ID, if this is the first run with a new build.
   // Null if this is the first run, or the previous build ID is unknown.
   _previousBuildId: null,
   // Telemetry payloads sent by child processes.
@@ -734,16 +731,28 @@ var Impl = {
   _childrenToHearFrom: null,
   // monotonically-increasing id for USS reports
   _nextTotalMemoryId: 1,
   _USSFromChildProcesses: null,
   _lastEnvironmentChangeDate: 0,
   // We save whether the "new-profile" ping was sent yet, to
   // survive profile refresh and migrations.
   _newProfilePingSent: false,
+  // Keep track of the active observers
+  _observedTopics: new Set(),
+
+  addObserver(aTopic) {
+    Services.obs.addObserver(this, aTopic)
+    this._observedTopics.add(aTopic)
+  },
+
+  removeObserver(aTopic) {
+    Services.obs.removeObserver(this, aTopic)
+    this._observedTopics.delete(aTopic)
+  },
 
   get _log() {
     if (!this._logger) {
       this._logger = Log.repository.getLoggerWithMessagePrefix(LOGGER_NAME, LOGGER_PREFIX);
     }
     return this._logger;
   },
 
@@ -1443,51 +1452,36 @@ var Impl = {
     return TelemetryController.submitExternalPing(getPingType(payload), payload, options);
   },
 
   /**
    * Attaches the needed observers during Telemetry early init, in the
    * chrome process.
    */
   attachEarlyObservers() {
-    Services.obs.addObserver(this, "sessionstore-windows-restored");
+    this.addObserver("sessionstore-windows-restored");
     if (AppConstants.platform === "android") {
-      Services.obs.addObserver(this, "application-background");
+      this.addObserver("application-background");
     }
-    Services.obs.addObserver(this, "xul-window-visible");
-    this._hasWindowRestoredObserver = true;
-    this._hasXulWindowVisibleObserver = true;
+    this.addObserver("xul-window-visible");
 
     // Attach the active-ticks related observers.
-    Services.obs.addObserver(this, "user-interaction-active");
-    Services.obs.addObserver(this, "user-interaction-inactive");
-    this._hasActiveTicksObservers = true;
+    this.addObserver("user-interaction-active");
+    this.addObserver("user-interaction-inactive");
   },
 
   attachObservers: function attachObservers() {
     if (!this._initialized)
       return;
-    Services.obs.addObserver(this, "idle-daily");
+    this.addObserver("idle-daily");
     if (Telemetry.canRecordExtended) {
-      Services.obs.addObserver(this, TOPIC_CYCLE_COLLECTOR_BEGIN);
+      this.addObserver(TOPIC_CYCLE_COLLECTOR_BEGIN);
     }
   },
 
-  detachObservers: function detachObservers() {
-    if (!this._initialized)
-      return;
-    Services.obs.removeObserver(this, "idle-daily");
-    try {
-      // Tests may flip Telemetry.canRecordExtended on and off. Just try to remove this
-      // observer and catch if it fails because the observer was not added.
-      Services.obs.removeObserver(this, TOPIC_CYCLE_COLLECTOR_BEGIN);
-    } catch (e) {
-      this._log.warn("detachObservers - Failed to remove " + TOPIC_CYCLE_COLLECTOR_BEGIN, e);
-    }
-  },
 
   /**
    * Lightweight init function, called as soon as Firefox starts.
    */
   earlyInit(testing) {
     this._log.trace("earlyInit");
 
     this._initStarted = true;
@@ -1611,17 +1605,17 @@ var Impl = {
     this._log.trace("setupContentProcess");
     this._testing = testing;
 
     if (!Telemetry.canRecordBase) {
       this._log.trace("setupContentProcess - base recording is disabled, not initializing");
       return;
     }
 
-    Services.obs.addObserver(this, "content-child-shutdown");
+    this.addObserver("content-child-shutdown");
     cpml.addMessageListener(MESSAGE_TELEMETRY_GET_CHILD_THREAD_HANGS, this);
     cpml.addMessageListener(MESSAGE_TELEMETRY_GET_CHILD_USS, this);
 
     let delayedTask = new DeferredTask(() => {
       this._initialized = true;
 
       this.attachObservers();
       this.gatherMemory();
@@ -1851,34 +1845,27 @@ var Impl = {
       overwrite: true,
     };
     return TelemetryController.addPendingPing(getPingType(payload), payload, options);
   },
 
   /**
    * Do some shutdown work that is common to all process types.
    */
-  uninstall: function uninstall() {
-    this.detachObservers();
-    if (this._hasWindowRestoredObserver) {
-      Services.obs.removeObserver(this, "sessionstore-windows-restored");
-      this._hasWindowRestoredObserver = false;
+  uninstall() {
+    for (let topic of this._observedTopics) {
+      try {
+        // Tests may flip Telemetry.canRecordExtended on and off. It can be the case
+        // that the observer TOPIC_CYCLE_COLLECTOR_BEGIN was not added.
+        this.removeObserver(topic);
+      } catch (e) {
+        this._log.warn("uninstall - Failed to remove " + topic, e);
+      }
     }
-    if (this._hasXulWindowVisibleObserver) {
-      Services.obs.removeObserver(this, "xul-window-visible");
-      this._hasXulWindowVisibleObserver = false;
-    }
-    if (AppConstants.platform === "android") {
-      Services.obs.removeObserver(this, "application-background");
-    }
-    if (this._hasActiveTicksObservers) {
-      Services.obs.removeObserver(this, "user-interaction-active");
-      Services.obs.removeObserver(this, "user-interaction-inactive");
-      this._hasActiveTicksObservers = false;
-    }
+
     GCTelemetry.shutdown();
   },
 
   getPayload: function getPayload(reason, clearSubsession) {
     this._log.trace("getPayload - clearSubsession: " + clearSubsession);
     reason = reason || REASON_GATHER_PAYLOAD;
     // This function returns the current Telemetry payload to the caller.
     // We only gather startup info once.
@@ -1972,41 +1959,38 @@ var Impl = {
     // Prevent the cycle collector begin topic from cluttering the log.
     if (aTopic != TOPIC_CYCLE_COLLECTOR_BEGIN) {
       this._log.trace("observe - " + aTopic + " notified.");
     }
 
     switch (aTopic) {
     case "content-child-shutdown":
       // content-child-shutdown is only registered for content processes.
-      Services.obs.removeObserver(this, "content-child-shutdown");
       this.uninstall();
       Telemetry.flushBatchedChildTelemetry();
       this.sendContentProcessPing(REASON_SAVED_SESSION);
       break;
     case TOPIC_CYCLE_COLLECTOR_BEGIN:
       let now = new Date();
       if (!gLastMemoryPoll
           || (TELEMETRY_INTERVAL <= now - gLastMemoryPoll)) {
         gLastMemoryPoll = now;
         this.gatherMemory();
       }
       break;
     case "xul-window-visible":
-      Services.obs.removeObserver(this, "xul-window-visible");
-      this._hasXulWindowVisibleObserver = false;
+      this.removeObserver("xul-window-visible");
       var counters = processInfo.getCounters();
       if (counters) {
         [this._startupIO.startupWindowVisibleReadBytes,
           this._startupIO.startupWindowVisibleWriteBytes] = counters;
       }
       break;
     case "sessionstore-windows-restored":
-      Services.obs.removeObserver(this, "sessionstore-windows-restored");
-      this._hasWindowRestoredObserver = false;
+      this.removeObserver("sessionstore-windows-restored");
       // Check whether debugger was attached during startup
       let debugService = Cc["@mozilla.org/xpcom/debug;1"].getService(Ci.nsIDebug2);
       gWasDebuggerAttached = debugService.isDebuggerAttached;
       this.gatherStartup();
       break;
     case "idle-daily":
       // Enqueue to main-thread, otherwise components may be inited by the
       // idle-daily category and miss the gather-telemetry notification.