1072748 - Switch Thunderbird from xpinstall.whitelist.add to using a default permissions file (port bug 1050080)

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Matthew N. [:MattN]]

Bug 1050080 needs to be ported to Thunderbird to unbreak add-on/LWT installation (perhaps just in new profiles).

Bug 506446 hardcoded a path of "resource://app/chrome/browser/default_permissions" for the default location but I don't know if that works for Thunderbird so may need to be changed or overridden via the permissions.manager.defaultsUrl preference.

Requesting tracking since this needs to be fixed in 35 or else we may want to backout 1050080?

MXR search: https://mxr.mozilla.org/comm-central/search?string=whitelist.add

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1072748_tb_default_permissions.patch

This is the thunderbird port, but we need a resolution to bug 1073095 first.

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: bug1072748_tb_default_permissions.patch

To test, remove permissions.sqlite from your profile before starting.

Comment 3

[Magnus Melin [:mkmelin]]

... after that, you should still be able to install an extension from amo like before.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8565119 [details] [diff] [review]
bug1072748_tb_default_permissions.patch

rs=me, by code inspection.

(It would probably be nice to get someone who has worked with the build system a little more to double-check the patch. ;)

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 8565119 [details] [diff] [review]
bug1072748_tb_default_permissions.patch

Joshua, want to take double check?

Comment 6

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8565119 [details] [diff] [review]
bug1072748_tb_default_permissions.patch

Review of attachment 8565119 [details] [diff] [review]:
-----------------------------------------------------------------

This way of installing a file seems odd to me, but it's the same mechanism used by the browser folks...

Comment 7

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/bd2fc52626fc -> FIXED

Comment 8

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

Backed out for XPCshell and Mozmill bustage: http://hg.mozilla.org/comm-central/rev/7bd8677b6b41

Comment 9

[Magnus Melin [:mkmelin]]

17:27:08     INFO -  PROCESS | 26028 | [26028] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /builds/slave/tb-c-cen-lx-d-0000000000000000/build/mozilla/extensions/cookie/nsPermissionManager.cpp, line 1878
17:27:08     INFO -  (xpcshell/head.js) | test pending (3)
17:27:08     INFO -  (xpcshell/head.js) | test MAIN run_test finished (3)
17:27:08     INFO -  running event loop
17:27:08     INFO -  TEST-PASS | extensions/cookie/test/unit/test_permmanager_notifications.js | permission_observer.prototype.observe - [permission_observer.prototype.observe : 82] "perm-changed" == "perm-changed"
17:27:08     INFO -  TEST-PASS | extensions/cookie/test/unit/test_permmanager_notifications.js | permission_observer.prototype.observe - [permission_observer.prototype.observe : 93] "test/expiration-perm" == "test/expiration-perm"
17:27:08     INFO -  TEST-PASS | extensions/cookie/test/unit/test_permmanager_notifications.js | permission_observer.prototype.observe - [permission_observer.prototype.observe : 94] 2 == 2
17:27:08     INFO -  TEST-PASS | extensions/cookie/test/unit/test_permmanager_notifications.js | permission_observer.prototype.observe - [permission_observer.prototype.observe : 95] 1424482128000 == 1424482128000
17:27:08     INFO -  (xpcshell/head.js) | test pending (3)
17:27:08     INFO -  (xpcshell/head.js) | test finished (3)
17:27:08     INFO -  TEST-PASS | extensions/cookie/test/unit/test_permmanager_notifications.js | permission_observer.prototype.observe - [permission_observer.prototype.observe : 82] "perm-changed" == "perm-changed"
17:27:08     INFO -  TEST-PASS | extensions/cookie/test/unit/test_permmanager_notifications.js | permission_observer.prototype.observe - [permission_observer.prototype.observe : 106] "test/expiration-perm" == "test/expiration-perm"
17:27:08     INFO -  TEST-PASS | extensions/cookie/test/unit/test_permmanager_notifications.js | permission_observer.prototype.observe - [permission_observer.prototype.observe : 107] 2 == 2
17:27:08     INFO -  TEST-PASS | extensions/cookie/test/unit/test_permmanager_notifications.js | permission_observer.prototype.observe - [permission_observer.prototype.observe : 108] 1424482228000 == 1424482228000
17:27:08     INFO -  (xpcshell/head.js) | test pending (3)
17:27:08     INFO -  (xpcshell/head.js) | test finished (3)
17:27:08     INFO -  TEST-PASS | extensions/cookie/test/unit/test_permmanager_notifications.js | permission_observer.prototype.observe - [permission_observer.prototype.observe : 82] "perm-changed" == "perm-changed"
17:27:08     INFO -  TEST-PASS | extensions/cookie/test/unit/test_permmanager_notifications.js | permission_observer.prototype.observe - [permission_observer.prototype.observe : 119] "test/expiration-perm" == "test/expiration-perm"
17:27:08     INFO -  (xpcshell/head.js) | test pending (3)
17:27:08     INFO -  (xpcshell/head.js) | test finished (3)
17:27:08     INFO -  TEST-PASS | extensions/cookie/test/unit/test_permmanager_notifications.js | permission_observer.prototype.observe - [permission_observer.prototype.observe : 82] "perm-changed" == "perm-changed"
17:27:08     INFO -  TEST-PASS | extensions/cookie/test/unit/test_permmanager_notifications.js | permission_observer.prototype.observe - [permission_observer.prototype.observe : 127] true == true

