Bug 1669749 - Fix edge-cases with fission experiment pref integration, r=kmag

This should fix some of the issues discovered by QA during their review of the
fission experiment's preference behaviour.

Differential Revision: https://phabricator.services.mozilla.com/D92780

modules/libpref/init/StaticPrefList.yaml

@@ -3567,6 +3567,18 @@
   value: false
   mirror: never
 
+# Prefs used by normandy to orchestrate the fission experiment. For more
+# details, see the comments in nsAppRunner.cpp.
+- name: fission.experiment.enrollmentStatus
+  type: uint32_t
+  value: 0
+  mirror: never
+
+- name: fission.experiment.startupEnrollmentStatus
+  type: uint32_t
+  value: 0
+  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.

toolkit/xre/nsAppRunner.cpp

@@ -597,9 +597,32 @@ static void FissionExperimentDisqualify() {
 static void OnFissionEnrollmentStatusChanged(const char* aPref, void* aData) {
   Preferences::SetUint(
       kPrefFissionExperimentStartupEnrollmentStatus,
-      Preferences::GetUint(kPrefFissionExperimentEnrollmentStatus));
+      Preferences::GetUint(kPrefFissionExperimentEnrollmentStatus,
+                           nsIXULRuntime::eExperimentStatusUnenrolled));
 }
 
+namespace {
+// This observer is notified during `profile-before-change`, and ensures that
+// the experiment enrollment status is synced over before the browser shuts
+// down, even if it was not modified since FissionAutostart was initialized.
+class FissionEnrollmentStatusShutdownObserver final : public nsIObserver {
+ public:
+  NS_DECL_ISUPPORTS
+
+  NS_IMETHOD Observe(nsISupports* aSubject, const char* aTopic,
+                     const char16_t* aData) override {
+    MOZ_ASSERT(!strcmp("profile-before-change", aTopic));
+    OnFissionEnrollmentStatusChanged(kPrefFissionExperimentEnrollmentStatus,
+                                     nullptr);
+    return NS_OK;
+  }
+
+ private:
+  ~FissionEnrollmentStatusShutdownObserver() = default;
+};
+NS_IMPL_ISUPPORTS(FissionEnrollmentStatusShutdownObserver, nsIObserver)
+}  // namespace
+
 static void OnFissionAutostartChanged(const char* aPref, void* aData) {
   MOZ_ASSERT(FissionExperimentEnrolled());
   if (Preferences::HasUserValue(kPrefFissionAutostart)) {
@@ -632,6 +655,18 @@ static void EnsureFissionAutostartInitialized() {
           ? nsIXULRuntime::ExperimentStatus(experimentRaw)
           : nsIXULRuntime::eExperimentStatusDisqualified;
 
+  // Watch the experiment enrollment status pref to detect experiment
+  // disqualification, and ensure it is propagated for the next restart.
+  Preferences::RegisterCallback(&OnFissionEnrollmentStatusChanged,
+                                kPrefFissionExperimentEnrollmentStatus);
+  if (nsCOMPtr<nsIObserverService> observerService =
+          mozilla::services::GetObserverService()) {
+    nsCOMPtr<nsIObserver> shutdownObserver =
+        new FissionEnrollmentStatusShutdownObserver();
+    observerService->AddObserver(shutdownObserver, "profile-before-change",
+                                 false);
+  }
+
   // 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) &&
@@ -693,10 +728,8 @@ static void EnsureFissionAutostartInitialized() {
                        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 we're actively enrolled in the fission experiment, disqualify the user
+  // from the experiment if the fission pref is modified.
   if (FissionExperimentEnrolled()) {
     Preferences::RegisterCallback(&OnFissionAutostartChanged,
                                   kPrefFissionAutostart);

toolkit/xre/test/marionette/test_fission_autostart.py

@@ -1,6 +1,7 @@
 from __future__ import absolute_import, print_function
 
 from marionette_harness import MarionetteTestCase
+from contextlib import contextmanager
 
 
 class ExperimentStatus:
@@ -154,6 +155,16 @@ class TestFissionAutostart(MarionetteTestCase):
                     .getService(Ci.nsIEnvironment);
         ''')
 
+    @contextmanager
+    def full_restart(self):
+        profile = self.marionette.instance.profile
+        try:
+            self.marionette.quit(in_app=True, clean=False)
+            yield profile
+        finally:
+            self.marionette.start_session()
+            self.setUpSession()
+
     def setUp(self):
         super(TestFissionAutostart, self).setUp()
 
@@ -277,3 +288,59 @@ class TestFissionAutostart(MarionetteTestCase):
                                   experiment=ExperimentStatus.DISQUALIFIED,
                                   decision='disabledByE10sEnv',
                                   dynamic=True)
+
+    def test_fission_startup(self):
+        if self.check_pref_locked():
+            # Need to be able to flip Fission prefs for this test to work.
+            return
+
+        # Starting the browser with STARTUP_ENROLLMENT_STATUS set to treatment
+        # should make the current session run under treatment.
+        with self.full_restart() as profile:
+            profile.set_preferences({
+                Prefs.STARTUP_ENROLLMENT_STATUS: ExperimentStatus.ENROLLED_TREATMENT,
+            }, filename='prefs.js')
+
+        self.assertEqual(self.marionette.get_pref(Prefs.ENROLLMENT_STATUS),
+                         ExperimentStatus.UNENROLLED,
+                         'Dynamic pref should be unenrolled')
+        self.assertEqual(self.marionette.get_pref(Prefs.STARTUP_ENROLLMENT_STATUS),
+                         ExperimentStatus.ENROLLED_TREATMENT,
+                         'Startup pref should be in treatment')
+        self.check_fission_status(enabled=True,
+                                  experiment=ExperimentStatus.ENROLLED_TREATMENT,
+                                  decision='experimentTreatment')
+
+        # If normandy doesn't re-set `ENROLLMENT_STATUS` during the session, it
+        # should be cleared back to disabled after a restart.
+        self.marionette.restart(in_app=True, clean=False)
+
+        self.assertEqual(self.marionette.get_pref(Prefs.ENROLLMENT_STATUS),
+                         ExperimentStatus.UNENROLLED,
+                         'Should unenroll dynamic pref after shutdown')
+        self.assertEqual(self.marionette.get_pref(Prefs.STARTUP_ENROLLMENT_STATUS),
+                         ExperimentStatus.UNENROLLED,
+                         'Should unenroll startup pref after shutdown')
+        self.check_fission_status(enabled=False,
+                                  experiment=ExperimentStatus.UNENROLLED,
+                                  decision='disabledByDefault')
+
+        # If the browser is started with a customized `fisison.autostart`,
+        # while also enrolled in an experiment, the experiment should be
+        # disqualified at startup.
+        with self.full_restart() as profile:
+            profile.set_preferences({
+                Prefs.FISSION_AUTOSTART: True,
+                Prefs.STARTUP_ENROLLMENT_STATUS: ExperimentStatus.ENROLLED_TREATMENT,
+            }, filename='prefs.js')
+
+        self.assertEqual(self.marionette.get_pref(Prefs.ENROLLMENT_STATUS),
+                         ExperimentStatus.DISQUALIFIED,
+                         'Should disqualify dynamic pref on startup')
+        self.assertEqual(self.marionette.get_pref(Prefs.STARTUP_ENROLLMENT_STATUS),
+                         ExperimentStatus.DISQUALIFIED,
+                         'Should disqualify startup pref on startup')
+
+        self.check_fission_status(enabled=True,
+                                  experiment=ExperimentStatus.DISQUALIFIED,
+                                  decision='enabledByUserPref')