Closed Bug 1549192 Opened 5 years ago Closed 5 years ago

webextensions shouldn't depend on getting a reliable ADDON_ENABLE startup

Categories

(WebExtensions :: General, defect)

defect
Not set
normal

Tracking

(relnote-firefox 66+, firefox-esr60 wontfix, firefox66 wontfix, firefox67 wontfix, firefox68 fixed)

RESOLVED FIXED
mozilla68
Tracking Status
relnote-firefox --- 66+
firefox-esr60 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: bmaris, Assigned: aswan)

References

Details

(Whiteboard: cert2019)

Attachments

(3 files)

  1. Download 66.0.3 and disable normandy.
  2. Install https://addons.mozilla.org/en-US/firefox/addon/binghomepage/?src=search addon.
  3. Make sure that the homepage changed to something from Bing.
  4. Simulate add-on disabling.
  5. Update to latest RC 66.0.4-build3 (which contains the certificate fix)

Expected results: Homepage changes back to he one the user used to have (bing)

Actual results: Homepage remains the default one after updating to latest dot release (new:tab in this case).

Note:
Please note that if the hotfix is not skipped then the homepage will change back to the one that the user had (bing).

See Also: → 1549145

Andrew, Kris, Dave -- Gijs is starting to look at this, but I'd appreciate if one of you could take over from him once your online and up-to-speed. I'll follow up on slack. Thanks.

Assignee: nobody → gijskruitbosch+bugs
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(dtownsend)
Flags: needinfo?(aswan)

For search (bug 1549145), when testing with nightly, I get:

" {file: "resource://gre/modules/SearchService.jsm" line: 709}]'[JavaScript Error: "Something tried to use the search service before it's been properly intialized. Please examine the stack trace to figure out what and where to fix it:
_ensureInitialized@resource://gre/modules/SearchService.jsm:709:15
get defaultEngine@resource://gre/modules/SearchService.jsm:2264:10
processDefaultSearchSetting@chrome://browser/content/parent/ext-chrome-settings-overrides.js:99:9
async*removeSearchSettings@chrome://browser/content/parent/ext-chrome-settings-overrides.js:146:12
onUpdate@chrome://browser/content/parent/ext-chrome-settings-overrides.js:174:12
apiManager</</<@resource://gre/modules/ExtensionParent.jsm:103:16
Async*apiManager</<@resource://gre/modules/ExtensionParent.jsm:101:46
async*emit@resource://gre/modules/ExtensionCommon.jsm:310:24
update@resource://gre/modules/Extension.jsm:1313:23
callBootstrapMethod@resource://gre/modules/addons/XPIProvider.jsm:1742:33
_install@resource://gre/modules/addons/XPIProvider.jsm:1908:18
update@resource://gre/modules/addons/XPIProvider.jsm:1986:17
async*applyStartupChange@resource://gre/modules/addons/XPIDatabase.jsm:2912:67
processFileChanges@resource://gre/modules/addons/XPIDatabase.jsm:2826:26
checkForChanges@resource://gre/modules/addons/XPIProvider.jsm:2747:55
startup@resource://gre/modules/addons/XPIProvider.jsm:2293:12
callProvider@resource://gre/modules/AddonManager.jsm:193:31
_startProvider@resource://gre/modules/AddonManager.jsm:568:5
startup@resource://gre/modules/AddonManager.jsm:723:14
startup@resource://gre/modules/AddonManager.jsm:2722:26
observe@resource://gre/modules/addonManager.js:65:29
" {file: "resource://gre/modules/SearchService.jsm" line: 709}]' when calling method: [nsISearchService::defaultEngine]

which sort of makes sense - we're initing the search service early as a result of the early startup check for changes and that breaks things. I don't know to what degree this issue is the same as on release, but I wouldn't be surprised if there was a similar issue even if the specifics differed. Will look at the homepage thing next.

I can confirm I see something similar in release:

