From ee34cc38db53d5c25e19d4f4dfebfb787eb6cd97 Mon Sep 17 00:00:00 2001 From: Tomislav Jovanovic Date: Sun, 19 May 2024 18:51:06 +0000 Subject: [PATCH] Bug 1874406 - Refactor, optimize and cleanup event page idle management r=robwu Differential Revision: https://phabricator.services.mozilla.com/D210220 --- .../extensions/ExtensionParent.sys.mjs | 2 +- .../extensions/parent/ext-backgroundPage.js | 168 +++++++++--------- .../test_ext_dnr_static_rules_limits.js | 2 +- .../test/xpcshell/test_ext_eventpage_idle.js | 6 +- ...t_ext_webRequest_eventPage_StreamFilter.js | 4 +- 5 files changed, 88 insertions(+), 94 deletions(-) diff --git a/toolkit/components/extensions/ExtensionParent.sys.mjs b/toolkit/components/extensions/ExtensionParent.sys.mjs index bbc82092b00b..000cd22c78dc 100644 --- a/toolkit/components/extensions/ExtensionParent.sys.mjs +++ b/toolkit/components/extensions/ExtensionParent.sys.mjs @@ -1206,7 +1206,7 @@ ParentAPIManager = { !context.extension.persistentBackground ) { context.extension.emit("background-script-reset-idle", { - reason: "parentApiCall", + reason: "parentapicall", path: data.path, }); } diff --git a/toolkit/components/extensions/parent/ext-backgroundPage.js b/toolkit/components/extensions/parent/ext-backgroundPage.js index c10912aa3c36..43163bc494e8 100644 --- a/toolkit/components/extensions/parent/ext-backgroundPage.js +++ b/toolkit/components/extensions/parent/ext-backgroundPage.js @@ -497,8 +497,8 @@ class BackgroundContextOwner { EventManager.clearPrimedListeners(this.extension, false); } - // Ensure there is no backgroundTimer running - this.backgroundBuilder.clearIdleTimer(); + // Ensure any idle background timer is not running. + this.backgroundBuilder.idleManager.clearTimer(); const bgInstance = this.bgInstance; if (bgInstance) { @@ -664,6 +664,7 @@ class BackgroundBuilder { constructor(extension) { this.extension = extension; this.backgroundContextOwner = new BackgroundContextOwner(this, extension); + this.idleManager = new IdleManager(extension); } async build() { @@ -718,26 +719,6 @@ class BackgroundBuilder { } } - observe(subject, topic) { - if (topic == "timer-callback") { - let { extension } = this; - this.clearIdleTimer(); - extension?.terminateBackground(); - } - } - - clearIdleTimer() { - this.backgroundTimer?.cancel(); - this.backgroundTimer = null; - } - - resetIdleTimer() { - this.clearIdleTimer(); - let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); - timer.init(this, backgroundIdleTimeout, Ci.nsITimer.TYPE_ONE_SHOT); - this.backgroundTimer = timer; - } - primeBackground(isInStartup = true) { let { extension } = this; @@ -779,79 +760,45 @@ class BackgroundBuilder { return bgStartupPromise; }; - let resetBackgroundIdle = (eventName, resetIdleDetails) => { - this.clearIdleTimer(); - if (!this.extension || extension.persistentBackground) { - // Extension was already shut down or is persistent and - // does not idle timout. - return; - } - // TODO remove at an appropriate point in the future prior - // to general availability. There may be some racy conditions - // with idle timeout between an event starting and the event firing - // but we still want testing with an idle timeout. - if ( - !Services.prefs.getBoolPref("extensions.background.idle.enabled", true) - ) { - return; - } - + let resetBackgroundIdle = (event, { reason }) => { if ( extension.backgroundState == BACKGROUND_STATE.SUSPENDING && // After we begin suspending the background, parent API calls from // runtime.onSuspend listeners shouldn't cancel the suspension. - resetIdleDetails?.reason !== "parentApiCall" + reason !== "parentapicall" ) { extension.backgroundState = BACKGROUND_STATE.RUNNING; - // call runtime.onSuspendCanceled extension.emit("background-script-suspend-canceled"); } - this.resetIdleTimer(); + this.idleManager.resetTimer(); - if ( - eventName === "background-script-reset-idle" && - // TODO(Bug 1790087): record similar telemetry for background service worker. - !this.isWorker - ) { - // Record the reason for resetting the event page idle timeout - // in a idle result histogram, with the category set based - // on the reason for resetting (defaults to 'reset_other' - // if resetIdleDetails.reason is missing or not mapped into the - // telemetry histogram categories). - // - // Keep this in sync with the categories listed in Histograms.json - // for "WEBEXT_EVENTPAGE_IDLE_RESULT_COUNT". - let category = "reset_other"; - switch (resetIdleDetails?.reason) { - case "event": - category = "reset_event"; - return; // not break; because too frequent, see bug 1868960. - case "hasActiveNativeAppPorts": - category = "reset_nativeapp"; - break; - case "hasActiveStreamFilter": - category = "reset_streamfilter"; - break; - case "pendingListeners": - category = "reset_listeners"; - break; - case "parentApiCall": - category = "reset_parentapicall"; - return; // not break; because too frequent, see bug 1868960. - } - - ExtensionTelemetry.eventPageIdleResult.histogramAdd({ - extension, - category, - }); + if (this.isWorker) { + // TODO(Bug 1790087): record similar telemetry for service workers. + return; } + if (reason === "event" || reason === "parentapicall") { + // Bug 1868960: not recording these because too frequent. + return; + } + + // Keep in sync with categories in WEBEXT_EVENTPAGE_IDLE_RESULT_COUNT. + let KNOWN = ["nativeapp", "streamfilter", "listeners"]; + ExtensionTelemetry.eventPageIdleResult.histogramAdd({ + extension, + category: `reset_${KNOWN.includes(reason) ? reason : "other"}`, + }); }; - // Listen for events from the EventManager - extension.on("background-script-reset-idle", resetBackgroundIdle); - // After the background is started, initiate the first timer - extension.once("background-script-started", resetBackgroundIdle); + if (!extension.persistentBackground) { + // Listen for events from the EventManager + extension.on("background-script-reset-idle", resetBackgroundIdle); + + // After the background is started, initiate the first timer + extension.once("background-script-started", () => { + this.idleManager.resetTimer(); + }); + } // TODO bug 1844488: terminateBackground should account for externally // triggered background restarts. It does currently performs various @@ -889,9 +836,7 @@ class BackgroundBuilder { !disableResetIdleForTest && extension.backgroundContext?.hasActiveNativeAppPorts ) { - extension.emit("background-script-reset-idle", { - reason: "hasActiveNativeAppPorts", - }); + extension.emit("background-script-reset-idle", { reason: "nativeapp" }); return; } @@ -900,7 +845,7 @@ class BackgroundBuilder { extension.backgroundContext?.pendingRunListenerPromisesCount ) { extension.emit("background-script-reset-idle", { - reason: "pendingListeners", + reason: "listeners", pendingListeners: extension.backgroundContext.pendingRunListenerPromisesCount, }); @@ -941,7 +886,7 @@ class BackgroundBuilder { }); if (!disableResetIdleForTest && hasActiveStreamFilter) { extension.emit("background-script-reset-idle", { - reason: "hasActiveStreamFilter", + reason: "streamfilter", }); return; } @@ -956,7 +901,7 @@ class BackgroundBuilder { } extension.backgroundState = BACKGROUND_STATE.SUSPENDING; - this.clearIdleTimer(); + this.idleManager.clearTimer(); // call runtime.onSuspend await extension.emit("background-script-suspend"); // If in the meantime another event fired, state will be RUNNING, @@ -1027,6 +972,53 @@ class BackgroundBuilder { } } +/** + * Times the suspension of the background page, acts like a 3-state machine: + * - suspended (or uninitialized) + * - waiting for a timeout (now() < sleepTime) + * - TODO: waiting on a promise (keepAwake.size > 0) + */ +var IdleManager = class IdleManager { + sleepTime = 0; + /** @type {nsITimer} */ + timer = null; + /** @type {Map} */ + keepAlive = new Map(); + + constructor(extension) { + this.extension = extension; + } + + clearTimer() { + this.timer?.cancel(); + this.timer = null; + } + + resetTimer() { + this.sleepTime = Cu.now() + backgroundIdleTimeout; + if (!this.timer) { + this.createTimer(); + } + } + + createTimer() { + let timeLeft = this.sleepTime - Cu.now(); + this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); + this.timer.init(() => this.timeout(), timeLeft, Ci.nsITimer.TYPE_ONE_SHOT); + } + + timeout() { + this.clearTimer(); + if (!this.keepAlive.size) { + if (Cu.now() < this.sleepTime) { + this.createTimer(); + } else { + this.extension.terminateBackground(); + } + } + } +}; + this.backgroundPage = class extends ExtensionAPI { async onManifestEntry() { let { extension } = this; diff --git a/toolkit/components/extensions/test/xpcshell/test_ext_dnr_static_rules_limits.js b/toolkit/components/extensions/test/xpcshell/test_ext_dnr_static_rules_limits.js index c2505f175519..00341d751344 100644 --- a/toolkit/components/extensions/test/xpcshell/test_ext_dnr_static_rules_limits.js +++ b/toolkit/components/extensions/test/xpcshell/test_ext_dnr_static_rules_limits.js @@ -36,7 +36,7 @@ add_setup(async () => { // e.g. see Bug 1803801) have a higher chance that the test extension may have hit the // idle timeout and being suspended by the time the test is going to trigger API method // calls through test API events (which do not expect the lifetime of the event page). - Services.prefs.setBoolPref("extensions.background.idle.enabled", false); + Services.prefs.setIntPref("extensions.background.idle.timeout", 300_000); // NOTE: reduce the static rules limits to reduce the amount of time needed to run // this xpcshell test. diff --git a/toolkit/components/extensions/test/xpcshell/test_ext_eventpage_idle.js b/toolkit/components/extensions/test/xpcshell/test_ext_eventpage_idle.js index 46d34f1bcb85..716abf7d6a07 100644 --- a/toolkit/components/extensions/test/xpcshell/test_ext_eventpage_idle.js +++ b/toolkit/components/extensions/test/xpcshell/test_ext_eventpage_idle.js @@ -208,7 +208,7 @@ add_task( "background-script-reset-idle" ); - equal(resetData.reason, "parentApiCall", "Got the expected idle reset."); + equal(resetData.reason, "parentapicall", "Got the expected idle reset."); await promiseExtensionEvent(extension, "shutdown-background-script"); @@ -640,7 +640,7 @@ add_task( Assert.deepEqual( resetIdleData, { - reason: "pendingListeners", + reason: "listeners", pendingListeners: 2, }, "Got the expected idle reset reason and pendingListeners count" @@ -754,7 +754,7 @@ add_task( Assert.deepEqual( resetIdleData, { - reason: "pendingListeners", + reason: "listeners", pendingListeners: 1, }, "Got the expected idle reset reason and pendingListeners count" diff --git a/toolkit/components/extensions/test/xpcshell/test_ext_webRequest_eventPage_StreamFilter.js b/toolkit/components/extensions/test/xpcshell/test_ext_webRequest_eventPage_StreamFilter.js index 820e04956e71..6a0e5de4f947 100644 --- a/toolkit/components/extensions/test/xpcshell/test_ext_webRequest_eventPage_StreamFilter.js +++ b/toolkit/components/extensions/test/xpcshell/test_ext_webRequest_eventPage_StreamFilter.js @@ -325,7 +325,9 @@ async function test_create_new_streamfilter_while_suspending({ await extension.awaitMessage("suspend-listener"); info("Simulated idle timeout canceled"); - extension.extension.emit("background-script-reset-idle"); + extension.extension.emit("background-script-reset-idle", { + reason: "other/simulate-idle-reset", + }); await extension.awaitMessage("suspend-canceled-listener"); await extension.unload();