Backed out changeset 09043ee913d4 (bug 1366294)
authorSebastian Hengst <archaeopteryx@coole-files.de>
Fri, 21 Jul 2017 15:19:36 +0200
changeset 418927 9f869d8dc3e36c3e8376e9651d3304e95809814f
parent 418926 724d5d0cef066e4716af07ba8f60ac65703c33cc
child 418928 6da4ecf79a4b61cc0e10457b59541ac8fb26a39f
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1366294
milestone56.0a1
backs out09043ee913d4d4c3ccee9187834aa33c54905393
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
Backed out changeset 09043ee913d4 (bug 1366294)
toolkit/components/telemetry/TelemetryHistogram.cpp
toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -606,22 +606,22 @@ internal_ReflectHistogramSnapshot(JSCont
   Histogram::SampleSet ss;
   h->SnapshotSample(&ss);
   return internal_ReflectHistogramAndSamples(cx, obj, h, ss);
 }
 
 bool
 internal_ShouldReflectHistogram(Histogram* h, HistogramID id)
 {
-  // Only flag histograms are serialized when they are empty.
+  // Only count & flag histograms are serialized when they are empty.
   // This has historical reasons, changing this will require downstream changes.
   // The cheaper path here is to just deprecate flag histograms in favor
   // of scalars.
   uint32_t type = gHistogramInfos[id].histogramType;
-  if (internal_IsEmpty(h) && type != nsITelemetry::HISTOGRAM_FLAG) {
+  if (internal_IsEmpty(h) && (type != nsITelemetry::HISTOGRAM_FLAG)) {
     return false;
   }
 
   return true;
 }
 
 } // namespace
 
@@ -950,22 +950,27 @@ internal_ClearHistogram(HistogramID id, 
   nsTArray<SessionType> sessionTypes;
   if (!onlySubsession) {
     sessionTypes.AppendElement(SessionType::Session);
   }
 #if !defined(MOZ_WIDGET_ANDROID)
   sessionTypes.AppendElement(SessionType::Subsession);
 #endif
 
+  if (sessionTypes.Length() == 0) {
+    // Nothing to do here.
+    return;
+  }
+
   // Now reset the histograms instances for all processes.
-  for (SessionType sessionType : sessionTypes) {
+  for (uint32_t sessionIdx = 0; sessionIdx < sessionTypes.Length(); ++sessionIdx) {
     for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
       internal_ClearHistogramById(id,
                                   static_cast<ProcessID>(process),
-                                  sessionType);
+                                  sessionTypes[sessionIdx]);
     }
   }
 }
 
 } // namespace
 
 
 ////////////////////////////////////////////////////////////////////////
@@ -1912,16 +1917,38 @@ TelemetryHistogram::CreateHistogramSnaps
   }
 
 #if !defined(MOZ_WIDGET_ANDROID)
   SessionType sessionType = SessionType(subsession);
 #else
   SessionType sessionType = SessionType::Session;
 #endif
 
