Bug 1667426 - fisison.autostart/normandy pref integration improvements, r=kmag,mythmon
☠☠ backed out by 0acb34a4e5f3 ☠ ☠
authorNika Layzell <nika@thelayzells.com>
Thu, 01 Oct 2020 15:56:14 +0000
changeset 551143 fdd402fe6537b1d0e5ff81261f4cd7a1216d9e29
parent 551142 969a3a2a7b6538e25d86d2b48deca4bc9de71349
child 551144 0a0ad7d6a16b9cb5d72b0bbb388409054cc3d919
push id127689
push usernlayzell@mozilla.com
push dateThu, 01 Oct 2020 18:45:44 +0000
treeherderautoland@0a0ad7d6a16b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag, mythmon
bugs1667426
milestone83.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 1667426 - fisison.autostart/normandy pref integration improvements, r=kmag,mythmon Differential Revision: https://phabricator.services.mozilla.com/D91687
modules/libpref/init/StaticPrefList.yaml
toolkit/xre/nsAppRunner.cpp
toolkit/xre/test/marionette/test_fission_autostart.py
xpcom/system/nsIXULRuntime.idl
--- a/modules/libpref/init/StaticPrefList.yaml
+++ b/modules/libpref/init/StaticPrefList.yaml
@@ -3555,27 +3555,29 @@
   value: false
   mirror: always
 
 #---------------------------------------------------------------------------
 # Prefs starting with "fission."
 #---------------------------------------------------------------------------
 
 # Whether to enable Fission in new windows by default.
-# IMPORTANT: This preference should *almost never* be checked directly, since
-# any session can contain a mix of Fission and non-Fission windows. Instead,
+# IMPORTANT: This preference should *never* be checked directly, since any
+# session can contain a mix of Fission and non-Fission windows. Instead,
 # callers should check whether the relevant nsILoadContext has the
 # `useRemoteSubframes` flag set.
+# Callers which cannot use `useRemoteSubframes` must use
+# `Services.appinfo.fissionAutostart` or `mozilla::FissionAutostart()` to check
+# if fission is enabled by default.
 # Note: This is overridden in all.js on RELEASE_OR_BETA in order to add the
 # locked attribute.
 - name: fission.autostart
   type: bool
   value: false
-  mirror: once
-  do_not_use_directly: true
+  mirror: never
 
 # This pref has no effect within fission windows, it only controls the
 # behaviour within non-fission windows. If true, preserve browsing contexts
 # between process swaps.
 - name: fission.preserve_browsing_contexts
   type: bool
   value: true
   mirror: always
--- a/toolkit/xre/nsAppRunner.cpp
+++ b/toolkit/xre/nsAppRunner.cpp
@@ -462,81 +462,155 @@ bool gFxREmbedded = false;
 // The following factors determine whether or not Fission is enabled for a
 // session, in order of precedence:
 //
 // - Safe mode: In safe mode, Fission is never enabled.
 //
 // - The MOZ_FORCE_ENABLE_FISSION environment variable: If set to any value,
 //   Fission will be enabled.
 //
-// - The following preferences:
+// - The 'fission.autostart' preference, if it has been configured by the user.
+static const char kPrefFissionAutostart[] = "fission.autostart";
+//
+// - The fission experiment enrollment status set during the previous run, which
+//   is controlled by the following preferences:
 //
 // The current enrollment status as controlled by Normandy. This value is only
 // stored in the default preference branch, and is not persisted across
 // sessions by the preference service. It therefore isn't available early
 // enough at startup, and needs to be synced to a preference in the user
 // branch which is persisted across sessions.
 static const char kPrefFissionExperimentEnrollmentStatus[] =
     "fission.experiment.enrollmentStatus";
 //
 // The enrollment status to be used at browser startup. This automatically
 // synced from the above enrollmentStatus preference whenever the latter is
 // changed. It can have any of the values defined in the
 // `nsIXULRuntime_ExperimentStatus` enum. Meanings are documented in
 // the declaration of `nsIXULRuntime.fissionExperimentStatus`
 static const char kPrefFissionExperimentStartupEnrollmentStatus[] =
     "fission.experiment.startupEnrollmentStatus";
+
+// The computed FissionAutostart value for the session, read by content
+// processes to initialize gFissionAutostart.
 //
