Bug 1468809 - Do not snapshot expired keyed histograms. r?chutten,janerik draft
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Fri, 15 Jun 2018 19:31:26 +0200
changeset 807759 271aa1766ab00624bbf707b83e9ad438d4a9dca9
parent 807714 0b5495dc100dd3bfda0886a4ad563a3c729c9b72
push id113202
push useralessio.placitelli@gmail.com
push dateFri, 15 Jun 2018 17:51:00 +0000
reviewerschutten, janerik
bugs1468809
milestone62.0a1
Bug 1468809 - Do not snapshot expired keyed histograms. r?chutten,janerik This patch changes the snapshotting code for keyed histograms so that requesting a snapshot does not report expired histograms. MozReview-Commit-ID: GDiw6yOcF8J
toolkit/components/telemetry/Histograms.json
toolkit/components/telemetry/TelemetryHistogram.cpp
toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -7414,16 +7414,28 @@
   },
   "TELEMETRY_TEST_EXPIRED": {
     "record_in_processes": ["main", "content"],
     "alert_emails": ["telemetry-client-dev@mozilla.com"],
     "expires_in_version": "4",
     "kind": "flag",
     "description": "a testing histogram; not meant to be touched"
   },
+  "TELEMETRY_TEST_EXPIRED_KEYED": {
+    "record_in_processes": ["main", "content"],
+    "alert_emails": ["telemetry-client-dev@mozilla.com"],
+    "expires_in_version": "4",
+    "kind": "linear",
+    "keyed": true,
+    "low": 1,
+    "high": 2147483646,
+    "n_buckets": 10,
+    "bug_numbers": [1468809],
+    "description": "a testing histogram; not meant to be touched"
+  },
   "TELEMETRY_TEST_CONTENT_PROCESS": {
     "record_in_processes": ["content"],
     "alert_emails": ["telemetry-client-dev@mozilla.com"],
     "expires_in_version": "never",
     "kind": "linear",
     "low": 1,
     "high": 10000,
     "n_buckets": 10,
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -174,17 +174,17 @@ struct KeyedHistogramSnapshotInfo {
   HistogramID histogramId;
 };
 
 typedef mozilla::Vector<KeyedHistogramSnapshotInfo> KeyedHistogramSnapshotsArray;
 typedef mozilla::Vector<KeyedHistogramSnapshotsArray> KeyedHistogramProcessSnapshotsArray;
 
 class KeyedHistogram {
 public:
-  KeyedHistogram(HistogramID id, const HistogramInfo& info);
+  KeyedHistogram(HistogramID id, const HistogramInfo& info, bool expired);
   ~KeyedHistogram();
   nsresult GetHistogram(const nsCString& name, Histogram** histogram);
   Histogram* GetHistogram(const nsCString& name);
   uint32_t GetHistogramType() const { return mHistogramInfo.histogramType; }
   nsresult GetJSKeys(JSContext* cx, JS::CallArgs& args);
   // Note: unlike other methods, GetJSSnapshot is thread safe.
   nsresult GetJSSnapshot(JSContext* cx, JS::Handle<JSObject*> obj,
                          bool clearSubsession);
@@ -193,23 +193,26 @@ public:
 
   nsresult Add(const nsCString& key, uint32_t aSample, ProcessID aProcessType);
   void Clear();
 
   HistogramID GetHistogramID() const { return mId; }
 
   bool IsEmpty() const { return mHistogramMap.IsEmpty(); }
 
+  bool IsExpired() const { return mIsExpired; }
+
 private:
   typedef nsBaseHashtableET<nsCStringHashKey, Histogram*> KeyedHistogramEntry;
   typedef AutoHashtable<KeyedHistogramEntry> KeyedHistogramMapType;
   KeyedHistogramMapType mHistogramMap;
 
   const HistogramID mId;
   const HistogramInfo& mHistogramInfo;
+  bool mIsExpired;
 };
 
 } // namespace
 
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
 //
@@ -232,16 +235,19 @@ Histogram** gHistogramStorage;
 KeyedHistogram** gKeyedHistogramStorage;
 
 // Cache of histogram name to a histogram id.
 StringToHistogramIdMap gNameToHistogramIDMap(HistogramCount);
 
 // To simplify logic below we use a single histogram instance for all expired histograms.
 Histogram* gExpiredHistogram = nullptr;
 
+// The single placeholder for expired keyed histograms.
+KeyedHistogram* gExpiredKeyedHistogram = nullptr;
+
 // This tracks whether recording is enabled for specific histograms.
 // To utilize C++ initialization rules, we invert the meaning to "disabled".
 bool gHistogramRecordingDisabled[HistogramCount] = {};
 
 // This is for gHistogramInfos, gHistogramStringTable
 #include "TelemetryHistogramData.inc"
 
 } // namespace
@@ -379,17 +385,31 @@ internal_GetKeyedHistogramById(Histogram
 
   KeyedHistogram* kh = internal_GetKeyedHistogramFromStorage(histogramId,
                                                              processId);
   if (kh || !instantiate) {
     return kh;
   }
 
   const HistogramInfo& info = gHistogramInfos[histogramId];
-  kh = new KeyedHistogram(histogramId, info);
+  const bool isExpired = IsExpiredVersion(info.expiration());
+
+  // If the keyed histogram is expired, set its storage to the expired
+  // keyed histogram.
+  if (isExpired) {
+    if (!gExpiredKeyedHistogram) {
+      // If we don't have an expired keyed histogram, create one.
+      gExpiredKeyedHistogram = new KeyedHistogram(histogramId, info, true /* expired */);
+      MOZ_ASSERT(gExpiredKeyedHistogram);
+    }
+    kh = gExpiredKeyedHistogram;
+  } else {
+    kh = new KeyedHistogram(histogramId, info, false /* expired */);
+  }
+
   internal_SetKeyedHistogramInStorage(histogramId, processId, kh);
 
   return kh;
 }
 
 // Look up a histogram id from a histogram name.
 nsresult
 internal_GetHistogramIdByName(const StaticMutexAutoLock& aLock,
@@ -894,20 +914,21 @@ internal_ReflectKeyedHistogram(const Key
                              histogramSnapshot, JSPROP_ENUMERATE)) {
       return NS_ERROR_FAILURE;
     }
   }
 
   return NS_OK;
 }
 
