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.

Patch brings back a dummy Legacy API which is disabling Legacy Webextensions.

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.

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

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.

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.

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.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/514d2129ecbc
Disable Legacy WebExtensions in TB78. r=Fallen

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.

Thunderbird 81.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/6eec51ddaadb

Heads up to Patrick and Kai, but we anticipate this shouldn't affect the Enigmail add-on.

The Enigmail migrator is a true WebExtension using an experiment; this shouldn't affect us.

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?

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.

Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/b7a660c8653e

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.

https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=514d2129ecbca6bf3426a291054b8e7b3336f7ab

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

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?

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

I've just looked at the patch, and I see a couple of potential issues:

• _legacyCheck is an async function that is not awaited 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

This is not a replacement, but just a fix for what currently goes wrong.

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.

Backout Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/85fc86689124

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.

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.

Backed out from trunk: https://hg.mozilla.org/comm-central/rev/472a119b532f83feded21ce542a3eeacc2e32e08

Yeah, sorry, I was going to do it at the m-c merge yesterday, but that never happened.

https://support.mozilla.org/en-US/questions/1302178#answer-1346371 is an example?

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

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.

I'll let :rjl handle backing out from beta

fixing flag per backout https://hg.mozilla.org/releases/comm-esr78/rev/85fc86689124860ac5637083c75f60cb72b1c7dc

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.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=da075811c1531f71ea93561e24b4b118a8cfa305

Removed left over comment.

Note, we are going to need this early in 78.3.*

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/70cee5b9a331
Disable Legacy WebExtensions in TB78 - Take 3. r=Fallen

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:

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

Fine with me. Cannot wait for that candidate build!

Thunderbird 78.3.0:
https://hg.mozilla.org/releases/comm-esr78/rev/bb77d8e29de4

Link to treeherder build for c-esr78 build in comment 44. It just started so give in an hour or so.

https://treeherder.mozilla.org/#/jobs?repo=comm-esr78&revision=bb77d8e29de4c166c343bef799380ad596543c10

I do not understand this at all. It works in Daily where the patch landed yesterday, but not in the candidate build.

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!!!!!

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.

Thanks Walt. Those are great results.