Bug 1335343 - Prevent histogram recording in disallowed processes. r=Dexter

Unfortunately it means using FLAG histograms because I need to test that their
"we have a hidden default value" behaviour is appropriately silenced as well.

MozReview-Commit-ID: J8xGXwCTRIg

toolkit/components/telemetry/Histograms.json

@@ -7616,6 +7616,67 @@
     "kind": "flag",
     "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,
+    "bug_numbers": [1335343],
+    "description": "a testing histogram; not meant to be touched"
+  },
+  "TELEMETRY_TEST_KEYED_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,
+    "keyed": true,
+    "bug_numbers": [1335343],
+    "description": "a testing histogram; not meant to be touched"
+  },
+  "TELEMETRY_TEST_FLAG_CONTENT_PROCESS": {
+    "record_in_processes": ["content"],
+    "alert_emails": ["telemetry-client-dev@mozilla.com"],
+    "expires_in_version": "never",
+    "kind": "flag",
+    "bug_numbers": [1335343],
+    "description": "a testing histogram; not meant to be touched"
+  },
+  "TELEMETRY_TEST_FLAG_MAIN_PROCESS": {
+    "record_in_processes": ["main"],
+    "alert_emails": ["telemetry-client-dev@mozilla.com"],
+    "expires_in_version": "never",
+    "kind": "flag",
+    "bug_numbers": [1335343],
+    "description": "a testing histogram; not meant to be touched"
+  },
+  "TELEMETRY_TEST_ALL_PROCESSES": {
+    "record_in_processes": ["all"],
+    "alert_emails": ["telemetry-client-dev@mozilla.com"],
+    "expires_in_version": "never",
+    "kind": "linear",
+    "low": 1,
+    "high": 10000,
+    "n_buckets": 10,
+    "bug_numbers": [1335343],
+    "description": "a testing histogram; not meant to be touched"
+  },
+  "TELEMETRY_TEST_ALL_CHILD_PROCESSES": {
+    "record_in_processes": ["all_childs"],
+    "alert_emails": ["telemetry-client-dev@mozilla.com"],
+    "expires_in_version": "never",
+    "kind": "linear",
+    "low": 1,
+    "high": 10000,
+    "n_buckets": 10,
+    "bug_numbers": [1335343],
+    "description": "a testing histogram; not meant to be touched"
+  },
   "STARTUP_CRASH_DETECTED": {
     "record_in_processes": ["main", "content"],
     "expires_in_version": "never",

toolkit/components/telemetry/TelemetryHistogram.cpp

@@ -39,6 +39,7 @@ using mozilla::Telemetry::Accumulation;
 using mozilla::Telemetry::KeyedAccumulation;
 using mozilla::Telemetry::ProcessID;
 using mozilla::Telemetry::Common::LogToBrowserConsole;
+using mozilla::Telemetry::Common::RecordedProcessType;
 
 namespace TelemetryIPCAccumulator = mozilla::TelemetryIPCAccumulator;
 
@@ -133,6 +134,7 @@ struct HistogramInfo {
   uint32_t dataset;
   uint32_t label_index;
   uint32_t label_count;
+  RecordedProcessType record_in_processes;
   bool keyed;
 
   const char *id() const;
@@ -984,7 +986,7 @@ KeyedHistogram::Add(const nsCString& key, uint32_t sample)
   bool canRecordDataset = CanRecordDataset(mDataset,
                                            internal_CanRecordBase(),
                                            internal_CanRecordExtended());
-  if (!canRecordDataset) {
+  if (!canRecordDataset || !IsRecordingEnabled()) {
     return NS_OK;
   }
 
@@ -1001,10 +1003,6 @@ KeyedHistogram::Add(const nsCString& key, uint32_t sample)
   }
 #endif
 
-  if (!IsRecordingEnabled()) {
-    return NS_OK;
-  }
-
   histogram->Add(sample);
 #if !defined(MOZ_WIDGET_ANDROID)
   subsession->Add(sample);
@@ -1871,7 +1869,6 @@ void TelemetryHistogram::InitializeGlobalState(bool canRecordBase,
                            new KeyedHistogram(id, expiration, h.histogramType,
                                               h.min, h.max, h.bucketCount, h.dataset));
 
-
       nsCString gpuId(id);
       gpuId.AppendLiteral(GPU_HISTOGRAM_SUFFIX);
       gKeyedHistograms.Put(gpuId,
@@ -1952,6 +1949,15 @@ void
 TelemetryHistogram::InitHistogramRecordingEnabled()
 {
   StaticMutexAutoLock locker(gTelemetryHistogramMutex);
+  auto processType = XRE_GetProcessType();
+  for (size_t i = 0; i < mozilla::ArrayLength(gHistograms); ++i) {
+    const HistogramInfo& h = gHistograms[i];
+    mozilla::Telemetry::HistogramID id = mozilla::Telemetry::HistogramID(i);
+    internal_SetHistogramRecordingEnabled(id,
+                                          CanRecordInProcess(h.record_in_processes,
+                                                             processType));
+  }
+
   for (auto recordingInitiallyDisabledID : kRecordingInitiallyDisabledIDs) {
     internal_SetHistogramRecordingEnabled(recordingInitiallyDisabledID,
                                           false);
@@ -1967,6 +1973,12 @@ TelemetryHistogram::SetHistogramRecordingEnabled(mozilla::Telemetry::HistogramID
     return;
   }
 
+  const HistogramInfo& h = gHistograms[aID];
+  if (!CanRecordInProcess(h.record_in_processes, XRE_GetProcessType())) {
+    // Don't permit record_in_process-disabled recording to be re-enabled.
+    return;
+  }
+
   StaticMutexAutoLock locker(gTelemetryHistogramMutex);
   internal_SetHistogramRecordingEnabled(aID, aEnabled);
 }
@@ -1977,8 +1989,19 @@ TelemetryHistogram::SetHistogramRecordingEnabled(const nsACString &id,
                                                  bool aEnabled)
 {
   StaticMutexAutoLock locker(gTelemetryHistogramMutex);
+
+  mozilla::Telemetry::HistogramID hId;
+  nsresult rv = internal_GetHistogramEnumId(PromiseFlatCString(id).get(), &hId);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  const HistogramInfo& hi = gHistograms[hId];
+  if (!CanRecordInProcess(hi.record_in_processes, XRE_GetProcessType())) {
+    return NS_OK;
+  }
+
   Histogram *h;
-  nsresult rv = internal_GetHistogramByName(id, &h);
+  rv = internal_GetHistogramByName(id, &h);
   if (NS_SUCCEEDED(rv)) {
     h->SetRecordingEnabled(aEnabled);
     return NS_OK;
@@ -2175,11 +2198,13 @@ TelemetryHistogram::CreateHistogramSnapshots(JSContext *cx,
 
   // Ensure that all the HISTOGRAM_FLAG & HISTOGRAM_COUNT histograms have
   // been created, so that their values are snapshotted.
+  auto processType = XRE_GetProcessType();
   for (size_t i = 0; i < mozilla::Telemetry::HistogramCount; ++i) {
-    if (gHistograms[i].keyed) {
+    const HistogramInfo& hi = gHistograms[i];
+    if (hi.keyed || !CanRecordInProcess(hi.record_in_processes, processType)) {
       continue;
     }
-    const uint32_t type = gHistograms[i].histogramType;
+    const uint32_t type = hi.histogramType;
     if (type == nsITelemetry::HISTOGRAM_FLAG ||
         type == nsITelemetry::HISTOGRAM_COUNT) {
       Histogram *h;
@@ -2210,7 +2235,8 @@ TelemetryHistogram::CreateHistogramSnapshots(JSContext *cx,
   // OK, now we can actually reflect things.
   JS::Rooted<JSObject*> hobj(cx);
   for (size_t i = 0; i < mozilla::Telemetry::HistogramCount; ++i) {
-    if (gHistograms[i].keyed) {
+    const HistogramInfo& hi = gHistograms[i];
+    if (hi.keyed) {
       continue;
     }
 
@@ -2218,7 +2244,8 @@ TelemetryHistogram::CreateHistogramSnapshots(JSContext *cx,
     mozilla::Telemetry::HistogramID id = mozilla::Telemetry::HistogramID(i);
 
     for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
-      if ((ProcessID(process) == ProcessID::Gpu) && !includeGPUProcess) {
+      if (!CanRecordInProcess(hi.record_in_processes, ProcessID(process)) ||
+        ((ProcessID(process) == ProcessID::Gpu) && !includeGPUProcess)) {
         continue;
       }
       nsresult rv = internal_GetHistogramByEnumId(id, &h, ProcessID(process));

toolkit/components/telemetry/gen-histogram-data.py

@@ -20,7 +20,7 @@ def print_array_entry(output, histogram, name_index, exp_index, label_index, lab
     cpp_guard = histogram.cpp_guard()
     if cpp_guard:
         print("#if defined(%s)" % cpp_guard, file=output)
-    print("  { %s, %s, %s, %s, %d, %d, %s, %d, %d, %s },"
+    print("  { %s, %s, %s, %s, %d, %d, %s, %d, %d, %s, %s },"
           % (histogram.low(),
              histogram.high(),
              histogram.n_buckets(),
@@ -30,6 +30,7 @@ def print_array_entry(output, histogram, name_index, exp_index, label_index, lab
              histogram.dataset(),
              label_index,
              label_count,
+             " | ".join(histogram.record_in_processes_enum()),
              "true" if histogram.keyed() else "false"), file=output)
     if cpp_guard:
         print("#endif", file=output)

toolkit/components/telemetry/histogram-whitelists.json

@@ -2002,6 +2002,8 @@
     "TELEMETRY_TEST_COUNT_INIT_NO_RECORD",
     "TELEMETRY_TEST_EXPIRED",
     "TELEMETRY_TEST_FLAG",
+    "TELEMETRY_TEST_FLAG_CONTENT_PROCESS",
+    "TELEMETRY_TEST_FLAG_MAIN_PROCESS",
     "TELEMETRY_TEST_KEYED_COUNT",
     "TELEMETRY_TEST_KEYED_COUNT_INIT_NO_RECORD",
     "TELEMETRY_TEST_KEYED_FLAG",

toolkit/components/telemetry/histogram_tools.py

@@ -120,6 +120,7 @@ symbol that should guard C/C++ definitions associated with the histogram."""
         self._keyed = definition.get('keyed', False)
         self._expiration = definition.get('expires_in_version')
         self._labels = definition.get('labels', [])
+        self._record_in_processes = definition.get('record_in_processes')
 
         self.compute_bucket_parameters(definition)
         self.set_nsITelemetry_kind()
@@ -179,11 +180,11 @@ associated with the histogram.  Returns None if no guarding is necessary."""
 
     def record_in_processes(self):
         """Returns a list of processes this histogram is permitted to record in."""
-        return self.definition['record_in_processes']
+        return self._record_in_processes
 
     def record_in_processes_enum(self):
         """Get the non-empty list of flags representing the processes to record data in"""
-        return [utils.process_name_to_enum(p) for p in self.record_in_processes]
+        return [utils.process_name_to_enum(p) for p in self.record_in_processes()]
 
     def ranges(self):
         """Return an array of lower bounds for each bucket in the histogram."""

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

@@ -38,6 +38,20 @@ function run_child_test() {
   countKeyed.add("a");
   countKeyed.add("b");
   countKeyed.add("b");
+
+  // Test record_in_processes
+  let contentLinear = Telemetry.getHistogramById("TELEMETRY_TEST_CONTENT_PROCESS");
+  contentLinear.add(10);
+  let contentKeyed = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_CONTENT_PROCESS");
+  contentKeyed.add("content", 1);
+  let contentFlag = Telemetry.getHistogramById("TELEMETRY_TEST_FLAG_CONTENT_PROCESS");
+  contentFlag.add(true);
+  let mainFlag = Telemetry.getHistogramById("TELEMETRY_TEST_FLAG_MAIN_PROCESS");
+  mainFlag.add(true);
+  let allLinear = Telemetry.getHistogramById("TELEMETRY_TEST_ALL_PROCESSES");
+  allLinear.add(10);
+  let allChildLinear = Telemetry.getHistogramById("TELEMETRY_TEST_ALL_CHILD_PROCESSES");
+  allChildLinear.add(10);
 }
 
 function check_histogram_values(payload) {
@@ -96,6 +110,21 @@ add_task(async function() {
            "histograms" in payload.processes.content &&
            "TELEMETRY_TEST_COUNT" in payload.processes.content.histograms;
   });
+
+  // Test record_in_processes in main process, too
+  let contentLinear = Telemetry.getHistogramById("TELEMETRY_TEST_CONTENT_PROCESS");
+  contentLinear.add(20);
+  let contentKeyed = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_CONTENT_PROCESS");
+  contentKeyed.add("parent", 1);
+  let contentFlag = Telemetry.getHistogramById("TELEMETRY_TEST_FLAG_CONTENT_PROCESS");
+  contentFlag.add(true);
+  let mainFlag = Telemetry.getHistogramById("TELEMETRY_TEST_FLAG_MAIN_PROCESS");
+  mainFlag.add(true);
+  let allLinear = Telemetry.getHistogramById("TELEMETRY_TEST_ALL_PROCESSES");
+  allLinear.add(20);
+  let allChildLinear = Telemetry.getHistogramById("TELEMETRY_TEST_ALL_CHILD_PROCESSES");
+  allChildLinear.add(20);
+
   const payload = TelemetrySession.getPayload("test-ping");
   Assert.ok("processes" in payload, "Should have processes section");
   Assert.ok("content" in payload.processes, "Should have child process section");
@@ -103,5 +132,33 @@ add_task(async function() {
   Assert.ok("keyedHistograms" in payload.processes.content, "Child process section should have keyed histograms.");
   check_histogram_values(payload.processes.content);
 
+  // Check record_in_processes
+  // Content Process
+  let hs = payload.processes.content.histograms;
+  let khs = payload.processes.content.keyedHistograms;
+  Assert.ok("TELEMETRY_TEST_CONTENT_PROCESS" in hs, "Should have content process histogram");
+  Assert.equal(hs.TELEMETRY_TEST_CONTENT_PROCESS.sum, 10, "Should have correct value");
+  Assert.ok("TELEMETRY_TEST_KEYED_CONTENT_PROCESS" in khs, "Should have keyed content process histogram");
+  Assert.equal(khs.TELEMETRY_TEST_KEYED_CONTENT_PROCESS.content.sum, 1, "Should have correct value");
+  Assert.ok("TELEMETRY_TEST_FLAG_CONTENT_PROCESS" in hs, "Should have content process histogram");
+  Assert.equal(hs.TELEMETRY_TEST_FLAG_CONTENT_PROCESS.sum, 1, "Should have correct value");
+  Assert.ok("TELEMETRY_TEST_ALL_PROCESSES" in hs, "Should have content process histogram");
+  Assert.equal(hs.TELEMETRY_TEST_ALL_PROCESSES.sum, 10, "Should have correct value");
+  Assert.ok("TELEMETRY_TEST_ALL_CHILD_PROCESSES" in hs, "Should have content process histogram");
+  Assert.equal(hs.TELEMETRY_TEST_ALL_CHILD_PROCESSES.sum, 10, "Should have correct value");
+  Assert.ok(!("TELEMETRY_TEST_FLAG_MAIN_PROCESS" in hs), "Should not have main process histogram in child process payload");
+
+  // Main Process
+  let mainHs = payload.histograms;
+  let mainKhs = payload.keyedHistograms;
+  Assert.ok(!("TELEMETRY_TEST_CONTENT_PROCESS" in mainHs), "Should not have content process histogram in main process payload");
+  Assert.ok(!("TELEMETRY_TEST_KEYED_CONTENT_PROCESS" in mainKhs), "Should not have keyed content process histogram in main process payload");
+  Assert.ok(!("TELEMETRY_TEST_FLAG_CONTENT_PROCESS" in mainHs), "Should not have content process histogram in main process payload");
+  Assert.ok("TELEMETRY_TEST_ALL_PROCESSES" in mainHs, "Should have all-process histogram in main process payload");
+  Assert.equal(mainHs.TELEMETRY_TEST_ALL_PROCESSES.sum, 20, "Should have correct value");
+  Assert.ok(!("TELEMETRY_TEST_ALL_CHILD_PROCESSES" in mainHs), "Should not have all-child process histogram in main process payload");
+  Assert.ok("TELEMETRY_TEST_FLAG_MAIN_PROCESS" in mainHs, "Should have main process histogram in main process payload");
+  Assert.equal(mainHs.TELEMETRY_TEST_FLAG_MAIN_PROCESS.sum, 1, "Should have correct value");
+
   do_test_finished();
 });