From 407966f9e8474e2d0936e05f7078e1532ceae94b Mon Sep 17 00:00:00 2001 From: Blair McBride Date: Mon, 19 Jan 2015 22:55:15 +1300 Subject: [PATCH] Bug 1075625 - AddonManager can race between provider startup / shutdown and methods that call back to providers. r=Mossop --- toolkit/mozapps/extensions/AddonManager.jsm | 111 +++++++++++++++--- .../extensions/internal/XPIProvider.jsm | 2 + .../extensions/test/xpcshell/head_addons.js | 10 ++ .../extensions/test/xpcshell/test_isReady.js | 49 ++++++++ .../test/xpcshell/test_provider_markSafe.js | 47 ++++++++ .../test_provider_unsafe_access_shutdown.js | 61 ++++++++++ .../test_provider_unsafe_access_startup.js | 53 +++++++++ .../extensions/test/xpcshell/xpcshell.ini | 4 + 8 files changed, 320 insertions(+), 17 deletions(-) create mode 100644 toolkit/mozapps/extensions/test/xpcshell/test_isReady.js create mode 100644 toolkit/mozapps/extensions/test/xpcshell/test_provider_markSafe.js create mode 100644 toolkit/mozapps/extensions/test/xpcshell/test_provider_unsafe_access_shutdown.js create mode 100644 toolkit/mozapps/extensions/test/xpcshell/test_provider_unsafe_access_startup.js diff --git a/toolkit/mozapps/extensions/AddonManager.jsm b/toolkit/mozapps/extensions/AddonManager.jsm index 8d9dcf724ea..99f79005866 100644 --- a/toolkit/mozapps/extensions/AddonManager.jsm +++ b/toolkit/mozapps/extensions/AddonManager.jsm @@ -286,7 +286,7 @@ function getLocale() { * just the AsyncObjectCaller */ function AsyncObjectCaller(aObjects, aMethod, aListener) { - this.objects = aObjects.slice(0); + this.objects = [...aObjects]; this.method = aMethod; this.listener = aListener; @@ -518,7 +518,8 @@ var AddonManagerInternal = { installListeners: [], addonListeners: [], typeListeners: [], - providers: [], + pendingProviders: new Set(), + providers: new Set(), providerShutdowns: new Map(), types: {}, startupChanges: {}, @@ -666,10 +667,20 @@ var AddonManagerInternal = { throw Components.Exception("AddonManager is not initialized", Cr.NS_ERROR_NOT_INITIALIZED); + logger.debug(`Starting provider: ${providerName(aProvider)}`); callProvider(aProvider, "startup", null, aAppChanged, aOldAppVersion, aOldPlatformVersion); if ('shutdown' in aProvider) { let name = providerName(aProvider); let AMProviderShutdown = () => { + // If the provider has been unregistered, it will have been removed from + // this.providers. If it hasn't been unregistered, then this is a normal + // shutdown - and we move it to this.pendingProviders incase we're + // running in a test that will start AddonManager again. + if (this.providers.has(aProvider)) { + this.providers.delete(aProvider); + this.pendingProviders.add(aProvider); + } + return new Promise((resolve, reject) => { logger.debug("Calling shutdown blocker for " + name); resolve(aProvider.shutdown()); @@ -683,6 +694,10 @@ var AddonManagerInternal = { this.providerShutdowns.set(aProvider, AMProviderShutdown); AddonManager.shutdown.addBlocker(name, AMProviderShutdown); } + + this.pendingProviders.delete(aProvider); + this.providers.add(aProvider); + logger.debug(`Provider finished startup: ${providerName(aProvider)}`); }, /** @@ -805,6 +820,7 @@ var AddonManagerInternal = { try { Components.utils.import(url, {}); + logger.debug(`Loaded provider scope for ${url}`); } catch (e) { AddonManagerPrivate.recordException("AMI", "provider " + url + " load failed", e); @@ -822,7 +838,7 @@ var AddonManagerInternal = { // Once we start calling providers we must allow all normal methods to work. gStarted = true; - for (let provider of this.providers) { + for (let provider of this.pendingProviders) { this._startProvider(provider, appChanged, oldAppVersion, oldPlatformVersion); } @@ -839,6 +855,9 @@ var AddonManagerInternal = { logger.error("startup failed", e); AddonManagerPrivate.recordException("AMI", "startup failed", e); } + + logger.debug("Completed startup sequence"); + this.callManagerListeners("onStartup"); }, /** @@ -858,7 +877,7 @@ var AddonManagerInternal = { throw Components.Exception("aTypes must be an array or null", Cr.NS_ERROR_INVALID_ARG); - this.providers.push(aProvider); + this.pendingProviders.add(aProvider); if (aTypes) { aTypes.forEach(function(aType) { @@ -906,13 +925,11 @@ var AddonManagerInternal = { throw Components.Exception("aProvider must be specified", Cr.NS_ERROR_INVALID_ARG); - let pos = 0; - while (pos < this.providers.length) { - if (this.providers[pos] == aProvider) - this.providers.splice(pos, 1); - else - pos++; - } + this.providers.delete(aProvider); + // The test harness will unregister XPIProvider *after* shutdown, which is + // after the provider will have been moved from providers to + // pendingProviders. + this.pendingProviders.delete(aProvider); for (let type in this.types) { this.types[type].providers = this.types[type].providers.filter(function filterProvider(p) p != aProvider); @@ -944,6 +961,39 @@ var AddonManagerInternal = { return undefined; }, + /** + * Mark a provider as safe to access via AddonManager APIs, before its + * startup has completed. + * + * Normally a provider isn't marked as safe until after its (synchronous) + * startup() method has returned. Until a provider has been marked safe, + * it won't be used by any of the AddonManager APIs. markProviderSafe() + * allows a provider to mark itself as safe during its startup; this can be + * useful if the provider wants to perform tasks that block startup, which + * happen after its required initialization tasks and therefore when the + * provider is in a safe state. + * + * @param aProvider Provider object to mark safe + */ + markProviderSafe: function AMI_markProviderSafe(aProvider) { + if (!gStarted) { + throw Components.Exception("AddonManager is not initialized", + Cr.NS_ERROR_NOT_INITIALIZED); + } + + if (!aProvider || typeof aProvider != "object") { + throw Components.Exception("aProvider must be specified", + Cr.NS_ERROR_INVALID_ARG); + } + + if (!this.pendingProviders.has(aProvider)) { + return; + } + + this.pendingProviders.delete(aProvider); + this.providers.add(aProvider); + }, + /** * Calls a method on all registered providers if it exists and consumes any * thrown exception. Return values are ignored. Any parameters after the @@ -960,7 +1010,7 @@ var AddonManagerInternal = { throw Components.Exception("aMethod must be a non-empty string", Cr.NS_ERROR_INVALID_ARG); - let providers = this.providers.slice(0); + let providers = [...this.providers]; for (let provider of providers) { try { if (aMethod in provider) @@ -998,6 +1048,8 @@ var AddonManagerInternal = { */ shutdownManager: Task.async(function* () { logger.debug("shutdown"); + this.callManagerListeners("onShutdown"); + gRepoShutdownState = "pending"; gShutdownInProgress = true; // Clean up listeners @@ -1560,7 +1612,23 @@ var AddonManagerInternal = { throw Components.Exception("aType must be a non-empty string", Cr.NS_ERROR_INVALID_ARG); - this.callProviders("addonChanged", aID, aType, aPendingRestart); + // Temporary hack until bug 520124 lands. + // We can get here during synchronous startup, at which point it's + // considered unsafe (and therefore disallowed by AddonManager.jsm) to + // access providers that haven't been initialized yet. Since this is when + // XPIProvider is starting up, XPIProvider can't access itself via APIs + // going through AddonManager.jsm. Furthermore, LightweightThemeManager may + // not be initialized until after XPIProvider is, and therefore would also + // be unaccessible during XPIProvider startup. Thankfully, these are the + // only two uses of this API, and we know it's safe to use this API with + // both providers; so we have this hack to allow bypassing the normal + // safetey guard. + // The notifyAddonChanged/addonChanged API will be unneeded and therefore + // removed by bug 520124, so this is a temporary quick'n'dirty hack. + let providers = [...this.providers, ...this.pendingProviders]; + for (let provider of providers) { + callProvider(provider, "addonChanged", null, aID, aType, aPendingRestart); + } }, /** @@ -1671,7 +1739,7 @@ var AddonManagerInternal = { throw Components.Exception("aBrowser must be a nsIDOMElement or null", Cr.NS_ERROR_INVALID_ARG); - let providers = this.providers.slice(0); + let providers = [...this.providers]; for (let provider of providers) { if (callProvider(provider, "supportsMimetype", false, aMimetype)) { callProviderAsync(provider, "getInstallForURL", @@ -1803,8 +1871,9 @@ var AddonManagerInternal = { throw Components.Exception("aURI is not a nsIURI", Cr.NS_ERROR_INVALID_ARG); } + // Try all providers - let providers = this.providers.slice(0); + let providers = [...this.providers]; for (let provider of providers) { var id = callProvider(provider, "mapURIToAddonID", null, aURI); if (id !== null) { @@ -1831,7 +1900,7 @@ var AddonManagerInternal = { throw Components.Exception("aMimetype must be a non-empty string", Cr.NS_ERROR_INVALID_ARG); - let providers = this.providers.slice(0); + let providers = [...this.providers]; for (let provider of providers) { if (callProvider(provider, "supportsMimetype", false, aMimetype) && callProvider(provider, "isInstallEnabled")) @@ -1863,7 +1932,7 @@ var AddonManagerInternal = { throw Components.Exception("aURI must be a nsIURI or null", Cr.NS_ERROR_INVALID_ARG); - let providers = this.providers.slice(0); + let providers = [...this.providers]; for (let provider of providers) { if (callProvider(provider, "supportsMimetype", false, aMimetype) && callProvider(provider, "isInstallAllowed", null, aURI)) @@ -2412,6 +2481,10 @@ this.AddonManagerPrivate = { AddonManagerInternal.unregisterProvider(aProvider); }, + markProviderSafe: function AMP_markProviderSafe(aProvider) { + AddonManagerInternal.markProviderSafe(aProvider); + }, + backgroundUpdateCheck: function AMP_backgroundUpdateCheck() { return AddonManagerInternal.backgroundUpdateCheck(); }, @@ -2718,6 +2791,10 @@ this.AddonManager = { }, #endif + get isReady() { + return gStartupComplete && !gShutdownInProgress; + }, + getInstallForURL: function AM_getInstallForURL(aUrl, aCallback, aMimetype, aHash, aName, aIcons, aVersion, aBrowser) { diff --git a/toolkit/mozapps/extensions/internal/XPIProvider.jsm b/toolkit/mozapps/extensions/internal/XPIProvider.jsm index 250fe6ebfff..186c2b1fc6a 100644 --- a/toolkit/mozapps/extensions/internal/XPIProvider.jsm +++ b/toolkit/mozapps/extensions/internal/XPIProvider.jsm @@ -2094,6 +2094,8 @@ this.XPIProvider = { // Changes to installed extensions may have changed which theme is selected this.applyThemeChange(); + AddonManagerPrivate.markProviderSafe(this); + if (aAppChanged === undefined) { // For new profiles we will never need to show the add-on selection UI Services.prefs.setBoolPref(PREF_SHOWN_SELECTION_UI, true); diff --git a/toolkit/mozapps/extensions/test/xpcshell/head_addons.js b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js index 42cb72accbe..729608f77d5 100644 --- a/toolkit/mozapps/extensions/test/xpcshell/head_addons.js +++ b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js @@ -911,6 +911,7 @@ function getExpectedInstall(aAddon) { const AddonListener = { onPropertyChanged: function(aAddon, aProperties) { + do_print(`Got onPropertyChanged event for ${aAddon.id}`); let [event, properties] = getExpectedEvent(aAddon.id); do_check_eq("onPropertyChanged", event); do_check_eq(aProperties.length, properties.length); @@ -924,6 +925,7 @@ const AddonListener = { }, onEnabling: function(aAddon, aRequiresRestart) { + do_print(`Got onEnabling event for ${aAddon.id}`); let [event, expectedRestart] = getExpectedEvent(aAddon.id); do_check_eq("onEnabling", event); do_check_eq(aRequiresRestart, expectedRestart); @@ -934,6 +936,7 @@ const AddonListener = { }, onEnabled: function(aAddon) { + do_print(`Got onEnabled event for ${aAddon.id}`); let [event, expectedRestart] = getExpectedEvent(aAddon.id); do_check_eq("onEnabled", event); do_check_false(hasFlag(aAddon.permissions, AddonManager.PERM_CAN_ENABLE)); @@ -941,6 +944,7 @@ const AddonListener = { }, onDisabling: function(aAddon, aRequiresRestart) { + do_print(`Got onDisabling event for ${aAddon.id}`); let [event, expectedRestart] = getExpectedEvent(aAddon.id); do_check_eq("onDisabling", event); do_check_eq(aRequiresRestart, expectedRestart); @@ -951,6 +955,7 @@ const AddonListener = { }, onDisabled: function(aAddon) { + do_print(`Got onDisabled event for ${aAddon.id}`); let [event, expectedRestart] = getExpectedEvent(aAddon.id); do_check_eq("onDisabled", event); do_check_false(hasFlag(aAddon.permissions, AddonManager.PERM_CAN_DISABLE)); @@ -958,6 +963,7 @@ const AddonListener = { }, onInstalling: function(aAddon, aRequiresRestart) { + do_print(`Got onInstalling event for ${aAddon.id}`); let [event, expectedRestart] = getExpectedEvent(aAddon.id); do_check_eq("onInstalling", event); do_check_eq(aRequiresRestart, expectedRestart); @@ -967,12 +973,14 @@ const AddonListener = { }, onInstalled: function(aAddon) { + do_print(`Got onInstalled event for ${aAddon.id}`); let [event, expectedRestart] = getExpectedEvent(aAddon.id); do_check_eq("onInstalled", event); return check_test_completed(arguments); }, onUninstalling: function(aAddon, aRequiresRestart) { + do_print(`Got onUninstalling event for ${aAddon.id}`); let [event, expectedRestart] = getExpectedEvent(aAddon.id); do_check_eq("onUninstalling", event); do_check_eq(aRequiresRestart, expectedRestart); @@ -982,12 +990,14 @@ const AddonListener = { }, onUninstalled: function(aAddon) { + do_print(`Got onUninstalled event for ${aAddon.id}`); let [event, expectedRestart] = getExpectedEvent(aAddon.id); do_check_eq("onUninstalled", event); return check_test_completed(arguments); }, onOperationCancelled: function(aAddon) { + do_print(`Got onOperationCancelled event for ${aAddon.id}`); let [event, expectedRestart] = getExpectedEvent(aAddon.id); do_check_eq("onOperationCancelled", event); return check_test_completed(arguments); diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_isReady.js b/toolkit/mozapps/extensions/test/xpcshell/test_isReady.js new file mode 100644 index 00000000000..6222398a75e --- /dev/null +++ b/toolkit/mozapps/extensions/test/xpcshell/test_isReady.js @@ -0,0 +1,49 @@ +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2"); + +function run_test() { + run_next_test(); +} + +add_task(function* () { + equal(AddonManager.isReady, false, "isReady should be false before startup"); + + let gotStartupEvent = false; + let gotShutdownEvent = false; + let listener = { + onStartup() { + gotStartupEvent = true; + }, + onShutdown() { + gotShutdownEvent = true; + }, + }; + AddonManager.addManagerListener(listener); + + do_print("Starting manager..."); + startupManager(); + equal(AddonManager.isReady, true, "isReady should be true after startup"); + equal(gotStartupEvent, true, "Should have seen onStartup event after startup"); + equal(gotShutdownEvent, false, "Should not have seen onShutdown event before shutdown"); + + gotStartupEvent = false; + gotShutdownEvent = false; + + do_print("Shutting down manager..."); + let shutdownPromise = promiseShutdownManager(); + equal(AddonManager.isReady, false, "isReady should be false when shutdown commences"); + yield shutdownPromise; + + equal(AddonManager.isReady, false, "isReady should be false after shutdown"); + equal(gotStartupEvent, false, "Should not have seen onStartup event after shutdown"); + equal(gotShutdownEvent, true, "Should have seen onShutdown event after shutdown"); + + AddonManager.addManagerListener(listener); + gotStartupEvent = false; + gotShutdownEvent = false; + + do_print("Starting manager again..."); + startupManager(); + equal(AddonManager.isReady, true, "isReady should be true after repeat startup"); + equal(gotStartupEvent, true, "Should have seen onStartup event after repeat startup"); + equal(gotShutdownEvent, false, "Should not have seen onShutdown event before shutdown, following repeat startup"); +}); diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_provider_markSafe.js b/toolkit/mozapps/extensions/test/xpcshell/test_provider_markSafe.js new file mode 100644 index 00000000000..55d503f2c81 --- /dev/null +++ b/toolkit/mozapps/extensions/test/xpcshell/test_provider_markSafe.js @@ -0,0 +1,47 @@ +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2"); + +let startupOrder = []; + +function mockAddonProvider(name) { + let mockProvider = { + markSafe: false, + apiAccessed: false, + + startup() { + if (this.markSafe) + AddonManagerPrivate.markProviderSafe(this); + + let uri = Services.io.newURI("beard://long", null, null); + AddonManager.isInstallEnabled("made-up-mimetype"); + }, + supportsMimetype(mimetype) { + this.apiAccessed = true; + return false; + }, + + get name() name, + }; + + return mockProvider; +}; + +function run_test() { + run_next_test(); +} + +add_task(function* testMarkSafe() { + do_print("Starting with provider normally"); + let provider = mockAddonProvider("Mock1"); + AddonManagerPrivate.registerProvider(provider); + startupManager(); + ok(!provider.apiAccessed, "Provider API should not have been accessed"); + AddonManagerPrivate.unregisterProvider(provider); + yield promiseShutdownManager(); + + do_print("Starting with provider that marks itself safe"); + provider.apiAccessed = false; + provider.markSafe = true; + AddonManagerPrivate.registerProvider(provider); + startupManager(); + ok(provider.apiAccessed, "Provider API should have been accessed"); +}); diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_provider_unsafe_access_shutdown.js b/toolkit/mozapps/extensions/test/xpcshell/test_provider_unsafe_access_shutdown.js new file mode 100644 index 00000000000..df717f5a573 --- /dev/null +++ b/toolkit/mozapps/extensions/test/xpcshell/test_provider_unsafe_access_shutdown.js @@ -0,0 +1,61 @@ +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2"); + +let shutdownOrder = []; + +function mockAddonProvider(name) { + let mockProvider = { + hasShutdown: false, + unsafeAccess: false, + + shutdownCallback: null, + + startup() { }, + shutdown() { + this.hasShutdown = true; + shutdownOrder.push(this.name); + if (this.shutdownCallback) + return this.shutdownCallback(); + }, + getAddonByID(id, callback) { + if (this.hasShutdown) { + unsafeAccess = true; + } + callback(null); + }, + + get name() name, + }; + + return mockProvider; +}; + +function run_test() { + run_next_test(); +} + +add_task(function* unsafeProviderShutdown() { + let firstProvider = mockAddonProvider("Mock1"); + AddonManagerPrivate.registerProvider(firstProvider); + let secondProvider = mockAddonProvider("Mock2"); + AddonManagerPrivate.registerProvider(secondProvider); + + startupManager(); + + let shutdownPromise = null; + yield new Promise(resolve => { + secondProvider.shutdownCallback = function() { + return new Promise(shutdownResolve => { + AddonManager.getAddonByID("does-not-exist", () => { + shutdownResolve(); + resolve(); + }); + }); + }; + + shutdownPromise = promiseShutdownManager(); + }); + yield shutdownPromise; + + equal(shutdownOrder.join(","), ["Mock1", "Mock2"].join(","), "Mock providers should have shutdown in expected order"); + ok(!firstProvider.unsafeAccess, "First registered mock provider should not have been accessed unsafely"); +}); diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_provider_unsafe_access_startup.js b/toolkit/mozapps/extensions/test/xpcshell/test_provider_unsafe_access_startup.js new file mode 100644 index 00000000000..867dc967367 --- /dev/null +++ b/toolkit/mozapps/extensions/test/xpcshell/test_provider_unsafe_access_startup.js @@ -0,0 +1,53 @@ +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2"); + +let startupOrder = []; + +function mockAddonProvider(name) { + let mockProvider = { + hasStarted: false, + unsafeAccess: false, + + startupCallback: null, + + startup() { + this.hasStarted = true; + startupOrder.push(this.name); + if (this.startupCallback) + this.startupCallback(); + }, + getAddonByID(id, callback) { + if (!this.hasStarted) { + unsafeAccess = true; + } + callback(null); + }, + + get name() name, + }; + + return mockProvider; +}; + +function run_test() { + run_next_test(); +} + +add_task(function* unsafeProviderStartup() { + let secondProvider = null; + + yield new Promise(resolve => { + let firstProvider = mockAddonProvider("Mock1"); + firstProvider.startupCallback = function() { + AddonManager.getAddonByID("does-not-exist", resolve); + }; + AddonManagerPrivate.registerProvider(firstProvider); + + secondProvider = mockAddonProvider("Mock2"); + AddonManagerPrivate.registerProvider(secondProvider); + + startupManager(); + }); + + equal(startupOrder.join(","), ["Mock1", "Mock2"].join(","), "Mock providers should have hasStarted in expected order"); + ok(!secondProvider.unsafeAccess, "Second registered mock provider should not have been accessed unsafely"); +}); diff --git a/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini b/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini index f4f6333de02..ccd4367ffe3 100644 --- a/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini +++ b/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini @@ -11,11 +11,15 @@ support-files = [test_addon_path_service.js] [test_asyncBlocklistLoad.js] [test_DeferredSave.js] +[test_isReady.js] [test_metadata_update.js] [test_openh264.js] run-if = appname == "firefox" [test_pluginInfoURL.js] +[test_provider_markSafe.js] [test_provider_shutdown.js] +[test_provider_unsafe_access_shutdown.js] +[test_provider_unsafe_access_startup.js] [test_shutdown.js] [test_XPIcancel.js] [test_XPIStates.js]