Bug 1789727 - Default to the component implementation of Screenshots for nightly builds. r=extension-reviewers,sfoster,robwu,chutten

* Flip the component pref to true by default for nightly builds only
* Move the pref check and initialization to a startup idle task
* And be a bit smarter about when we get and disable the addon
* Fix a bug where we try to communicate with the overlay after the window actor is destroyed when
  the component pref gets flipped off during use

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

Author: sfoster

browser/app/profile/firefox.js

@@ -2404,8 +2404,12 @@ pref("browser.suppress_first_window_animation", true);
 // Preference that allows individual users to disable Screenshots.
 pref("extensions.screenshots.disabled", false);
 
-// Preference that determines whether Screenshots is opened as a dedicated browser component
-pref("screenshots.browser.component.enabled", false);
+// Preference that determines whether Screenshots uses the dedicated browser component
+#ifdef NIGHTLY_BUILD
+  pref("screenshots.browser.component.enabled", true);
+#else
+  pref("screenshots.browser.component.enabled", false);
+#endif
 
 // Preference that determines what button to focus
 pref("screenshots.browser.component.last-saved-method", "download");

browser/base/content/test/performance/browser_startup_content_mainthreadio.js

@@ -152,6 +152,13 @@ const processes = {
       condition: WIN,
       stat: 1,
     },
+    {
+      // We should remove this in bug 1882427
+      path: "*screenshots@mozilla.org.xpi",
+      condition: true,
+      ignoreIfUnused: true,
+      close: 1,
+    },
   ],
 };
 

browser/components/BrowserGlue.sys.mjs

