553092 - Initialise the permissions manager

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dave, can you please give a bit more details on that bug? Thanks.

Comment 2

[Dave Townsend [:mossop]]

The new code doesn't yet initialise the extension install whitelist

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dave, can we please get this bug fixed before our testday next Friday? It's a frustrating thing to always have to open the options dialog first, before you will be able to install any extension. I would like to see that we do not run into this problem during the testday.

Comment 4

[Dave Townsend [:mossop]]

Landed in http://hg.mozilla.org/projects/addonsmgr/rev/4ac299ae3dd0 and http://hg.mozilla.org/projects/addonsmgr/rev/7d4615d25f99

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This is a simple patch that just removes all the various places we currently initialize the permissions manager and just does it on startup whenever the app changes. This shouldn't be costly, the permissions manager is loaded on every startup anyway (by opening the hidden window) and keeping the initialisation all in one place makes far more sense.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 440859 [details] [diff] [review]
patch rev 1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -7570,33 +7570,16 @@ var LightWeightThemeWebInstaller = {
>     gBrowser.tabContainer.removeEventListener("TabSelect", this, false);
> 
>     this._manager.resetPreview();
>   },
> 
>   _isAllowed: function (node) {
>     var pm = Services.perms;
> 
>-    var prefs = [["xpinstall.whitelist.add", pm.ALLOW_ACTION],
>-                 ["xpinstall.whitelist.add.36", pm.ALLOW_ACTION],
>-                 ["xpinstall.blacklist.add", pm.DENY_ACTION]];
>-    prefs.forEach(function ([pref, permission]) {
>-      try {
>-        var hosts = gPrefService.getCharPref(pref);
>-      } catch (e) {}
>-
>-      if (hosts) {
>-        hosts.split(",").forEach(function (host) {
>-          pm.add(makeURI("http://" + host), "install", permission);
>-        });
>-
>-        gPrefService.setCharPref(pref, "");
>-      }
>-    });
>-
>     var uri = node.ownerDocument.documentURIObject;
>     return pm.testPermission(uri, "install") == pm.ALLOW_ACTION;
nit: just use Services.perms

Comment 7

[Dave Townsend [:mossop]]

http://hg.mozilla.org/mozilla-central/rev/393d1f6b7c3b

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100514 Minefield/3.7a5pre.

Dave, I have one remaining question for the Litmus test. Before that patch went in the user always had to open the preferences dialog to initialize the permission manager, right? Also after a restart and not only for a fresh profile.

Comment 9

[Dave Townsend [:mossop]]

This would have only affected fresh profiles and yes previously opening the permissions preferences would have initialized it.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #9)
> This would have only affected fresh profiles and yes previously opening the
> permissions preferences would have initialized it.

So a test like the following would fulfill all needs?

1. Create a fresh profile
2. Open AMO and try to install extension X

As result the extension has to be installed.

Comment 11

[Dave Townsend [:mossop]]

As long as the install dialog appears and no notification bar then yes that is a pass.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Litmus test has been added:
https://litmus.mozilla.org/show_test.cgi?id=15217

Such a test would be perfect for Mozmill. Adding whiteboard entry.