DEPRECATION WARNING: Search service falling back to synchronous initialization. This is generally the consequence of an add-on using a deprecated search service API.
You may find more details about this deprecation at: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIBrowserSearchService#async_warning
jar:file:///Desktop/Firefox.app/Contents/Resources/omni.ja!/components/nsSearchService.js 2703 SRCH_SVC__ensureInitialized
jar:file:///Desktop/Firefox.app/Contents/Resources/omni.ja!/components/nsSearchService.js 4185 get currentEngine
jar:file:///Desktop/Firefox.app/Contents/Resources/omni.ja!/components/nsSearchService.js 4178 get defaultEngine
chrome://browser/content/parent/ext-chrome-settings-overrides.js 89 processDefaultSearchSetting
Deprecated.jsm:77
    warning resource://gre/modules/Deprecated.jsm:77
    SRCH_SVC__ensureInitialized jar:file:///Desktop/Firefox.app/Contents/Resources/omni.ja!/components/nsSearchService.js:2703
    get currentEngine jar:file:///Desktop/Firefox.app/Contents/Resources/omni.ja!/components/nsSearchService.js:4185
    get defaultEngine jar:file:///Desktop/Firefox.app/Contents/Resources/omni.ja!/components/nsSearchService.js:4178
    processDefaultSearchSetting chrome://browser/content/parent/ext-chrome-settings-overrides.js:89
    next self-hosted:1210

Locking this bug down to only users who have editbugs permissions, for now.

Restrict Comments: true

I reproduced this and on a whim I checked the extension startupReason. For a reason I haven't traced, it is "APP_STARTUP" and not "ADDON_ENABLE".

Flags: needinfo?(dtownsend)

So based on comment 5, in addition to homepage, this potentially affects extensions that use these apis:
browser.browserSettings.*
browser.contextualIdentities.*
browser.privacy.*
browser.proxy.settings.*

And to be clear, users who get their addons repaired by the hotfix will be fine, this only happens to users who have existing addons but don't get the hotfix, who then do update to 66.0.4.

Andrew took this over so assigning this.

Assignee: gijskruitbosch+bugs → aswan
Whiteboard: cert2019

So the issue here is that for affected users who never got the hotfix but went straight to 66.0.4 (or 0.5), their addons were all re-verified early in startup (in the not-very-clearly-named XPIDatabase.processFileChanges() for what its worth). When that happens, we don't keep any record of that transition and when extensions are started, APP_STARTUP is passed as the reason.

But various parts of the webextensions framework (in particular, ExtensionPreferencesManager) assume that after an addon is disabled, they will get a startup with ADDON_ENABLE as the reason. Bootstrap startup reasons have never been particularly reliable, rather than doing a bunch of work in the addon manager for this case, I think we should just stop using this particular footgun and make ExtensionPreferencesManager recognize by itself when something needs to be re-enabled rather than depending on startupReason.

Fixing the damage this caused is a separate issue, I'll address that in bug 1549145.

Component: Untriaged → General
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)
Product: Firefox → WebExtensions
Summary: Homepage add-ons do not reactivate after directly update to 66.0.4-build3 skipping hotfix → webextensions shouldn't depend on getting a reliable ADDON_ENABLE startup

And upon further consideration, a similar problem also applies to similar code that looks at ADDON_DISABLE as the shutdownReason

API implementations that check shutdownReason for values other than
APP_SHUTDOWN during extension shutdown are inherently broken since an
addon may be disabled or uninstalled between browser sessions, in which
case code that is meant to run in these cases will not get executed.
This patch fixes existing api implementations that are broken in this way.

Also ensure that APIs' onDisabled methods get called properly when an
extension is disabled between browser sessions.

Checking extension.shutdownReason for any purpose other than detecting
app shutdown is unreliable, since actions such as disabing, uninstalling,
etc. may happen ito an extension after it has shut down. Remove the
temptation for api authors to write incorrect code by removing
extension.shutdownReason and replacing it with an isAppShutdown boolean
passed to shutdown handlers.

Is this something we want to try to do something for on ESR60 still or just let it ride the trains?

Flags: needinfo?(aswan)

The bug here was only an issue if disabled extensions got re-enabled during browser startup. That would only happen in practice if we did something silly like letting a signing certificate expire and then pushing a dot release to fix it. Seriously, I don't think there's any good reason to try to uplift this back to esr60.

Flags: needinfo?(aswan)
Depends on: 1562212
Regressions: 1562212
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: