Closed
Bug 1072748
Opened 10 years ago
Closed 6 years ago
Switch Thunderbird from xpinstall.whitelist.add to using a default permissions file (port bug 1050080)
Categories
(Thunderbird :: General, task)
Thunderbird
General
Tracking
(thunderbird_esr52 wontfix, thunderbird_esr6064+ fixed, thunderbird64 fixed, thunderbird65 fixed)
RESOLVED
FIXED
Thunderbird 65.0
People
(Reporter: MattN, Assigned: mkmelin)
References
()
Details
(Keywords: addon-compat)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
jorgk-bmo
:
review+
jorgk-bmo
:
approval-comm-beta+
jorgk-bmo
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
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 | ||
Updated•10 years ago
|
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
Assignee | ||
Updated•10 years ago
|
QA Contact: mkmelin+mozilla
Assignee | ||
Comment 1•10 years ago
|
||
This is the thunderbird port, but we need a resolution to bug 1073095 first.
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
To test, remove permissions.sqlite from your profile before starting.
Attachment #8544813 -
Attachment is obsolete: true
Attachment #8565119 -
Flags: review?(bwinton)
Assignee | ||
Comment 3•10 years ago
|
||
... after that, you should still be able to install an extension from amo like before.
Comment 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8565119 [details] [diff] [review]
bug1072748_tb_default_permissions.patch
Joshua, want to take double check?
Attachment #8565119 -
Flags: feedback?(Pidgeot18)
Comment 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
Comment 8•10 years ago
|
||
Backed out for XPCshell and Mozmill bustage: http://hg.mozilla.org/comm-central/rev/7bd8677b6b41
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
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•9 years ago
|
||
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)
Assignee | ||
Comment 12•9 years ago
|
||
Pretty sure it's still broken (as is). See bug 1168178.
Flags: needinfo?(mkmelin+mozilla)
Updated•8 years ago
|
Target Milestone: Thunderbird 38.0 → ---
Updated•6 years ago
|
status-thunderbird62:
--- → affected
status-thunderbird63:
--- → affected
status-thunderbird_esr52:
--- → affected
status-thunderbird_esr60:
--- → affected
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)
Comment 13•6 years ago
|
||
Magnus, can we finish this off ASAP for bug 1482780 comment 3? See also bug 1482780 comment 2
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 14•6 years ago
|
||
Working on it
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b310ffb4db4f26b2a629671c7e72a2319943a866
Flags: needinfo?(mkmelin+mozilla)
Comment 15•6 years ago
|
||
Thanks.
What is next step?
When do you want to move to beta?
Assignee | ||
Comment 16•6 years ago
|
||
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.
Assignee | ||
Comment 17•6 years ago
|
||
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 18•6 years ago
|
||
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+
Updated•6 years ago
|
status-thunderbird35:
affected → ---
status-thunderbird62:
affected → ---
status-thunderbird63:
affected → ---
status-thunderbird64:
--- → affected
status-thunderbird65:
--- → affected
tracking-thunderbird35:
? → ---
Comment 19•6 years ago
|
||
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)
Assignee | ||
Comment 20•6 years ago
|
||
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)
Comment 21•6 years ago
|
||
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: 10 years ago → 6 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 65.0
Updated•6 years ago
|
Attachment #9025007 -
Flags: approval-comm-esr60+
Attachment #9025007 -
Flags: approval-comm-beta+
Comment 22•6 years ago
|
||
Comment 23•6 years ago
|
||
Updated•5 years ago
|
Type: defect → task
You need to log in
before you can comment on or make changes to this bug.
Description
•