1073095 - nsPermissionManager.cpp references a browser path by default in kDefaultsUrlPrefName

Status: RESOLVED FIXED

(Core :: Permission Manager, defect)

Description

[Matthew N. [:MattN]]

> // If the pref above doesn't exist, the URL we use by default.
> static const char kDefaultsUrl[] = "resource://app/chrome/browser/default_permissions";

The path above contains "/browser/" which doesn't seem correct for code living in code shared between applications. Ehsan agreed with me on firefox-dev.

Comment 1

[Matthew N. [:MattN]]

I think resource://app/defaults/… may be a better place. We may even want to rename the leaf name then to get rid of the two occurrences of "default"

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: bug1073095_default_prefs_working.patch

It's not currently possible to use an app override for the pref as it checks for HasUserValue which would be false since it's the apps default!

This patch would fix that, but do we want to change the url to something better?

Comment 3

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Magnus Melin from comment #2)
> Created attachment 8544812 [details] [diff] [review]
> bug1073095_default_prefs_working.patch
> 
> It's not currently possible to use an app override for the pref as it checks
> for HasUserValue which would be false since it's the apps default!

I think MattN was proposing the default URI is (say) resource://app/defaults/permissions, which IIUC, would not change the preferences semantics at all.  IIUC, doing that would "simply" be a build change so that premissions file ends up under "defaults" in the omnijar and a change to kDefaultsUrl.

Comment 4

[Magnus Melin [:mkmelin]]

Sounds good to me, though I don't know how one would achieve that packaging scheme.

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug1073095_default_prefs_working.patch

Make it resource://app/defaults/permissions, and allow overriding in app too (not just user changed)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2ad96028d7c

Comment 6

[Benjamin Smedberg]

Comment on attachment 8555468 [details] [diff] [review]
bug1073095_default_prefs_working.patch

I'm not sure why we allow overriding via a pref, but that already exists so I'll let it go.

Comment 7

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8555468 [details] [diff] [review]
bug1073095_default_prefs_working.patch

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +1892,5 @@
>  nsresult
>  nsPermissionManager::ImportDefaults()
>  {
> +  nsCString defaultsURL = mozilla::Preferences::GetCString(kDefaultsUrlPrefName);
> +  if (defaultsURL.IsEmpty()) {

We explicitly called HasUserValue() so an empty string could indicate "no defaults".  With this patch I don't think there is a way to express this.  I guess this implies a comment is needed :/

> I'm not sure why we allow overriding via a pref, but that already
> exists so I'll let it go.

This was introduced in bug 506446, and although that bug doesn't clear say so, the idea is that, eg, enterprises can arrange for different default permissions.

Comment 8

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

To followup from a discussion with Benjamin, we explicitly called out the old behaviour in a post to the enterprise mailing list - https://mail.mozilla.org/private/enterprise/2014-September/004989.html:

> If you just want to *remove* all builtin permissions, you can create this preference as 
> an empty string.  Firefox will then have no default permissions.

Benjamin, I don't want to remove your r+, but also don't want to let this be changed without an explicit decision around this change in behaviour.

Comment 9

[Benjamin Smedberg]

Ugh. I don't think HasUserValue is correct here, but we should preserve the empty-pref behavior.

I think we should ship the value resource://app/defaults/permissions in firefox.js instead of hardcoded in this file, and then GetPref will return an empty/null string if there is no pref set and we can bail out of this function.

Comment 10

[Magnus Melin [:mkmelin]]

Pushed with that changed - https://hg.mozilla.org/integration/mozilla-inbound/rev/dc40389064bc

Comment 11

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Magnus Melin from comment #10)
> Pushed with that changed -
> https://hg.mozilla.org/integration/mozilla-inbound/rev/dc40389064bc

hmmm - r+ was removed from the patch!

Comment 12

[Magnus Melin [:mkmelin]]

Yes...? That's not r-
I interpreted that comment as r+ with that comment addressed, given it's a trivial modification.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc40389064bc

Comment 14

[Benjamin Smedberg]

I probably would have liked to see it again, but I've looked it over and it's fine.