http://mxr.mozilla.org/comm-central/source/mozilla/extensions/cookie/nsPermissionManager.cpp#1878

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: bug1072748_defaults_debug.patch

The very odd thing here is that the test failure verifies things work in thunderbird.

Did some investigation and extensions/cookie/test/unit/test_permmanager_notifications.js seems to work by accident for firefox. pm.removeAll() calls nsPermissionManager::ImportDefaults in http://mxr.mozilla.org/comm-central/source/mozilla/extensions/cookie/nsPermissionManager.cpp?force=1#1089 so the default permissions are re-imported => the test that there aren't any permissions left should fail - http://mxr.mozilla.org/comm-central/source/mozilla/extensions/cookie/test/unit/test_permmanager_notifications.js#128

This turns out to be because the permissions.manager.defaultsUrl pref is not set. 
This is a pref set in firefox.js. Even if I set permissions.manager.defaultsUrl to resource://app/defaults/permissions in the test it turns out that URL can't be read (ImportDefaults can't get an inputStream) - which isn't true for firefox itself though.
In thunderbird the pref is set and the import succeeds, so the test fails.

So, are the tests supposed to be run with application prefs present or just as components?
And how come that's different in thunderbird vs firefox?

Comment 11

[Philip Chee]

Bug 1072751 is fixed. Do you want to try re-landing the patch in attachment 8565119 [details] [diff] [review]?

(Bug 1072751 comment #0)
> 0001-Bug-XXXXXXX-reinstate-PermissionUtils-and-XPIProvide.patch
> 
> Bug 1050080 removed PermissionUtils.jsm and XPIProvider.jsm, which turns out
> to break other apps (eg, bug 1072748, bug 1072744, bug bug 1072751).
> This bug is to revert those files (ie, is a partial backout of bug 1050080).

Comment 12

[Magnus Melin [:mkmelin]]

Pretty sure it's still broken (as is). See bug 1168178.

Comment 13

[Wayne Mery (:wsmwk)]

Magnus, can we finish this off ASAP for bug 1482780 comment 3?  See also bug 1482780 comment 2

Comment 14

[Magnus Melin [:mkmelin]]

Working on it 
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b310ffb4db4f26b2a629671c7e72a2319943a866

Comment 15

[Wayne Mery (:wsmwk)]

Thanks.
What is next step?  
When do you want to move to beta?

Comment 16

[Magnus Melin [:mkmelin]]

Will have to get the failing core tests fixed first. 

Sadly like the try run shows, things hadn't changed for the better since 2015 :( The tests are showing errors because we're running with the real prefs, and the test failures show up exactly because it *is* working. The tests are being run without Firefox prefs (for Firefox) and thus the Firefox defaults do not get picked up. 

Will have to patch the core tests to make sure they assume no defaults before we can land our patch.

Comment 17

[Magnus Melin [:mkmelin]]

Attachment: bug1072748_tb_default_permissions.patch

Bug 1506390 is now on inbound. Once that's merged we can land this too.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9025007 [details] [diff] [review]
bug1072748_tb_default_permissions.patch

Looks OK. Any try runs for this?

Comment 19

[Wayne Mery (:wsmwk)]

Thanks for the great progress. Given the bad user experience noted in bug 1482780 we should get this into 60.4.0, which is still a month away, if not before. (I'd be happy to take this in a point release)

Is that realistic given bug 1506390?  Will they take that on esr branch immediately?  In other words, what is the path forward so that users are't still facing this in 2019?

Comment 20

[Magnus Melin [:mkmelin]]

No new try run, but can do that after the merge.

For ESR, dunno about how strict they are with uplifts, but this is tests only, and not really changing any behaviour. We can take it on our 60 branch if it doesn't land on m-c esr

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6d676a4a8ef9
Switch Thunderbird from xpinstall.whitelist.add to using a default permissions file. r=jorgk

Comment 22

[Jorg K (CEST = GMT+2)]

TB 60.4 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/d3b45f75c1dbec8907242485f4c0f42f166a1b28

Comment 23

[Jorg K (CEST = GMT+2)]

Beta (TB 64 beta 4):
https://hg.mozilla.org/releases/comm-beta/rev/1376227af5a90240ebbdea18a89bb6a68d3d9db3