Bug 1530361 - Properly check if child process is allowed r=chutten
authorJan-Erik Rediger <jrediger@mozilla.com>
Mon, 25 Feb 2019 18:27:32 +0000
changeset 518834 c3fd93ab694f412a09a85567acf52699af0abf94
parent 518833 98486a2bc3c60878d4fce5073eacff3bd1df17b1
child 518835 7d429f711f4d58978174ed1a4ec5812f675559a3
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschutten
bugs1530361
milestone67.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 1530361 - Properly check if child process is allowed r=chutten RecordedProcessType::Main is always equal to 1 (now ensured by an assert). `AllChildren` is always `All-1`. The process type always has only a single bit set. We can therefore reduce this to a simple bit check against the allowed values. Differential Revision: https://phabricator.services.mozilla.com/D21021
toolkit/components/telemetry/core/TelemetryCommon.cpp
toolkit/components/telemetry/core/TelemetryCommon.h
toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
--- a/toolkit/components/telemetry/core/TelemetryCommon.cpp
+++ b/toolkit/components/telemetry/core/TelemetryCommon.cpp
@@ -60,24 +60,22 @@ bool CanRecordDataset(uint32_t aDataset,
 
   // We're not recording extended telemetry or this is not the base
   // dataset. Bail out.
   return false;
 }
 
 bool CanRecordInProcess(RecordedProcessType processes,
                         GeckoProcessType processType) {
-  bool recordAllChildren = !!(processes & RecordedProcessType::AllChildren);
   // We can use (1 << ProcessType) due to the way RecordedProcessType is
   // defined.
   bool canRecordProcess =
       !!(processes & static_cast<RecordedProcessType>(1 << processType));
 
-  return canRecordProcess ||
-         ((processType != GeckoProcessType_Default) && recordAllChildren);
+  return canRecordProcess;
 }
 
 bool CanRecordInProcess(RecordedProcessType processes, ProcessID processId) {
   return CanRecordInProcess(processes, GetGeckoProcessType(processId));
 }
 
 bool CanRecordProduct(SupportedProduct aProducts) {
   return !!(aProducts & GetCurrentProduct());
--- a/toolkit/components/telemetry/core/TelemetryCommon.h
+++ b/toolkit/components/telemetry/core/TelemetryCommon.h
@@ -21,19 +21,23 @@ namespace Common {
 typedef nsTHashtable<nsCStringHashKey> StringHashSet;
 
 enum class RecordedProcessType : uint16_t {
   Main = (1 << GeckoProcessType_Default),  // Also known as "parent process"
   Content = (1 << GeckoProcessType_Content),
   Gpu = (1 << GeckoProcessType_GPU),
   Socket = (1 << GeckoProcessType_Socket),
   AllChildren = 0xFFFF - 1,  // All the child processes (i.e. content, gpu, ...)
+                             // Always `All-Main` to allow easy matching.
   All = 0xFFFF               // All the processes
 };
 MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(RecordedProcessType);
+static_assert(static_cast<uint16_t>(RecordedProcessType::Main) == 1,
+              "Main process type must be equal to 1 to allow easy matching in "
+              "CanRecordInProcess");
 
 enum class SupportedProduct : uint8_t {
   Firefox = (1 << 0),
   Fennec = (1 << 1),
   Geckoview = (1 << 2),
   All = 0xFF  // All the products
 };
 MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(SupportedProduct);
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ -1614,8 +1614,20 @@ add_task(async function test_multistore_
   hist.add("key-1", 3);
   Assert.deepEqual(undefined, hist.snapshot({store: "main"}));
   Assert.deepEqual(40, hist.snapshot({store: "sync"})["key-1"].sum);
 
   hist.clear({store: "sync"});
   Assert.deepEqual(undefined, hist.snapshot({store: "main"}));
   Assert.deepEqual({}, hist.snapshot({store: "sync"}));
 });
+
+add_task(async function test_can_record_in_process_regression_bug_1530361() {
+  Telemetry.getSnapshotForHistograms("main", true);
+
+  // The socket and gpu processes should not have any histograms.
+  // Flag and count histograms have defaults, so if we're accidentally recording them
+  // in these processes they'd show up even immediately after being cleared.
+  let snapshot = Telemetry.getSnapshotForHistograms("main", true);
+
+  Assert.deepEqual(snapshot.gpu, {}, "No histograms should have been recorded for the gpu process");
+  Assert.deepEqual(snapshot.socket, {}, "No histograms should have been recorded for the socket process");
+});