+  // Ensure that all the HISTOGRAM_FLAG histograms have
+  // been created, so that their values are snapshotted.
+  auto processType = XRE_GetProcessType();
+  for (uint32_t histogramId = 0; histogramId < HistogramCount; ++histogramId) {
+    const HistogramInfo& info = gHistogramInfos[histogramId];
+    if (info.keyed ||
+      info.histogramType != nsITelemetry::HISTOGRAM_FLAG ||
+      !CanRecordInProcess(info.record_in_processes, processType)) {
+      continue;
+    }
+
+    for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
+      if ((ProcessID(process) == ProcessID::Gpu) && !includeGPUProcess) {
+        continue;
+      }
+
+      mozilla::DebugOnly<Histogram*> h = nullptr;
+      h = internal_GetHistogramById(HistogramID(histogramId), ProcessID(process), sessionType);
+      MOZ_ASSERT(h);
+    }
+  }
+
   for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
     JS::Rooted<JSObject*> processObject(cx, JS_NewPlainObject(cx));
     if (!processObject) {
       return NS_ERROR_FAILURE;
     }
     if (!JS_DefineProperty(cx, root_obj, GetNameForProcessID(ProcessID(process)),
                            processObject, JSPROP_ENUMERATE)) {
       return NS_ERROR_FAILURE;
@@ -1934,21 +1961,17 @@ TelemetryHistogram::CreateHistogramSnaps
 
       HistogramID id = HistogramID(i);
 
       if (!CanRecordInProcess(info.record_in_processes, ProcessID(process)) ||
         ((ProcessID(process) == ProcessID::Gpu) && !includeGPUProcess)) {
         continue;
       }
 
-      bool shouldInstantiate =
-        info.histogramType == nsITelemetry::HISTOGRAM_FLAG;
-      Histogram* h = internal_GetHistogramById(id, ProcessID(process),
-                                               sessionType,
-                                               shouldInstantiate);
+      Histogram* h = internal_GetHistogramById(id, ProcessID(process), sessionType, /* instantiate = */ false);
       if (!h || internal_IsExpired(h) || !internal_ShouldReflectHistogram(h, id)) {
         continue;
       }
 
       JS::Rooted<JSObject*> hobj(cx, JS_NewPlainObject(cx));
       if (!hobj) {
         return NS_ERROR_FAILURE;
       }
--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ -775,40 +775,40 @@ add_task(async function test_checkSubses
   ]);
 
   // Compare the two sets of histograms.
   // The "subsession" histograms should match the registered
   // "classic" histograms. However, histograms can change
   // between us collecting the different payloads, so we only
   // check for deep equality on known stable histograms.
   let checkHistograms = (classic, subsession, message) => {
-    for (let id of Object.keys(subsession)) {
+    for (let id of Object.keys(classic)) {
       if (!registeredIds.has(id)) {
         continue;
       }
 
-      Assert.ok(id in classic, message + ` (${id})`);
+      Assert.ok(id in subsession, message + ` (${id})`);
       if (stableHistograms.has(id)) {
         Assert.deepEqual(classic[id],
                          subsession[id], message);
       } else {
         Assert.equal(classic[id].histogram_type,
                      subsession[id].histogram_type, message);
       }
     }
   };
 
   // Same as above, except for keyed histograms.
   let checkKeyedHistograms = (classic, subsession, message) => {
-    for (let id of Object.keys(subsession)) {
+    for (let id of Object.keys(classic)) {
       if (!registeredIds.has(id)) {
         continue;
       }
 
-      Assert.ok(id in classic, message);
+      Assert.ok(id in subsession, message);
       if (stableKeyedHistograms.has(id)) {
         Assert.deepEqual(classic[id],
                          subsession[id], message);
       }
     }
   };
 
   // Both classic and subsession payload histograms should start the same.
@@ -891,18 +891,19 @@ add_task(async function test_checkSubses
   checkKeyedHistograms(classic.keyedHistograms, subsession.keyedHistograms, "Should be able to reset subsession keyed");
 
   // ... then check that the next snapshot shows the subsession
   // histograms got reset.
   classic = TelemetrySession.getPayload();
   subsession = TelemetrySession.getPayload("environment-change");
 
   Assert.ok(COUNT_ID in classic.histograms);
-  Assert.ok(!(COUNT_ID in subsession.histograms));
+  Assert.ok(COUNT_ID in subsession.histograms);
   Assert.equal(classic.histograms[COUNT_ID].sum, 1);
+  Assert.equal(subsession.histograms[COUNT_ID].sum, 0);
 
   Assert.ok(KEYED_ID in classic.keyedHistograms);
   Assert.ok(!(KEYED_ID in subsession.keyedHistograms));
   Assert.equal(classic.keyedHistograms[KEYED_ID]["a"].sum, 1);
   Assert.equal(classic.keyedHistograms[KEYED_ID]["b"].sum, 1);
 
   // Adding values should get picked up in both again.
   count.add(1);
@@ -1051,17 +1052,17 @@ add_task(async function test_dailyCollec
   ping = await PingServer.promiseNextPing();
   Assert.ok(!!ping);
 
   Assert.equal(ping.type, PING_TYPE_MAIN);
   Assert.equal(ping.payload.info.reason, REASON_DAILY);
   subsessionStartDate = new Date(ping.payload.info.subsessionStartDate);
   Assert.equal(subsessionStartDate.toISOString(), expectedDate.toISOString());
 
-  Assert.ok(!(COUNT_ID in ping.payload.histograms));
+  Assert.equal(ping.payload.histograms[COUNT_ID].sum, 0);
   Assert.ok(!(KEYED_ID in ping.payload.keyedHistograms));
 
   // Trigger and collect another daily ping, with the histograms being set again.
   count.add(1);
   keyed.add("a", 1);
   keyed.add("b", 1);
 
   // The daily ping is rescheduled for "tomorrow".
@@ -1255,17 +1256,17 @@ add_task(async function test_environment
   Assert.ok(!!ping);
 
   Assert.equal(ping.type, PING_TYPE_MAIN);
   Assert.equal(ping.environment.settings.userPrefs[PREF_TEST], 1);
   Assert.equal(ping.payload.info.reason, REASON_ENVIRONMENT_CHANGE);
   subsessionStartDate = new Date(ping.payload.info.subsessionStartDate);
   Assert.equal(subsessionStartDate.toISOString(), startHour.toISOString());
 
-  Assert.ok(!(COUNT_ID in ping.payload.histograms));
+  Assert.equal(ping.payload.histograms[COUNT_ID].sum, 0);
   Assert.ok(!(KEYED_ID in ping.payload.keyedHistograms));
 
   await TelemetryController.testShutdown();
 });
 
 add_task(async function test_experimentAnnotations_subsession() {
   if (gIsAndroid) {
     // We don't split subsessions on environment changes yet on Android.