1549192 - webextensions shouldn't depend on getting a reliable ADDON_ENABLE startup

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Bogdan Maris, Desktop Test Engineering]

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).

Comment 1

[Maire Reavy [:mreavy]]

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.

Comment 2

[:Gijs (he/him)]

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.

Comment 3

[:Gijs (he/him)]

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

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 5

[Andrew Swan [:aswan]]

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".

Comment 6

[Andrew Swan [:aswan]]

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.*

Comment 7

[Andrew Swan [:aswan]]

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.

Comment 8

[:Gijs (he/him)]

Andrew took this over so assigning this.

Comment 11

[Andrew Swan [:aswan]]

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.

Comment 12

[Andrew Swan [:aswan]]

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

Comment 13

[Andrew Swan [:aswan]]

Attachment: Bug 1549192 Send ADDON_ENABLE startupReason for addons enabled early during browser startup r=kmag

Comment 14

[Andrew Swan [:aswan]]

Attachment: Bug 1549192 Fix extension apis that handle addon disabling r=kmag

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.

Comment 15

[Andrew Swan [:aswan]]

Attachment: Bug 1549192 Remove extension shutdownReason footgun r=kmag

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.

Comment 16

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90b6ea6c5c2ac2736134dbbc6d21c6515b13e8a3
Bug 1549192 Send ADDON_ENABLE startupReason for addons enabled early during browser startup r=kmag

https://hg.mozilla.org/integration/mozilla-inbound/rev/46b6ab028e3cb6259bae478a38c4b590384e7d4c
Bug 1549192 Fix extension apis that handle addon disabling r=kmag

https://hg.mozilla.org/integration/mozilla-inbound/rev/998d83fa2ae988509435ba88de2c640f4607f2fa
Bug 1549192 Remove extension shutdownReason footgun r=kmag

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/90b6ea6c5c2a
https://hg.mozilla.org/mozilla-central/rev/46b6ab028e3c
https://hg.mozilla.org/mozilla-central/rev/998d83fa2ae9

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Andrew Swan [: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.