Closed Bug 1073095 Opened 5 years ago Closed 5 years ago

nsPermissionManager.cpp references a browser path by default in kDefaultsUrlPrefName

Categories

(Core :: Permission Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed

People

(Reporter: MattN, Assigned: mkmelin)

References

Details

Attachments

(1 file, 1 obsolete file)

> // 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.
Flags: qe-verify-
Flags: firefox-backlog+
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"
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?
Blocks: 1072748
(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.
Sounds good to me, though I don't know how one would achieve that packaging scheme.
Assignee: nobody → mkmelin+mozilla
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
Attachment #8555468 - Flags: review?(benjamin)
Attachment #8555468 - Flags: feedback?(mhammond)
Status: NEW → ASSIGNED
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.
Attachment #8555468 - Flags: review?(benjamin) → review+
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.
Attachment #8555468 - Flags: feedback?(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.
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
Attachment #8555468 - Flags: review+
Pushed with that changed - https://hg.mozilla.org/integration/mozilla-inbound/rev/dc40389064bc
Target Milestone: --- → mozilla38
(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!
Yes...? That's not r-
I interpreted that comment as r+ with that comment addressed, given it's a trivial modification.
Flags: needinfo?(benjamin)
Attachment #8544812 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/dc40389064bc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Iteration: --- → 38.3 - 23 Feb
I probably would have liked to see it again, but I've looked it over and it's fine.
Flags: needinfo?(benjamin)
Blocks: 1161834
See Also: → 1190233
Depends on: 1366180
You need to log in before you can comment on or make changes to this bug.