From 9911ad6052bcb0dd979be0b65bbedb1322fc2ab9 Mon Sep 17 00:00:00 2001 From: Sam Foster Date: Fri, 1 Mar 2024 15:02:51 +0000 Subject: [PATCH] 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 --- browser/app/profile/firefox.js | 8 +- .../browser_startup_content_mainthreadio.js | 7 ++ browser/components/BrowserGlue.sys.mjs | 32 +++--- .../screenshots/ScreenshotsUtils.sys.mjs | 7 +- .../browser_screenshots_test_toggle_pref.js | 97 ++++++++++++------- .../screenshots/test/browser/browser.toml | 6 +- .../harness/telemetry_harness/runner.py | 3 + 7 files changed, 107 insertions(+), 53 deletions(-) diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js index c1224e80205e..f41b76fb82df 100644 --- a/browser/app/profile/firefox.js +++ b/browser/app/profile/firefox.js @@ -2406,8 +2406,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"); diff --git a/browser/base/content/test/performance/browser_startup_content_mainthreadio.js b/browser/base/content/test/performance/browser_startup_content_mainthreadio.js index e60b95bb3cab..e26736306466 100644 --- a/browser/base/content/test/performance/browser_startup_content_mainthreadio.js +++ b/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, + }, ], }; diff --git a/browser/components/BrowserGlue.sys.mjs b/browser/components/BrowserGlue.sys.mjs index bced81a71fd2..55d535e74f60 100644 --- a/browser/components/BrowserGlue.sys.mjs +++ b/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(); }, }, diff --git a/browser/components/screenshots/ScreenshotsUtils.sys.mjs b/browser/components/screenshots/ScreenshotsUtils.sys.mjs index 68e4f896bfff..89880a630e9f 100644 --- a/browser/components/screenshots/ScreenshotsUtils.sys.mjs +++ b/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)) { diff --git a/browser/components/screenshots/tests/browser/browser_screenshots_test_toggle_pref.js b/browser/components/screenshots/tests/browser/browser_screenshots_test_toggle_pref.js index 0aafba8fb3a8..ad262a7e6754 100644 --- a/browser/components/screenshots/tests/browser/browser_screenshots_test_toggle_pref.js +++ b/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(); }); diff --git a/browser/extensions/screenshots/test/browser/browser.toml b/browser/extensions/screenshots/test/browser/browser.toml index 240471cc7d65..9721a44689ba 100644 --- a/browser/extensions/screenshots/test/browser/browser.toml +++ b/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"] diff --git a/toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/runner.py b/toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/runner.py index 37a91023cef0..29b03e4f55fe 100644 --- a/toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/runner.py +++ b/toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/runner.py @@ -52,6 +52,9 @@ class TelemetryTestRunner(BaseMarionetteTestRunner): # 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, } )