-// The "fission.autostart" preference: If none of the above conditions apply,
-// and the experiment enrollment status is unknown, then the value of the
-// "fisison.autostart" preference is used.
-
-static nsIXULRuntime::ExperimentStatus FissionExperimentStatus() {
-  static nsIXULRuntime::ExperimentStatus sExperimentStatus = ([]() {
-    uint32_t value =
-        Preferences::GetUint(kPrefFissionExperimentStartupEnrollmentStatus);
-    if (value >
-        uint32_t(nsIXULRuntime::ExperimentStatus::eExperimentStatusMax)) {
-      return nsIXULRuntime::ExperimentStatus::eUnknown;
-    }
-    return nsIXULRuntime::ExperimentStatus(value);
-  })();
-
-  return sExperimentStatus;
+// This pref is locked, and only configured on the default branch, so should
+// never be persisted in a profile.
+static const char kPrefFissionAutostartSession[] = "fission.autostart.session";
+
+static nsIXULRuntime::ExperimentStatus gFissionExperimentStatus =
+    nsIXULRuntime::eExperimentStatusUnenrolled;
+static bool gFissionAutostart = false;
+static bool gFissionAutostartInitialized = false;
+
+static bool FissionExperimentEnrolled() {
+  MOZ_ASSERT(XRE_IsParentProcess());
+  return gFissionExperimentStatus == nsIXULRuntime::eExperimentStatusControl ||
+         gFissionExperimentStatus == nsIXULRuntime::eExperimentStatusTreatment;
+}
+
+static void FissionExperimentDisqualify() {
+  MOZ_ASSERT(XRE_IsParentProcess());
+  // Setting this pref's user value will be detected by Normandy, causing the
+  // client to be unenrolled from the experiment.
+  Preferences::SetUint(kPrefFissionExperimentEnrollmentStatus,
+                       nsIXULRuntime::eExperimentStatusDisqualified);
 }
 
 static void OnFissionEnrollmentStatusChanged(const char* aPref, void* aData) {
   Preferences::SetUint(
       kPrefFissionExperimentStartupEnrollmentStatus,
       Preferences::GetUint(kPrefFissionExperimentEnrollmentStatus));
 }
 
+static void OnFissionAutostartChanged(const char* aPref, void* aData) {
+  MOZ_ASSERT(FissionExperimentEnrolled());
+  if (Preferences::HasUserValue(kPrefFissionAutostart)) {
+    FissionExperimentDisqualify();
+  }
+}
+
+static void EnsureFissionAutostartInitialized() {
+  if (gFissionAutostartInitialized) {
+    return;
+  }
+  gFissionAutostartInitialized = true;
+
+  if (!XRE_IsParentProcess()) {
+    // This pref is configured for the current session by the parent process.
+    gFissionAutostart = Preferences::GetBool(kPrefFissionAutostartSession,
+                                             false, PrefValueKind::Default);
+    return;
+  }
+
+  // Initialize the fission experiment, configuring fission.autostart's
+  // default, before checking other overrides. This allows opting-out of a
+  // fission experiment through about:preferences or about:config from a
+  // safemode session.
+  uint32_t experimentRaw =
+      Preferences::GetUint(kPrefFissionExperimentStartupEnrollmentStatus,
+                           nsIXULRuntime::eExperimentStatusUnenrolled);
+  gFissionExperimentStatus =
+      experimentRaw < nsIXULRuntime::eExperimentStatusCount
+          ? nsIXULRuntime::ExperimentStatus(experimentRaw)
+          : nsIXULRuntime::eExperimentStatusDisqualified;
+
+  // If the user has overridden an active experiment by setting a user value for
+  // "fission.autostart", disqualify the user from the experiment.
+  if (Preferences::HasUserValue(kPrefFissionAutostart) &&
+      FissionExperimentEnrolled()) {
+    FissionExperimentDisqualify();
+    gFissionExperimentStatus = nsIXULRuntime::eExperimentStatusDisqualified;
+  }
+
+  // Configure the default branch for "fission.autostart" based on experiment
+  // enrollment status.
+  if (FissionExperimentEnrolled()) {
+    bool isTreatment =
+        gFissionExperimentStatus == nsIXULRuntime::eExperimentStatusTreatment;
+    Preferences::SetBool(kPrefFissionAutostart, isTreatment,
+                         PrefValueKind::Default);
+  }
+
+  if (gSafeMode) {
+    gFissionAutostart = false;
+  } else if (EnvHasValue("MOZ_FORCE_ENABLE_FISSION")) {
+    gFissionAutostart = true;
+  } else {
+    // NOTE: This will take into account changes to the default due to
+    // `InitializeFissionExperimentStatus`.
+    gFissionAutostart = Preferences::GetBool(kPrefFissionAutostart, false);
+  }
+
+  // Content processes cannot run the same logic as we're running in the parent
+  // process, as the current value of various preferences may have changed
+  // between launches. Instead, the content process will read the default branch
+  // of the locked `fission.autostart.session` preference to determine the value
+  // determined by this method.
+  Preferences::Unlock(kPrefFissionAutostartSession);
+  Preferences::ClearUser(kPrefFissionAutostartSession);
+  Preferences::SetBool(kPrefFissionAutostartSession, gFissionAutostart,
+                       PrefValueKind::Default);
+  Preferences::Lock(kPrefFissionAutostartSession);
+
+  // Watch prefs which may be modified at runtime to configure experiment
+  // enrollment status for the next launch.
+  Preferences::RegisterCallback(&OnFissionEnrollmentStatusChanged,
+                                kPrefFissionExperimentEnrollmentStatus);
+  if (FissionExperimentEnrolled()) {
+    Preferences::RegisterCallback(&OnFissionAutostartChanged,
+                                  kPrefFissionAutostart);
+  }
+}
+
 namespace mozilla {
 
 bool FissionAutostart() {
-  static bool sFissionAutostart = ([]() {
-    if (gSafeMode) {
-      return false;
-    }
-
-    if (EnvHasValue("MOZ_FORCE_ENABLE_FISSION")) {
-      return true;
-    }
-
-    switch (FissionExperimentStatus()) {
-      case nsIXULRuntime::ExperimentStatus::eEnrolledControl:
-        return false;
-      case nsIXULRuntime::ExperimentStatus::eEnrolledTreatment:
-        return true;
-      default:
-        return StaticPrefs::fission_autostart_AtStartup_DoNotUseDirectly();
-    }
-  })();
-
-  return sFissionAutostart;
+  EnsureFissionAutostartInitialized();
+  return gFissionAutostart;
 }
 
 bool SessionHistoryInParent() {
   // Fission check will be enabled later.
   return /*FissionAutostart() ||*/
       StaticPrefs::fission_sessionHistoryInParent_AtStartup_DoNotUseDirectly();
 }
 
@@ -864,17 +938,22 @@ nsXULAppInfo::Observe(nsISupports* aSubj
 NS_IMETHODIMP
 nsXULAppInfo::GetFissionAutostart(bool* aResult) {
   *aResult = FissionAutostart();
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsXULAppInfo::GetFissionExperimentStatus(ExperimentStatus* aResult) {
-  *aResult = FissionExperimentStatus();
+  if (!XRE_IsParentProcess()) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
+  EnsureFissionAutostartInitialized();
+  *aResult = gFissionExperimentStatus;
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsXULAppInfo::GetSessionHistoryInParent(bool* aResult) {
   *aResult = SessionHistoryInParent();
   return NS_OK;
 }
@@ -4727,19 +4806,16 @@ nsresult XREMain::XRE_mainRun() {
     Preferences::RegisterCallbackAndCall(
         &OnDefaultAgentRemoteSettingsPrefChanged, kPrefServicesSettingsServer);
     Preferences::RegisterCallbackAndCall(
         &OnDefaultAgentRemoteSettingsPrefChanged,
         kPrefSecurityContentSignatureRootHash);
 #  endif  // defined(MOZ_DEFAULT_BROWSER_AGENT)
 #endif
 
-    Preferences::RegisterCallback(&OnFissionEnrollmentStatusChanged,
-                                  kPrefFissionExperimentEnrollmentStatus);
-
 #if defined(HAVE_DESKTOP_STARTUP_ID) && defined(MOZ_WIDGET_GTK)
     // Clear the environment variable so it won't be inherited by
     // child processes and confuse things.
     g_unsetenv("DESKTOP_STARTUP_ID");
 #endif
 
 #ifdef XP_MACOSX
     // we re-initialize the command-line service and do appleevents munging
--- a/toolkit/xre/test/marionette/test_fission_autostart.py
+++ b/toolkit/xre/test/marionette/test_fission_autostart.py
@@ -1,23 +1,25 @@
 from __future__ import absolute_import, print_function
 
 from marionette_harness import MarionetteTestCase
 
 
 class ExperimentStatus:
-    UNKNOWN = 0
+    UNENROLLED = 0
     ENROLLED_CONTROL = 1
     ENROLLED_TREATMENT = 2
+    DISQUALIFIED = 3
 
 
 class Prefs:
     ENROLLMENT_STATUS = 'fission.experiment.enrollmentStatus'
     STARTUP_ENROLLMENT_STATUS = 'fission.experiment.startupEnrollmentStatus'
     FISSION_AUTOSTART = 'fission.autostart'
+    FISSION_AUTOSTART_SESSION = 'fission.autostart.session'
 
 
 ENV_ENABLE_FISSION = 'MOZ_FORCE_ENABLE_FISSION'
 
 
 class TestFissionAutostart(MarionetteTestCase):
     SANDBOX_NAME = 'fission-autostart'
 
@@ -30,24 +32,31 @@ class TestFissionAutostart(MarionetteTes
 
     def get_fission_status(self):
         return self.execute_script(r'''
           let win = Services.wm.getMostRecentWindow("navigator:browser");
           return {
             fissionAutostart: Services.appinfo.fissionAutostart,
             fissionExperimentStatus: Services.appinfo.fissionExperimentStatus,
             useRemoteSubframes: win.docShell.nsILoadContext.useRemoteSubframes,
+            fissionAutostartSession: Services.prefs.getBoolPref("fission.autostart.session"),
+            dynamicFissionAutostart: Services.prefs.getBoolPref("fission.autostart"),
           };
         ''')
 
-    def check_fission_status(self, enabled, experiment):
+    def check_fission_status(self, enabled, experiment, dynamic=None):
+        if dynamic is None:
+            dynamic = enabled
+
         expected = {
             'fissionAutostart': enabled,
             'fissionExperimentStatus': experiment,
             'useRemoteSubframes': enabled,
+            'fissionAutostartSession': enabled,
+            'dynamicFissionAutostart': dynamic,
         }
 
         status = self.get_fission_status()
 
         for prop, value in expected.items():
             self.assertEqual(
                 status[prop], value,
                 '%s should have the value `%r`, but has `%r`'
@@ -81,17 +90,18 @@ class TestFissionAutostart(MarionetteTes
                 self.set_env(name, value)
 
         self.marionette.restart(in_app=True, clean=False)
         self.setUpSession()
 
         # Sanity check our environment.
         if prefs:
             for key, val in prefs.items():
-                self.assertEqual(self.marionette.get_pref(key), val)
+                if val is not None:
+                    self.assertEqual(self.marionette.get_pref(key), val)
         if env:
             for key, val in env.items():
                 self.assertEqual(self.get_env(key), val or '')
 
     def setUpSession(self):
         self.marionette.set_context(self.marionette.CONTEXT_CHROME)
 
         self.execute_script(r'''
@@ -116,68 +126,90 @@ class TestFissionAutostart(MarionetteTes
     def test_runtime_changes(self):
         """Tests that changes to preferences during runtime do not have any
         effect on the current session."""
 
         if self.marionette.instance.required_prefs.get(Prefs.FISSION_AUTOSTART):
             # Need to be able to flip Fission prefs for this test to work.
             return
 
+        self.check_fission_status(enabled=False,
+                                  experiment=ExperimentStatus.UNENROLLED)
+
         self.restart(prefs={Prefs.FISSION_AUTOSTART: True})
 
         self.check_fission_status(enabled=True,
-                                  experiment=ExperimentStatus.UNKNOWN)
+                                  experiment=ExperimentStatus.UNENROLLED)
 
         self.set_enrollment_status(ExperimentStatus.ENROLLED_CONTROL)
         self.check_fission_status(enabled=True,
-                                  experiment=ExperimentStatus.UNKNOWN)
+                                  experiment=ExperimentStatus.UNENROLLED)
 
         self.marionette.set_pref(Prefs.FISSION_AUTOSTART,
                                  False)
         self.check_fission_status(enabled=True,
-                                  experiment=ExperimentStatus.UNKNOWN)
+                                  experiment=ExperimentStatus.UNENROLLED,
+                                  dynamic=False)
+
+        self.marionette.clear_pref(Prefs.FISSION_AUTOSTART)
+        self.check_fission_status(enabled=True,
+                                  experiment=ExperimentStatus.UNENROLLED,
+                                  dynamic=False)
 
         self.restart()
         self.check_fission_status(enabled=False,
                                   experiment=ExperimentStatus.ENROLLED_CONTROL)
 
         self.marionette.set_pref(Prefs.ENROLLMENT_STATUS,
-                                 ExperimentStatus.UNKNOWN,
+                                 ExperimentStatus.UNENROLLED,
                                  default_branch=True)
         self.check_fission_status(enabled=False,
                                   experiment=ExperimentStatus.ENROLLED_CONTROL)
 
         self.set_env(ENV_ENABLE_FISSION, '1')
         self.check_fission_status(enabled=False,
                                   experiment=ExperimentStatus.ENROLLED_CONTROL)
 
     def test_fission_precedence(self):
         if self.marionette.instance.required_prefs.get(Prefs.FISSION_AUTOSTART):
             # Need to be able to flip Fission prefs for this test to work.
             return
 
+        self.check_fission_status(enabled=False,
+                                  experiment=ExperimentStatus.UNENROLLED)
+
         self.restart(prefs={Prefs.FISSION_AUTOSTART: False},
                      env={ENV_ENABLE_FISSION: '1'})
         self.check_fission_status(enabled=True,
-                                  experiment=ExperimentStatus.UNKNOWN)
+                                  experiment=ExperimentStatus.UNENROLLED,
+                                  dynamic=False)
 
         self.restart(prefs={Prefs.FISSION_AUTOSTART: True},
                      env={ENV_ENABLE_FISSION: ''})
         self.check_fission_status(enabled=True,
-                                  experiment=ExperimentStatus.UNKNOWN)
+                                  experiment=ExperimentStatus.UNENROLLED)
 
-        self.restart(prefs={Prefs.FISSION_AUTOSTART: False})
+        self.restart(prefs={Prefs.FISSION_AUTOSTART: None})
         self.check_fission_status(enabled=False,
-                                  experiment=ExperimentStatus.UNKNOWN)
+                                  experiment=ExperimentStatus.UNENROLLED)
 
         self.set_enrollment_status(ExperimentStatus.ENROLLED_TREATMENT)
         self.restart()
         self.check_fission_status(enabled=True,
                                   experiment=ExperimentStatus.ENROLLED_TREATMENT)
 
         self.set_enrollment_status(ExperimentStatus.ENROLLED_CONTROL)
         self.restart()
         self.check_fission_status(enabled=False,
                                   experiment=ExperimentStatus.ENROLLED_CONTROL)
 
-        self.restart(prefs={Prefs.FISSION_AUTOSTART: True})
+        self.marionette.set_pref(Prefs.FISSION_AUTOSTART, True)
         self.check_fission_status(enabled=False,
-                                  experiment=ExperimentStatus.ENROLLED_CONTROL)
+                                  experiment=ExperimentStatus.ENROLLED_CONTROL,
+                                  dynamic=True)
+
+        self.assertEqual(self.marionette.get_pref(Prefs.ENROLLMENT_STATUS),
+                         ExperimentStatus.DISQUALIFIED,
+                         'Setting fission.autostart should disqualify')
+
+        self.restart()
+        self.check_fission_status(enabled=True,
+                                  experiment=ExperimentStatus.DISQUALIFIED)
--- a/xpcom/system/nsIXULRuntime.idl
+++ b/xpcom/system/nsIXULRuntime.idl
@@ -41,50 +41,48 @@ bool SessionHistoryInParent();
 interface nsIXULRuntime : nsISupports
 {
   /**
    * Whether the application was launched in safe mode.
    */
   readonly attribute boolean inSafeMode;
 
   /**
-   * The status of a given experiment, as stored in a preference by Normandy.
+   * The status of a given normandy experiment.
    */
   cenum ExperimentStatus : 8 {
-    // The experiment status is unknown, and behavior should not be modified.
-    eUnknown = 0,
+    // The user is not actively enrolled in the experiment.
+    eExperimentStatusUnenrolled = 0,
     // The user is enrolled in the control group, and should see the default
     // behavior.
-    eEnrolledControl = 1,
+    eExperimentStatusControl = 1,
     // The user is enrolled in the treatment group, and should see the
     // experimental behavior which is being tested.
-    eEnrolledTreatment = 2,
-    eExperimentStatusMax = 2,
+    eExperimentStatusTreatment = 2,
+    // The user was enrolled in the experiment, but became ineligible due to
+    // manually modifying a relevant preference.
+    eExperimentStatusDisqualified = 3,
+
+    eExperimentStatusCount,
   };
 
   /**
    * Whether Fission should be automatically enabled for new browser windows.
+   * This may not match the value of the 'fission.autostart' pref.
    *
    * This value is guaranteed to remain constant for the length of a browser
    * session.
    */
   readonly attribute boolean fissionAutostart;
 
   /**
-   * The user's enrollment status in the Fission rollout experiment. May be
-   * one of:
-   *
-   *  - eUnknown: The user's enrollment status is unknown, and the experiment
-   *    should be ignored.
+   * The user's enrollment status in the Fission experiment at process startup.
+   * See `ExperimentStatus` for the potential values.
    *
-   *  - eEnrolledControl: The user is enrolled in the control group, and Fission
-   *    should be disabled.
-   *
-   *  - eEnrolledTreatment: The user is enrolled in the treatment group, and
-   *    Fission should be enabled.
+   * Only available in the parent process.
    *
    * This value is guaranteed to remain constant for the length of a browser
    * session.
    */
   readonly attribute nsIXULRuntime_ExperimentStatus fissionExperimentStatus;
 
   /**
    * Whether session history is stored in the parent process.