Disable Legacy WebExtensions in TB78
Categories
(Thunderbird :: Add-Ons: Extensions API, enhancement)
Tracking
(thunderbird_esr78+ fixed, thunderbird81 fixed)
People
(Reporter: TbSync, Assigned: TbSync)
References
()
Details
Attachments
(1 file, 6 obsolete files)
3.10 KB,
patch
|
Fallen
:
review+
wsmwk
:
approval-comm-esr78+
|
Details | Diff | Splinter Review |
TB78 dropped support for the legacy API but Legacy WebExtensions using that can still be installed, if they do not have a strict_max_version set. :Sancus fixed ATN to automatically adjust the strict_max_version for these, so they cannot get installed from inside the Add-On Manager
However, the app fails to disable already installed Legacy WebExtensions on upgrade from TB68 to TB78. They remain enabled. This causes confusion and already produced a lot of error reports as users think such add-ons "should" work but fail on their systems and send bug reports.
Furthermore, these add-ons do not get automatically updated, if there is a new version.
Attached bug mitigates both issues. Currently it only searches for updates, but does not install them. It indicates an available update and I think that is enough.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
Patch brings back a dummy Legacy API which is disabling Legacy Webextensions.
Assignee | ||
Comment 2•4 years ago
|
||
An alternative approach suggested by :darktroyan and :Fallen is to monkey patch isUsableAddon, but I ran into strange issues and overall it was not pretty. It is difficult to get access to the full manifest (including the legacy API information) in isUsableAddon()
. I was able to use the extension
object retrieved via
ExtensionParent.GlobalManager.getExtension(aAddon.id);
but that seemed to be not up to date when used. During add-on install it was undefined
and during updates extension.manifest
returned the old and not the new manifest which appDisabled the new version which than could no longer be enabled.
I may be missing something, but from my results I would say that this approach will not be successful.
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
|
||
Wayne told me, we need this in 78.2.2 as that is the one which will activate auto update from TB68 to TB78. We can remove this dummy API with the next ESR (and even from beta once this landed in TB78).
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 5•4 years ago
|
||
This patch is a monkey patch for XPIDatabase.isDisabledLegacy()
to also check for legacy WebExtensions. It also checks for these during app startup and adds a "not compatible" notification to appDisabled add-ons.
Assignee | ||
Comment 6•4 years ago
•
|
||
This patch is a monkey patch for XPIDatabase.isDisabledLegacy()
to also check for legacy WebExtensions. It also checks for these during app startup and adds a "not compatible" notification to appDisabled add-ons.
Comment 7•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 8•4 years ago
|
||
This patch is a monkey patch for XPIDatabase.isDisabledLegacy()
to also check for legacy WebExtensions. It also checks for these during app startup and adds a "not compatible" notification to appDisabled add-ons.
Most of the review comments have been addressed. XPIInternal.awaitPromise()
is still used.
Assignee | ||
Updated•4 years ago
|
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/514d2129ecbc
Disable Legacy WebExtensions in TB78. r=Fallen
Updated•4 years ago
|
Comment 10•4 years ago
|
||
Comment on attachment 9173781 [details] [diff] [review]
legacyDetect.patch
[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Fixes a confusing situation with legacy addons.
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
[Triage Comment]
Taking for 81.0b3 prior to esr78 per wsmwk via Matrix.
Comment 11•4 years ago
|
||
bugherder uplift |
Thunderbird 81.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/6eec51ddaadb
Comment 12•4 years ago
|
||
Heads up to Patrick and Kai, but we anticipate this shouldn't affect the Enigmail add-on.
Comment 13•4 years ago
|
||
The Enigmail migrator is a true WebExtension using an experiment; this shouldn't affect us.
Comment 14•4 years ago
|
||
Comment on attachment 9173781 [details] [diff] [review]
legacyDetect.patch
[Triage Comment]
Approved for ers78
John, which add-on can testers use to test the change?
Assignee | ||
Comment 15•4 years ago
•
|
||
You can use the following add-ons to test the patch:
The add-on category manager 3.11:
https://addons.thunderbird.net/thunderbird/downloads/file/1016369/categorymanager-3.11-tb.xpi
Without the patch:
a) it will not be disabled when updating from TB68 to TB78
b) it can be installed in TB78 via file install
c) it cannot be installed in TB78 via add-on manager search results (sancus had marked all these cases incompatible on ATN)
With the patch:
a) it will be disabled when updating from TB68 to TB78 (with a "not compatible" notification)
b) it cannot be installed in TB78 via file install
c) <no change in behaviour>
The add-on quicktext 2.15:
https://addons.thunderbird.net/thunderbird/downloads/file/1016601/quicktext-2.15-tb.xpi
Quicktext 2.15 will show the same behaviour as Category Manager 3.11, but after it has been disabled after the update from TB68 to TB78 it should automatically update to a newer version (v3.2) and get automatically enabled again.
Comment 16•4 years ago
|
||
bugherder uplift |
Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/b7a660c8653e
Comment 17•4 years ago
|
||
I'd like to back this out on all branches.
When this patch landed on comm-central, we started seeing Mochitest failures on win32 every time. The failures did not occur prior to this patch landing.
Additionally, when this hit comm-esr78 earlier today, I realized that it could not be infrastructure like originally thought. I first ran a try push of comm-esr78 without this patch and did not see the test failures.
https://treeherder.mozilla.org/#/jobs?repo=comm-esr78&selectedTaskRun=dRM46-7AS76CyUiJpqrBxA.0&revision=8ec9b9d4f520697628102d97c199a9136c7e3c34
Looking at comm-beta, the push with this patch the errors started, prior to that we did not see the errors.
https://treeherder.mozilla.org/#/jobs?repo=comm-beta&revision=6eec51ddaadbcab19577cbde4fa03d3b0ef1abc8
I followed up with a backout of this patch on comm-central and ran a try job.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=33d752bb1e97a4255a82b64d6ac5dbb9bdf9f41d
Assignee | ||
Comment 18•4 years ago
|
||
Hm. That is bad. The backout changeset says:
This is causing all Mochitests on win32 to look like they are failing
when they are not. Close examination of the logs shows that the tests
themselves pass (for the most part) and the error is caused by failure
to remove a zip file. The zip file turns out to be the special-powers
addon that is used by Mochitest. The error itself is likely due to the
file handle not being properly closed when Thunderbird exits.
Is this what you refer to as infrastructure, but you no longer think the special-powers add-on is the reason?
Assignee | ||
Comment 19•4 years ago
•
|
||
Hm, the tests themselves pass. What a devastating morning. If you think it has to be backed out, then go ahead. I am trying to understand why...
Comment 20•4 years ago
|
||
I've just looked at the patch, and I see a couple of potential issues:
_legacyCheck
is an async function that is notawait
ed for. As a result, if startup & shutdown is a quick cycle (as for a lot of mochitests), it could still be running.- You might be able to change
_onMailStartupDone
to async and await on it, however... - _onMailStartupDone is being called at every startup, so it seems expensive to iterate through the list every time.
Normally, I'd suggest moving this across to MailMigrator._migrateUI
and doing it there. However, the disadvantage with that is we still can't handle the async nature well enough. Ideally, we would probably do this before displaying the UI, though I'm assuming that these add-ons won't be loaded at all, so there won't be any flicker.
For nightly, I think the best thing is to change _onFirstWindowLoaded
to be async, and then await
the _legacyCheck()
call. That would delay the marionette-startup-requested
until after the check had completed.
You should also create a preference that gets checked before the legacy check is run, and set after - I don't see a need for this to be called on every startup of a profile and it looks like it could be expensive.
You can probably just do the simple change to make _onFirstWindowLoaded
async and await and test that out on try before going further.
For beta/esr, we could either uplift bug 1660706, or move the _legacyCheck
to be called from completeStartup
and make that an async function: https://searchfox.org/comm-esr78/rev/4d56fa3e089d022378c7f86f5ac9c94a6e0b7b36/mail/base/content/msgMail3PaneWindow.js#736
Assignee | ||
Comment 21•4 years ago
|
||
This is not a replacement, but just a fix for what currently goes wrong.
Assignee | ||
Comment 22•4 years ago
|
||
Mark Banner has run a try run for me with the fix applied:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0668c7f2b8029d4ccb3294d1c34ac2fb1d1cc4e3
The fix of course just hides the issue. I think the final product does not fail but its something within the test system itself. I leave it up to Rob to decide if he wants to back out the patch or if we should proceed and get the second patch reviewed and landed.
I will continue to work on a clean patch.
Comment 23•4 years ago
|
||
backout bugherder uplift |
Backout Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/85fc86689124
Comment 24•4 years ago
|
||
I backed the change out on c-esr78 prior to 78.2.2 so the tests are functional.
There is question as to whether the fix from comment 21 will cause problems for users, so I'm inclined not to use it until it's tested. I'll leave the backout question on c-central to Geoff since tests are his module. I personally think we should backout until the proper fix is ready and then reapply.
Assignee | ||
Comment 25•4 years ago
•
|
||
I would like to ask to back out this patch from all branches. We saw something strange going on with a local test with win32. Currently, it is not obvious if other test fails on try are related to this or not.
I have level 1 commit access now and can push to try, so I can check any potential new patch.
Comment 26•4 years ago
|
||
Backed out from trunk: https://hg.mozilla.org/comm-central/rev/472a119b532f83feded21ce542a3eeacc2e32e08
Comment 27•4 years ago
|
||
Yeah, sorry, I was going to do it at the m-c merge yesterday, but that never happened.
Comment 28•4 years ago
|
||
Comment 29•4 years ago
|
||
(In reply to Wayne Mery (:wsmwk) from comment #28)
https://support.mozilla.org/en-US/questions/1302178#answer-1346371 is an example?
No, that's a theme, and themes work fine. It's this one: https://addons.thunderbird.net/en-US/thunderbird/addon/audi_r8/
He's in Dark Mode, which has white folder icons. So when you enable a theme that has a white frame color with Dark Mode, your folder icons just blend into the frame color.
Assignee | ||
Comment 30•4 years ago
|
||
Can we back this out of comm-beta as well? I want to start the process all over with a new patch and I think it would be best if the old patch is removed from beta as well.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment 34•4 years ago
|
||
fixing flag per backout https://hg.mozilla.org/releases/comm-esr78/rev/85fc86689124860ac5637083c75f60cb72b1c7dc
Updated•4 years ago
|
Assignee | ||
Comment 35•4 years ago
|
||
Removed usage of XPIInternal.awaitPromise
which seemed to be the cause for the win32 hiccups. This patch only disables already installed legacy add-ons, but does not prevent new installations. This requires more work.
Assignee | ||
Comment 36•4 years ago
|
||
Removed left over comment.
Comment 37•4 years ago
|
||
Comment 38•4 years ago
|
||
Note, we are going to need this early in 78.3.*
Assignee | ||
Updated•4 years ago
|
Comment 39•4 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/70cee5b9a331
Disable Legacy WebExtensions in TB78 - Take 3. r=Fallen
Assignee | ||
Comment 40•4 years ago
|
||
Comment on attachment 9175917 [details] [diff] [review]
bug1661216-take3.patch
[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is mainly intended for Users updating from ESR68 to ESR78.
- User impact if declined: Legacy add-ons do not get disabled upon ESR upgrade.
- Fix Landed on Version:
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky):
- String or UUID changes made by this patch:
Assignee | ||
Comment 41•4 years ago
|
||
Comment on attachment 9175917 [details] [diff] [review]
bug1661216-take3.patch
[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
Comment 42•4 years ago
|
||
Comment on attachment 9175917 [details] [diff] [review]
bug1661216-take3.patch
[Triage Comment]
No more betas for 81.
Approved for esr78 because I think we want this badly enough, to avoid v68 users being affected. So we should test the update path with the candidate build that it does what it intends and no more.
Assignee | ||
Comment 43•4 years ago
|
||
Fine with me. Cannot wait for that candidate build!
Comment 44•4 years ago
|
||
bugherder uplift |
Thunderbird 78.3.0:
https://hg.mozilla.org/releases/comm-esr78/rev/bb77d8e29de4
Comment 45•4 years ago
•
|
||
Link to treeherder build for c-esr78 build in comment 44. It just started so give in an hour or so.
Assignee | ||
Comment 46•4 years ago
|
||
I do not understand this at all. It works in Daily where the patch landed yesterday, but not in the candidate build.
Assignee | ||
Comment 47•4 years ago
|
||
BIG RELIEF. It does work. The treeherder link showed a red box which redirected me to a different build. Downloaded the correct build and now it works!!!!!
Comment 48•4 years ago
|
||
Hope this helps.
Installed 68.12.0 in my test user account on Windows10.
Created a test profile and email account.
Installed Category Manager 3.11, CompactHeader 3.0.0beta5, Manually sort folders 2.0.2 and Quicktext 2.15.
Edited prefs.js and using Help > About Thunderbird 68.12.0 was updated to 78.2.2.
It appeared as if all extensions were still enabled after that update.
Used Help > About again and was updated to 78.3.0.
Quicktext updated to version 3.2 and the rest had a yellow warning that the extensions were incompatible with Thunderbird 78.3.0.
Description
•