Bug 689142 - Places telemetry gets lost, due to being collected on idle-daily.

r=taras,dietrich

toolkit/components/places/PlacesCategoriesStarter.js

@@ -43,10 +43,20 @@
 const Cc = Components.classes;
 const Ci = Components.interfaces;
 
+// Fired by TelemetryPing when async telemetry data should be collected.
+const TOPIC_GATHER_TELEMETRY = "gather-telemetry";
+
 // Seconds between maintenance runs.
 const MAINTENANCE_INTERVAL_SECONDS = 7 * 86400;
 
+////////////////////////////////////////////////////////////////////////////////
+//// Imports
+
 Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
+Components.utils.import("resource://gre/modules/Services.jsm");
+Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "PlacesDBUtils",
+                                  "resource://gre/modules/PlacesDBUtils.jsm");
 
 /**
  * This component can be used as a starter for modules that have to run when
@@ -54,6 +64,8 @@ Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
  */
 function PlacesCategoriesStarter()
 {
+  Services.obs.addObserver(this, TOPIC_GATHER_TELEMETRY, false);
+  Services.obs.addObserver(this, PlacesUtils.TOPIC_SHUTDOWN, false);
 }
 
 PlacesCategoriesStarter.prototype = {
@@ -63,15 +75,22 @@ PlacesCategoriesStarter.prototype = {
   observe: function PCS_observe(aSubject, aTopic, aData)
   {
     switch (aTopic) {
+      case PlacesUtils.TOPIC_SHUTDOWN:
+        Services.obs.removeObserver(this, PlacesUtils.TOPIC_SHUTDOWN);
+        Services.obs.removeObserver(this, TOPIC_GATHER_TELEMETRY);
+        break;
+      case TOPIC_GATHER_TELEMETRY:
+        PlacesDBUtils.telemetry();
+        break;
       case "idle-daily":
         // Once a week run places.sqlite maintenance tasks.
         let lastMaintenance = 0;
         try {
-          lastMaintenance = Services.prefs.getIntPref("places.database.lastMaintenance");
+          lastMaintenance =
+            Services.prefs.getIntPref("places.database.lastMaintenance");
         } catch (ex) {}
         let nowSeconds = parseInt(Date.now() / 1000);
         if (lastMaintenance < nowSeconds - MAINTENANCE_INTERVAL_SECONDS) {
-          Components.utils.import("resource://gre/modules/PlacesDBUtils.jsm");
           PlacesDBUtils.maintenanceOnIdle();
         }
         break;

toolkit/components/places/PlacesDBUtils.jsm

@@ -54,6 +54,8 @@ let EXPORTED_SYMBOLS = [ "PlacesDBUtils" ];
 
 const FINISHED_MAINTENANCE_TOPIC = "places-maintenance-finished";
 
+const BYTES_PER_MEBIBYTE = 1048576;
+
 ////////////////////////////////////////////////////////////////////////////////
 //// Smart getters
 
@@ -115,7 +117,6 @@ let PlacesDBUtils = {
       this.checkIntegrity
     , this.checkCoherence
     , this._refreshUI
-    , this._telemetry
     ]);
     tasks.callback = aCallback;
     tasks.scope = aScope;
@@ -861,7 +862,7 @@ let PlacesDBUtils = {
    * @param [optional] aTasks
    *        Tasks object to execute.
    */
-  _telemetry: function PDBU__telemetry(aTasks)
+  telemetry: function PDBU_telemetry(aTasks)
   {
     let tasks = new Tasks(aTasks);
 
@@ -911,7 +912,7 @@ let PlacesDBUtils = {
         let DBFile = Services.dirsvc.get("ProfD", Ci.nsILocalFile);
         DBFile.append("places.sqlite");
         try {
-          return parseInt(DBFile.fileSize / 1024);
+          return parseInt(DBFile.fileSize / BYTES_PER_MEBIBYTE);
         } catch (ex) {
           return 0;
         }
@@ -921,7 +922,7 @@ let PlacesDBUtils = {
         let DBFile = Services.dirsvc.get("ProfD", Ci.nsILocalFile);
         DBFile.append("places.sqlite-wal");
         try {
-          return parseInt(DBFile.fileSize / 1024);
+          return parseInt(DBFile.fileSize / BYTES_PER_MEBIBYTE);
         } catch (ex) {
           return 0;
         }

toolkit/components/places/tests/unit/test_telemetry.js

@@ -64,8 +64,10 @@ function run_test() {
   PlacesUtils.tagging.tagURI(uri, ["tag"]);
   PlacesUtils.bookmarks.setKeywordForBookmark(itemId, "keyword");
 
-  // Test generic database probes.
-  PlacesDBUtils._telemetry();
+  // Request to gather telemetry data.
+  Cc["@mozilla.org/places/categoriesStarter;1"]
+    .getService(Ci.nsIObserver)
+    .observe(null, "gather-telemetry", null);
 
   waitForAsyncUpdates(continue_test);
 }

toolkit/components/telemetry/TelemetryPing.js

@@ -62,6 +62,10 @@ const MEM_HISTOGRAMS = {
   "heap-allocated": "MEMORY_HEAP_ALLOCATED",
   "page-faults-hard": "PAGE_FAULTS_HARD"
 };
+// Seconds of idle time before pinging.
+// On idle-daily a gather-telemetry notification is fired, during it probes can
+// start asynchronous tasks to gather data.  On the next idle the data is sent.
+const IDLE_TIMEOUT_SECONDS = 5 * 60;
 
 var gLastMemoryPoll = null;
 
@@ -341,6 +345,10 @@ TelemetryPing.prototype = {
       return;
     Services.obs.removeObserver(this, "idle-daily");
     Services.obs.removeObserver(this, "cycle-collector-begin");
+    if (this._isIdleObserver) {
+      idle.removeIdleObserver(this, IDLE_TIMEOUT_SECONDS);
+      this._isIdleObserver = false;
+    }
   },
 
   /**
@@ -416,11 +424,26 @@ TelemetryPing.prototype = {
         this.attachObservers()
       }
       break;
+    case "idle-daily":
+      // Enqueue to main-thread, otherwise components may be inited by the
+      // idle-daily category and miss the gather-telemetry notification.
+      Services.tm.mainThread.dispatch((function() {
+        // Notify that data should be gathered now, since ping will happen soon.
+        Services.obs.notifyObservers(null, "gather-telemetry", null);
+        // The ping happens at the first idle of length IDLE_TIMEOUT_SECONDS.
+        idle.addIdleObserver(this, IDLE_TIMEOUT_SECONDS);
+        this._isIdleObserver = true;
+      }).bind(this), Ci.nsIThread.DISPATCH_NORMAL);
+      break;
     case "test-ping":
       server = aData;
       // fall through
-    case "idle-daily":
-      this.send(aTopic, server);
+    case "idle":
+      if (this._isIdleObserver) {
+        idle.removeIdleObserver(this, IDLE_TIMEOUT_SECONDS);
+        this._isIdleObserver = false;
+      }
+      this.send(aTopic == "idle" ? "idle-daily" : aTopic, server);
       break;
     }
   },

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

@@ -0,0 +1,19 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Check that TelemetryPing notifies correctly on idle.
+
+Components.utils.import("resource://gre/modules/Services.jsm");
+
+function run_test() {
+  do_test_pending();
+
+  Services.obs.addObserver(function observeTelemetry() {
+    Services.obs.removeObserver(observeTelemetry, "gather-telemetry");
+    do_test_finished();
+  }, "gather-telemetry", false);
+
+  Components.classes["@mozilla.org/base/telemetry-ping;1"]
+    .getService(Components.interfaces.nsIObserver)
+    .observe(null, "idle-daily", null);
+}

toolkit/components/telemetry/tests/unit/xpcshell.ini

@@ -6,3 +6,4 @@ tail =
 [test_TelemetryPing.js]
 # Bug 676989: test fails consistently on Android
 # fail-if = os == "android"
+[test_TelemetryPing_idle.js]