@@ -2144,26 +2144,31 @@ BrowserGlue.prototype = {
     const COMPONENT_PREF = "screenshots.browser.component.enabled";
     const ID = "screenshots@mozilla.org";
     const _checkScreenshotsPref = async () => {
-      let addon = await lazy.AddonManager.getAddonByID(ID);
-      if (!addon) {
-        return;
-      }
       let screenshotsDisabled = Services.prefs.getBoolPref(
         SCREENSHOTS_PREF,
         false
       );
-      let componentEnabled = Services.prefs.getBoolPref(COMPONENT_PREF, false);
+      let componentEnabled = Services.prefs.getBoolPref(COMPONENT_PREF, true);
+
+      let screenshotsAddon = await lazy.AddonManager.getAddonByID(ID);
+
       if (screenshotsDisabled) {
         if (componentEnabled) {
           lazy.ScreenshotsUtils.uninitialize();
-        } else {
-          await addon.disable({ allowSystemAddons: true });
+        } else if (screenshotsAddon?.isActive) {
+          await screenshotsAddon.disable({ allowSystemAddons: true });
         }
       } else if (componentEnabled) {
         lazy.ScreenshotsUtils.initialize();
-        await addon.disable({ allowSystemAddons: true });
+        if (screenshotsAddon?.isActive) {
+          await screenshotsAddon.disable({ allowSystemAddons: true });
+        }
       } else {
-        await addon.enable({ allowSystemAddons: true });
+        try {
+          await screenshotsAddon.enable({ allowSystemAddons: true });
+        } catch (ex) {
+          console.error(`Error trying to enable screenshots extension: ${ex}`);
+        }
         lazy.ScreenshotsUtils.uninitialize();
       }
     };
@@ -2434,7 +2439,6 @@ BrowserGlue.prototype = {
       LATE_TASKS_IDLE_TIME_SEC
     );
 
-    this._monitorScreenshotsPref();
     this._monitorWebcompatReporterPref();
     this._monitorHTTPSOnlyPref();
     this._monitorIonPref();
@@ -2787,13 +2791,9 @@ BrowserGlue.prototype = {
       },
 
       {
-        name: "ScreenshotsUtils.initialize",
+        name: "BrowserGlue._monitorScreenshotsPref",
         task: () => {
-          if (
-            Services.prefs.getBoolPref("screenshots.browser.component.enabled")
-          ) {
-            lazy.ScreenshotsUtils.initialize();
-          }
+          this._monitorScreenshotsPref();
         },
       },
 

browser/components/screenshots/ScreenshotsUtils.sys.mjs

@@ -544,7 +544,12 @@ export var ScreenshotsUtils = {
    * @param browser The current browser.
    */
   closeOverlay(browser, options = {}) {
-    let actor = this.getActor(browser);
+    // If the actor has been unregistered (e.g. if the component enabled pref is flipped false)
+    // its possible getActor will throw an exception. That's ok.
+    let actor;
+    try {
+      actor = this.getActor(browser);
+    } catch (ex) {}
     actor?.sendAsyncMessage("Screenshots:HideOverlay", options);
 
     if (this.browserToScreenshotsState.has(browser)) {

browser/components/screenshots/tests/browser/browser_screenshots_test_toggle_pref.js

@@ -9,6 +9,7 @@ const { sinon } = ChromeUtils.importESModule(
 
 ChromeUtils.defineESModuleGetters(this, {
   ScreenshotsUtils: "resource:///modules/ScreenshotsUtils.sys.mjs",
+  AddonManager: "resource://gre/modules/AddonManager.sys.mjs",
 });
 ChromeUtils.defineLazyGetter(this, "ExtensionManagement", () => {
   const { Management } = ChromeUtils.importESModule(
@@ -17,7 +18,11 @@ ChromeUtils.defineLazyGetter(this, "ExtensionManagement", () => {
   return Management;
 });
 
-add_task(async function test() {
+const COMPONENT_PREF = "screenshots.browser.component.enabled";
+const SCREENSHOTS_PREF = "extensions.screenshots.disabled";
+const SCREENSHOT_EXTENSION = "screenshots@mozilla.org";
+
+add_task(async function test_toggling_screenshots_pref() {
   let observerSpy = sinon.spy();
   let notifierSpy = sinon.spy();
 
@@ -31,13 +36,24 @@ add_task(async function test() {
       ScreenshotsUtils.notify.wrappedMethod.apply(this, arguments);
     });
 
+  // wait for startup idle tasks to complete
+  await new Promise(resolve => ChromeUtils.idleDispatch(resolve));
+  ok(Services.prefs.getBoolPref(COMPONENT_PREF), "Component enabled");
+  ok(!Services.prefs.getBoolPref(SCREENSHOTS_PREF), "Screenshots enabled");
+
+  let addon = await AddonManager.getAddonByID(SCREENSHOT_EXTENSION);
+  await BrowserTestUtils.waitForCondition(
+    () => !addon.isActive,
+    "The extension is not active when the component is prefd on"
+  );
+
   await BrowserTestUtils.withNewTab(
     {
       gBrowser,
       url: SHORT_TEST_PAGE,
     },
     async browser => {
-      function awaitExtensionEvent(eventName, id) {
+      function extensionEventPromise(eventName, id) {
         return new Promise(resolve => {
           let listener = (_eventName, ...args) => {
             let extension = args[0];
@@ -49,9 +65,21 @@ add_task(async function test() {
           ExtensionManagement.on(eventName, listener);
         });
       }
-      const SCREENSHOT_EXTENSION = "screenshots@mozilla.org";
 
       let helper = new ScreenshotsHelper(browser);
+      ok(
+        addon.userDisabled,
+        "The extension is disabled when the component is prefd on"
+      );
+      ok(
+        !addon.isActive,
+        "The extension is not initially active when the component is prefd on"
+      );
+      await BrowserTestUtils.waitForCondition(
+        () => ScreenshotsUtils.initialized,
+        "The component is initialized"
+      );
+      ok(ScreenshotsUtils.initialized, "The component is initialized");
 
       ok(observerSpy.notCalled, "Observer not called");
       helper.triggerUIFromToolbar();
@@ -80,12 +108,20 @@ add_task(async function test() {
 
       Assert.equal(observerSpy.callCount, 3, "Observer function called thrice");
 
-      const COMPONENT_PREF = "screenshots.browser.component.enabled";
-      await SpecialPowers.pushPrefEnv({
-        set: [[COMPONENT_PREF, false]],
-      });
+      let extensionReadyPromise = extensionEventPromise(
+        "ready",
+        SCREENSHOT_EXTENSION
+      );
+      Services.prefs.setBoolPref(COMPONENT_PREF, false);
       ok(!Services.prefs.getBoolPref(COMPONENT_PREF), "Extension enabled");
-      await awaitExtensionEvent("ready", SCREENSHOT_EXTENSION);
+
+      info("Waiting for the extension ready event");
+      await extensionReadyPromise;
+      await BrowserTestUtils.waitForCondition(
+        () => !addon.userDisabled,
+        "The extension gets un-disabled when the component is prefd off"
+      );
+      ok(addon.isActive, "Extension is active");
 
       helper.triggerUIFromToolbar();
       Assert.equal(
@@ -94,6 +130,7 @@ add_task(async function test() {
         "Observer function still called thrice"
       );
 
+      info("Waiting for the extensions overlay");
       await SpecialPowers.spawn(
         browser,
         ["#firefox-screenshots-preselection-iframe"],
@@ -115,6 +152,7 @@ add_task(async function test() {
         }
       );
 
+      info("Waiting for the extensions overlay");
       helper.triggerUIFromToolbar();
       await SpecialPowers.spawn(
         browser,
@@ -202,9 +240,7 @@ add_task(async function test() {
         "screenshots-component-initialized"
       );
 
-      await SpecialPowers.pushPrefEnv({
-        set: [[COMPONENT_PREF, true]],
-      });
+      Services.prefs.setBoolPref(COMPONENT_PREF, true);
       ok(Services.prefs.getBoolPref(COMPONENT_PREF), "Component enabled");
       // Needed for component to initialize
       await componentReady;
@@ -215,12 +251,6 @@ add_task(async function test() {
         4,
         "Observer function called four times"
       );
-
-      const SCREENSHOTS_PREF = "extensions.screenshots.disabled";
-      await SpecialPowers.pushPrefEnv({
-        set: [[SCREENSHOTS_PREF, true]],
-      });
-      ok(Services.prefs.getBoolPref(SCREENSHOTS_PREF), "Screenshots disabled");
     }
   );
 
@@ -230,7 +260,9 @@ add_task(async function test() {
       url: SHORT_TEST_PAGE,
     },
     async browser => {
-      const SCREENSHOTS_PREF = "extensions.screenshots.disabled";
+      Services.prefs.setBoolPref(SCREENSHOTS_PREF, true);
+      Services.prefs.setBoolPref(COMPONENT_PREF, true);
+
       ok(Services.prefs.getBoolPref(SCREENSHOTS_PREF), "Screenshots disabled");
 
       ok(
@@ -255,22 +287,23 @@ add_task(async function test() {
       menu.hidePopup();
       await popuphidden;
 
-      await SpecialPowers.pushPrefEnv({
-        set: [[SCREENSHOTS_PREF, false]],
-      });
-      ok(!Services.prefs.getBoolPref(SCREENSHOTS_PREF), "Screenshots enabled");
-    }
-  );
+      let componentReady = TestUtils.topicObserved(
+        "screenshots-component-initialized"
+      );
+
+      Services.prefs.setBoolPref(SCREENSHOTS_PREF, false);
 
-  await BrowserTestUtils.withNewTab(
-    {
-      gBrowser,
-      url: SHORT_TEST_PAGE,
-    },
-    async browser => {
-      const SCREENSHOTS_PREF = "extensions.screenshots.disabled";
       ok(!Services.prefs.getBoolPref(SCREENSHOTS_PREF), "Screenshots enabled");
 
+      await componentReady;
+
+      ok(ScreenshotsUtils.initialized, "The component is initialized");
+
+      ok(
+        !document.getElementById("screenshot-button").disabled,
+        "Toolbar button is enabled"
+      );
+
       let helper = new ScreenshotsHelper(browser);
 
       helper.triggerUIFromToolbar();
@@ -284,6 +317,4 @@ add_task(async function test() {
 
   observerStub.restore();
   notifierStub.restore();
-
-  await SpecialPowers.popPrefEnv();
 });

browser/extensions/screenshots/test/browser/browser.toml

@@ -1,5 +1,9 @@
 [DEFAULT]
-prefs = ["extensions.screenshots.disabled=false",] # The Screenshots extension is disabled by default in Mochitests. We re-enable it here, since it's a more realistic configuration.
+# The Screenshots extension is disabled by default in Mochitests. We re-enable it here, since it's a more realistic configuration.
+prefs = [
+ "extensions.screenshots.disabled=false",
+ "screenshots.browser.component.enabled=false",
+]
 
 ["browser_screenshot_button.js"]
 

toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/runner.py

@@ -52,6 +52,9 @@ def __init__(self, **kwargs):
                 # Disable Normandy a little harder (bug 1608807).
                 # This should also disable Nimbus.
                 "app.shield.optoutstudies.enabled": False,
+                # Bug 1789727: Keep the screenshots extension disabled to avoid
+                # disabling the addon resulting in extra subsessions
+                "screenshots.browser.component.enabled": False,
             }
         )