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

RESOLVED FIXED in Thunderbird 65.0

Status

defect
--
major
RESOLVED FIXED
5 years ago
8 months ago

People

(Reporter: MattN, Assigned: mkmelin)

Tracking

({addon-compat})

unspecified
Thunderbird 65.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr52 wontfix, thunderbird_esr6064+ fixed, thunderbird64 fixed, thunderbird65 fixed)

Details

()

Attachments

(1 attachment, 3 obsolete attachments)

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
Assignee: nobody → mkmelin+mozilla
QA Contact: mkmelin+mozilla
Summary: Switch from xpinstall.whitelist.add to using a default permissions file → Switch Thunderbird from xpinstall.whitelist.add to using a default permissions file
QA Contact: mkmelin+mozilla
Depends on: 1073095
This is the thunderbird port, but we need a resolution to bug 1073095 first.
Status: NEW → ASSIGNED
To test, remove permissions.sqlite from your profile before starting.
Attachment #8544813 - Attachment is obsolete: true
Attachment #8565119 - Flags: review?(bwinton)
... after that, you should still be able to install an extension from amo like before.
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. ;)
Attachment #8565119 - Flags: review?(bwinton) → review+
Comment on attachment 8565119 [details] [diff] [review]
bug1072748_tb_default_permissions.patch

Joshua, want to take double check?
Attachment #8565119 - Flags: feedback?(Pidgeot18)
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...
Attachment #8565119 - Flags: feedback?(Pidgeot18) → feedback+
https://hg.mozilla.org/comm-central/rev/bd2fc52626fc -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
Backed out for XPCshell and Mozmill bustage: http://hg.mozilla.org/comm-central/rev/7bd8677b6b41
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
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?
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).
Status: REOPENED → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Pretty sure it's still broken (as is). See bug 1168178.
Flags: needinfo?(mkmelin+mozilla)
Target Milestone: Thunderbird 38.0 → ---
See Also: → 1168178
Summary: Switch Thunderbird from xpinstall.whitelist.add to using a default permissions file → Switch Thunderbird from xpinstall.whitelist.add to using a default permissions file (port bug 1050080)
Blocks: 1482780
Magnus, can we finish this off ASAP for bug 1482780 comment 3?  See also bug 1482780 comment 2
Flags: needinfo?(mkmelin+mozilla)
Thanks.
What is next step?  
When do you want to move to beta?
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.
Depends on: 1506390
Bug 1506390 is now on inbound. Once that's merged we can land this too.
Attachment #8565119 - Attachment is obsolete: true
Attachment #8567681 - Attachment is obsolete: true
Attachment #9025007 - Flags: review?(jorgk)
Comment on attachment 9025007 [details] [diff] [review]
bug1072748_tb_default_permissions.patch

Looks OK. Any try runs for this?
Attachment #9025007 - Flags: review?(jorgk) → review+
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?
Flags: needinfo?(mkmelin+mozilla)
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
Flags: needinfo?(mkmelin+mozilla)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6d676a4a8ef9
Switch Thunderbird from xpinstall.whitelist.add to using a default permissions file. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 5 years ago8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
Attachment #9025007 - Flags: approval-comm-esr60+
Attachment #9025007 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.