-KeyedHistogram::KeyedHistogram(HistogramID id, const HistogramInfo& info)
+KeyedHistogram::KeyedHistogram(HistogramID id, const HistogramInfo& info, bool expired)
   : mHistogramMap()
   , mId(id)
   , mHistogramInfo(info)
+  , mIsExpired(expired)
 {
 }
 
 KeyedHistogram::~KeyedHistogram()
 {
   for (auto iter = mHistogramMap.Iter(); !iter.Done(); iter.Next()) {
     Histogram* h = iter.Get()->mData;
     if (h == gExpiredHistogram) {
@@ -964,16 +985,21 @@ KeyedHistogram::Add(const nsCString& key
                                            internal_CanRecordExtended());
   // If `histogram` is a non-parent-process histogram, then recording-enabled
   // has been checked in its owner process.
   if (!canRecordDataset ||
     (aProcessType == ProcessID::Parent && !internal_IsRecordingEnabled(mId))) {
     return NS_OK;
   }
 
+  // Don't record if expired.
+  if (IsExpired()) {
+    return NS_OK;
+  }
+
   // Don't record if the current platform is not enabled
   if (!CanRecordProduct(gHistogramInfos[mId].products)) {
     return NS_OK;
   }
 
   Histogram* histogram = GetHistogram(key);
   MOZ_ASSERT(histogram);
   if (!histogram) {
@@ -1129,17 +1155,17 @@ internal_GetKeyedHistogramsSnapshot(cons
 
       if (!IsInDataset(info.dataset, aDataset)) {
         continue;
       }
 
       KeyedHistogram* keyed = internal_GetKeyedHistogramById(id,
                                                              ProcessID(process),
                                                              /* instantiate = */ false);
-      if (!keyed || (aSkipEmpty && keyed->IsEmpty())) {
+      if (!keyed || (aSkipEmpty && keyed->IsEmpty()) || keyed->IsExpired()) {
         continue;
       }
 
       // Take a snapshot of the keyed histogram data!
       KeyedHistogramSnapshotData snapshot;
       if (!NS_SUCCEEDED(keyed->GetSnapshot(aLock, snapshot, aClearSubsession))) {
         return NS_ERROR_FAILURE;
       }
@@ -1995,28 +2021,31 @@ void TelemetryHistogram::DeInitializeGlo
   gCanRecordBase = false;
   gCanRecordExtended = false;
   gNameToHistogramIDMap.Clear();
   gInitDone = false;
 
   // FactoryGet `new`s Histograms for us, but requires us to manually delete.
   if (XRE_IsParentProcess()) {
     for (size_t i = 0; i < HistogramCount * size_t(ProcessID::Count); ++i) {
-      if (i < HistogramCount * size_t(ProcessID::Count)) {
+      if (i < HistogramCount * size_t(ProcessID::Count)
+          && gKeyedHistogramStorage[i] != gExpiredKeyedHistogram) {
         delete gKeyedHistogramStorage[i];
       }
       if (gHistogramStorage[i] != gExpiredHistogram) {
         delete gHistogramStorage[i];
       }
     }
     delete[] gHistogramStorage;
     delete[] gKeyedHistogramStorage;
   }
   delete gExpiredHistogram;
   gExpiredHistogram = nullptr;
+  delete gExpiredKeyedHistogram;
+  gExpiredKeyedHistogram = nullptr;
 }
 
 #ifdef DEBUG
 bool TelemetryHistogram::GlobalStateHasBeenInitialized() {
   StaticMutexAutoLock locker(gTelemetryHistogramMutex);
   return gInitDone;
 }
 #endif
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ -450,16 +450,33 @@ add_task(async function test_expired_his
     }
     Assert.equal(histograms[process].__expired__, undefined);
   }
   let parentHgrams = Telemetry.snapshotHistograms(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN,
                                                   false /* clear */).parent;
   Assert.equal(parentHgrams[test_expired_id], undefined);
 });
 
+add_task(async function test_keyed_expired_histogram() {
+  var test_expired_id = "TELEMETRY_TEST_EXPIRED_KEYED";
+  var dummy = Telemetry.getKeyedHistogramById(test_expired_id);
+  dummy.add("someKey", 1);
+
+  const histograms = Telemetry.snapshotKeyedHistograms(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN,
+                                                       false /* clear */);
+  for (let process of ["parent", "content", "gpu", "extension"]) {
+    if (!(process in histograms)) {
+      info("Nothing present for process " + process);
+      continue;
+    }
+    Assert.ok(!(test_expired_id in histograms[process]),
+              "The expired keyed histogram must not be reported");
+  }
+});
+
 add_task(async function test_keyed_histogram() {
   // Check that invalid names get rejected.
 
   let threw = false;
   try {
     Telemetry.getKeyedHistogramById("test::unknown histogram", "never", Telemetry.HISTOGRAM_BOOLEAN);
   } catch (e) {
     // This should throw as it is an unknown ID