Bug 1623529 - Fix supported features used in the profiler; r=julienw
authorGreg Tatum <gtatum@mozilla.com>
Tue, 24 Mar 2020 20:45:22 +0000
changeset 520278 d44263e0b47bc0435f6533e98ee28f0fef4c9279
parent 520277 5eacb3c300294f8de6d4dc81b627dac2cfb887a1
child 520279 bc5fee900c024eadf7cca28e7717aa947ff7ce94
push id37246
push useropoprus@mozilla.com
push dateWed, 25 Mar 2020 03:40:33 +0000
treeherdermozilla-central@14b59d4adc95 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjulienw
bugs1623529
milestone76.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 1623529 - Fix supported features used in the profiler; r=julienw about:profiling was failing to use the supported features of the target and was instead only checking the locally supported features. Differential Revision: https://phabricator.services.mozilla.com/D67528
devtools/client/performance-new/aboutprofiling/initializer.js
devtools/client/performance-new/initializer.js
devtools/client/performance-new/popup/background.jsm.js
devtools/client/performance-new/popup/panel.jsm.js
devtools/client/performance-new/test/browser/browser_aboutprofiling-presets-custom.js
devtools/client/performance-new/test/browser/browser_aboutprofiling-presets.js
devtools/client/performance-new/test/browser/browser_aboutprofiling-threads-behavior.js
devtools/client/performance-new/test/browser/browser_devtools-presets.js
devtools/client/performance-new/test/xpcshell/head.js
devtools/client/performance-new/test/xpcshell/test_popup_initial_state.js
--- a/devtools/client/performance-new/aboutprofiling/initializer.js
+++ b/devtools/client/performance-new/aboutprofiling/initializer.js
@@ -87,17 +87,20 @@ async function gInit(perfFront, pageCont
   // the browser.
   store.dispatch(
     actions.initializeStore({
       perfFront,
       receiveProfile,
       supportedFeatures,
       presets,
       // Get the preferences from the current browser
-      recordingPreferences: getRecordingPreferences(pageContext),
+      recordingPreferences: getRecordingPreferences(
+        pageContext,
+        supportedFeatures
+      ),
       /**
        * @param {RecordingStateFromPreferences} newRecordingPreferences
        */
       setRecordingPreferences: newRecordingPreferences =>
         setRecordingPreferences(pageContext, newRecordingPreferences),
 
       // The popup doesn't need to support remote symbol tables from the debuggee.
       // Only get the symbols from this browser.
--- a/devtools/client/performance-new/initializer.js
+++ b/devtools/client/performance-new/initializer.js
@@ -97,17 +97,20 @@ async function gInit(perfFront, pageCont
   }
 
   // Do some initialization, especially with privileged things that are part of the
   // the browser.
   store.dispatch(
     actions.initializeStore({
       perfFront,
       receiveProfile,
-      recordingPreferences: getRecordingPreferences(pageContext),
+      recordingPreferences: getRecordingPreferences(
+        pageContext,
+        supportedFeatures
+      ),
       presets,
       supportedFeatures,
       openAboutProfiling,
       pageContext: "devtools",
 
       // Go ahead and hide the implementation details for the component on how the
       // preference information is stored
       /**
--- a/devtools/client/performance-new/popup/background.jsm.js
+++ b/devtools/client/performance-new/popup/background.jsm.js
@@ -244,17 +244,19 @@ async function captureProfile() {
 function startProfiler(pageContext) {
   const { translatePreferencesToState } = lazyPreferenceManagement();
   const {
     entries,
     interval,
     features,
     threads,
     duration,
-  } = translatePreferencesToState(getRecordingPreferences(pageContext));
+  } = translatePreferencesToState(
+    getRecordingPreferences(pageContext, Services.profiler.GetFeatures())
+  );
 
   // Get the active BrowsingContext ID from browser.
   const { getActiveBrowsingContextID } = lazyRecordingUtils();
   const activeBrowsingContextID = getActiveBrowsingContextID();
 
   Services.profiler.StartProfiler(
     entries,
     interval,
@@ -336,79 +338,83 @@ function getPrefPostfix(pageContext) {
       const { UnhandledCaseError } = lazyUtils();
       throw new UnhandledCaseError(pageContext, "Page Context");
     }
   }
 }
 
 /**
  * @param {PageContext} pageContext
+ * @param {string[]} supportedFeatures
  * @returns {RecordingStateFromPreferences}
  */
-function getRecordingPreferences(pageContext) {
+function getRecordingPreferences(pageContext, supportedFeatures) {
   const postfix = getPrefPostfix(pageContext);
 
   // If you add a new preference here, please do not forget to update
   // `revertRecordingPreferences` as well.
   const objdirs = _getArrayOfStringsHostPref(OBJDIRS_PREF + postfix);
   const presetName = Services.prefs.getCharPref(PRESET_PREF + postfix);
 
   // First try to get the values from a preset.
-  const recordingPrefs = getRecordingPrefsFromPreset(presetName, objdirs);
+  const recordingPrefs = getRecordingPrefsFromPreset(
+    presetName,
+    supportedFeatures,
+    objdirs
+  );
   if (recordingPrefs) {
     return recordingPrefs;
   }
 
   // Next use the preferences to get the values.
   const entries = Services.prefs.getIntPref(ENTRIES_PREF + postfix);
   const interval = Services.prefs.getIntPref(INTERVAL_PREF + postfix);
   const features = _getArrayOfStringsPref(FEATURES_PREF + postfix);
   const threads = _getArrayOfStringsPref(THREADS_PREF + postfix);
   const duration = Services.prefs.getIntPref(DURATION_PREF + postfix);
 
-  const supportedFeatures = new Set(Services.profiler.GetFeatures());
-
   return {
     presetName: "custom",
     entries,
     interval,
     // Validate the features before passing them to the profiler.
-    features: features.filter(feature => supportedFeatures.has(feature)),
+    features: features.filter(feature => supportedFeatures.includes(feature)),
     threads,
     objdirs,
     duration,
   };
 }
 
 /**
  * @param {string} presetName
+ * @param {string[]} supportedFeatures
  * @param {string[]} objdirs
  * @return {RecordingStateFromPreferences | null}
  */
-function getRecordingPrefsFromPreset(presetName, objdirs) {
+function getRecordingPrefsFromPreset(presetName, supportedFeatures, objdirs) {
   if (presetName === "custom") {
     return null;
   }
 
   const preset = presets[presetName];
   if (!preset) {
     console.error(`Unknown profiler preset was encountered: "${presetName}"`);
     return null;
   }
 
-  const supportedFeatures = new Set(Services.profiler.GetFeatures());
-
   return {
     presetName,
     entries: preset.entries,
     // The interval is stored in preferences as microseconds, but the preset
     // defines it in terms of milliseconds. Make the conversion here.
     interval: preset.interval * 1000,
     // Validate the features before passing them to the profiler.
-    features: preset.features.filter(feature => supportedFeatures.has(feature)),
+    features: preset.features.filter(feature =>
+      supportedFeatures.includes(feature)
+    ),
     threads: preset.threads,
     objdirs,
     duration: preset.duration,
   };
 }
 
 /**
  * @param {PageContext} pageContext
@@ -453,29 +459,34 @@ function revertRecordingPreferences() {
   Services.prefs.clearUserPref(POPUP_FEATURE_FLAG_PREF);
 }
 
 /**
  * Change the prefs based on a preset. This mechanism is used by the popup to
  * easily switch between different settings.
  * @param {string} presetName
  * @param {PageContext} pageContext
+ * @param {string[]} supportedFeatures
  * @return {void}
  */
-function changePreset(pageContext, presetName) {
+function changePreset(pageContext, presetName, supportedFeatures) {
   const postfix = getPrefPostfix(pageContext);
   const objdirs = _getArrayOfStringsHostPref(OBJDIRS_PREF + postfix);
-  let recordingPrefs = getRecordingPrefsFromPreset(presetName, objdirs);
+  let recordingPrefs = getRecordingPrefsFromPreset(
+    presetName,
+    supportedFeatures,
+    objdirs
+  );
 
   if (!recordingPrefs) {
     // No recordingPrefs were found for that preset. Most likely this means this
     // is a custom preset, or it's one that we dont recognize for some reason.
     // Get the preferences from the individual preference values.
     Services.prefs.setCharPref(PRESET_PREF + postfix, presetName);
-    recordingPrefs = getRecordingPreferences(pageContext);
+    recordingPrefs = getRecordingPreferences(pageContext, supportedFeatures);
   }
 
   setRecordingPreferences(pageContext, recordingPrefs);
 }
 
 /**
  * This handler handles any messages coming from the WebChannel from profiler.firefox.com.
  *
--- a/devtools/client/performance-new/popup/panel.jsm.js
+++ b/devtools/client/performance-new/popup/panel.jsm.js
@@ -125,18 +125,22 @@ function createViewControllers(state, el
         const { height } = info.getBoundingClientRect();
         info.style.marginBlockEnd = `-${height}px`;
       } else {
         info.style.marginBlockEnd = "0";
       }
     },
 
     updatePresets() {
+      const { Services } = lazyServices();
       const { presets, getRecordingPreferences } = lazyBackground();
-      const { presetName } = getRecordingPreferences("aboutprofiling");
+      const { presetName } = getRecordingPreferences(
+        "aboutprofiling",
+        Services.profiler.GetFeatures()
+      );
       const preset = presets[presetName];
       if (preset) {
         elements.presetDescription.style.display = "block";
         elements.presetCustom.style.display = "none";
         elements.presetDescription.textContent = preset.description;
         elements.presetsMenuList.value = presetName;
       } else {
         elements.presetDescription.style.display = "none";
@@ -293,17 +297,21 @@ function addPopupEventHandlers(state, el
   });
 
   addHandler(elements.learnMore, "click", () => {
     elements.window.openWebLinkIn("https://profiler.firefox.com/docs/", "tab");
     view.hidePopup();
   });
 
   addHandler(elements.presetsMenuList, "command", () => {
-    changePreset("aboutprofiling", elements.presetsMenuList.value);
+    changePreset(
+      "aboutprofiling",
+      elements.presetsMenuList.value,
+      Services.profiler.GetFeatures()
+    );
     view.updatePresets();
   });
 
   addHandler(elements.presetsMenuList, "popuphidden", event => {
     // Changing a preset makes the popup autohide, this handler stops the
     // propagation of that event, so that only the menulist's popup closes,
     // and not the rest of the popup.
     event.stopPropagation();
--- a/devtools/client/performance-new/test/browser/browser_aboutprofiling-presets-custom.js
+++ b/devtools/client/performance-new/test/browser/browser_aboutprofiling-presets-custom.js
@@ -3,24 +3,29 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 add_task(async function test() {
   info(
     "Test that about:profiling presets override the individual settings when changed."
   );
+  const supportedFeatures = Services.profiler.GetFeatures();
 
-  if (!Services.profiler.GetFeatures().includes("stackwalk")) {
+  if (!supportedFeatures.includes("stackwalk")) {
     ok(true, "This platform does not support stackwalking, skip this test.");
     return;
   }
   // This test assumes that the Web Developer preset is set by default, which is
   // not the case on Nightly and custom builds.
-  BackgroundJSM.changePreset("aboutprofiling", "web-developer");
+  BackgroundJSM.changePreset(
+    "aboutprofiling",
+    "web-developer",
+    supportedFeatures
+  );
 
   await withAboutProfiling(async document => {
     const webdevPreset = await getNearestInputFromText(
       document,
       "Web Developer"
     );
     const customPreset = await getNearestInputFromText(document, "Custom");
     const stackwalkFeature = await getNearestInputFromText(
--- a/devtools/client/performance-new/test/browser/browser_aboutprofiling-presets.js
+++ b/devtools/client/performance-new/test/browser/browser_aboutprofiling-presets.js
@@ -8,17 +8,21 @@ add_task(async function test() {
   info("Test that about:profiling presets configure the profiler");
 
   if (!Services.profiler.GetFeatures().includes("stackwalk")) {
     ok(true, "This platform does not support stackwalking, skip this test.");
     return;
   }
   // This test assumes that the Web Developer preset is set by default, which is
   // not the case on Nightly and custom builds.
-  BackgroundJSM.changePreset("aboutprofiling", "web-developer");
+  BackgroundJSM.changePreset(
+    "aboutprofiling",
+    "web-developer",
+    Services.profiler.GetFeatures()
+  );
 
   await withAboutProfiling(async document => {
     const webdev = await getNearestInputFromText(document, "Web Developer");
     ok(webdev.checked, "By default the Web Developer preset is selected.");
 
     ok(
       !activeConfigurationHasFeature("stackwalk"),
       "Stackwalking is not enabled for the Web Developer workflow"
--- a/devtools/client/performance-new/test/browser/browser_aboutprofiling-threads-behavior.js
+++ b/devtools/client/performance-new/test/browser/browser_aboutprofiling-threads-behavior.js
@@ -5,17 +5,21 @@
 
 add_task(async function test() {
   info(
     "Test the behavior of thread toggling and the text summary works as expected."
   );
 
   // This test assumes that the Web Developer preset is set by default, which is
   // not the case on Nightly and custom builds.
-  BackgroundJSM.changePreset("aboutprofiling", "web-developer");
+  BackgroundJSM.changePreset(
+    "aboutprofiling",
+    "web-developer",
+    Services.profiler.GetFeatures()
+  );
 
   await withAboutProfiling(async document => {
     const threadTextEl = await getNearestInputFromText(
       document,
       "Add custom threads by name:"
     );
 
     is(
--- a/devtools/client/performance-new/test/browser/browser_devtools-presets.js
+++ b/devtools/client/performance-new/test/browser/browser_devtools-presets.js
@@ -8,17 +8,21 @@ add_task(async function test() {
 
   if (!Services.profiler.GetFeatures().includes("stackwalk")) {
     ok(true, "This platform does not support stackwalking, skip this test.");
     return;
   }
 
   // This test assumes that the Web Developer preset is set by default, which is
   // not the case on Nightly and custom builds.
-  BackgroundJSM.changePreset("aboutprofiling", "web-developer");
+  BackgroundJSM.changePreset(
+    "aboutprofiling",
+    "web-developer",
+    Services.profiler.GetFeatures()
+  );
 
   await withDevToolsPanel(async document => {
     {
       const presets = await getNearestInputFromText(document, "Settings");
 
       is(presets.value, "web-developer", "The presets default to webdev mode.");
       ok(
         !(await devToolsActiveConfigurationHasFeature(document, "stackwalk")),
--- a/devtools/client/performance-new/test/xpcshell/head.js
+++ b/devtools/client/performance-new/test/xpcshell/head.js
@@ -1,12 +1,14 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
+const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
+
 registerCleanupFunction(() => {
   // Always clean up the prefs after every test.
   const { revertRecordingPreferences } = ChromeUtils.import(
     "resource://devtools/client/performance-new/popup/background.jsm.js"
   );
   revertRecordingPreferences();
 });
--- a/devtools/client/performance-new/test/xpcshell/test_popup_initial_state.js
+++ b/devtools/client/performance-new/test/xpcshell/test_popup_initial_state.js
@@ -12,53 +12,58 @@ function setupBackgroundJsm() {
     "resource://devtools/client/performance-new/popup/background.jsm.js"
   );
 }
 
 add_task(function test() {
   info("Test that we get the default preference values from the browser.");
   const { getRecordingPreferences } = setupBackgroundJsm();
 
+  const preferences = getRecordingPreferences(
+    "aboutprofiling",
+    Services.profiler.GetFeatures()
+  );
+
   Assert.notEqual(
-    getRecordingPreferences("aboutprofiling").entries,
+    preferences.entries,
     undefined,
     "The initial state has the default entries."
   );
   Assert.notEqual(
-    getRecordingPreferences("aboutprofiling").interval,
+    preferences.interval,
     undefined,
     "The initial state has the default interval."
   );
   Assert.notEqual(
-    getRecordingPreferences("aboutprofiling").features,
+    preferences.features,
     undefined,
     "The initial state has the default features."
   );
   Assert.equal(
-    getRecordingPreferences("aboutprofiling").features.includes("js"),
+    preferences.features.includes("js"),
     true,
     "The js feature is initialized to the default."
   );
   Assert.notEqual(
-    getRecordingPreferences("aboutprofiling").threads,
+    preferences.threads,
     undefined,
     "The initial state has the default threads."
   );
   Assert.equal(
-    getRecordingPreferences("aboutprofiling").threads.includes("GeckoMain"),
+    preferences.threads.includes("GeckoMain"),
     true,
     "The GeckoMain thread is initialized to the default."
   );
   Assert.notEqual(
-    getRecordingPreferences("aboutprofiling").objdirs,
+    preferences.objdirs,
     undefined,
     "The initial state has the default objdirs."
   );
   Assert.notEqual(
-    getRecordingPreferences("aboutprofiling").duration,
+    preferences.duration,
     undefined,
     "The duration is initialized to the duration."
   );
 });
 
 add_task(function test() {
   info(
     "Test that the state and features are properly validated. This ensures that as " +
@@ -66,31 +71,40 @@ add_task(function test() {
       "Profiler interface to crash with invalid values."
   );
   const {
     getRecordingPreferences,
     setRecordingPreferences,
     changePreset,
   } = setupBackgroundJsm();
 
-  changePreset("aboutprofiling", "custom");
+  const supportedFeatures = Services.profiler.GetFeatures();
+
+  changePreset("aboutprofiling", "custom", supportedFeatures);
 
   Assert.ok(
-    getRecordingPreferences("aboutprofiling").features.includes("js"),
+    getRecordingPreferences(
+      "aboutprofiling",
+      supportedFeatures
+    ).features.includes("js"),
     "The js preference is present initially."
   );
 
-  const settings = getRecordingPreferences("aboutprofiling");
+  const settings = getRecordingPreferences("aboutprofiling", supportedFeatures);
   settings.features = settings.features.filter(feature => feature !== "js");
   settings.features.push("UNKNOWN_FEATURE_FOR_TESTS");
   setRecordingPreferences("aboutprofiling", settings);
 
   Assert.ok(
-    !getRecordingPreferences("aboutprofiling").features.includes(
-      "UNKNOWN_FEATURE_FOR_TESTS"
-    ),
+    !getRecordingPreferences(
+      "aboutprofiling",
+      supportedFeatures
+    ).features.includes("UNKNOWN_FEATURE_FOR_TESTS"),
     "The unknown feature is removed."
   );
   Assert.ok(
-    !getRecordingPreferences("aboutprofiling").features.includes("js"),
+    !getRecordingPreferences(
+      "aboutprofiling",
+      supportedFeatures
+    ).features.includes("js"),
     "The js preference is still flipped from the default."
   );
 });