1170200 - Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string

Status: RESOLVED FIXED

(Core :: Permission Manager, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

To allow for future changes to the implementation of nsPermissionManager to distinguish between http and https URIs, this patch changes the arguments to nsIPermissionManager::Remove to accept a nsIURI instead of a string.

This makes the API conform more closely to the API for nsIPermissionManager::Add, an allows for future changes to the backend to be made more easily.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

This patch should probably be split up to make reviewing it easier.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Oops - forgot to attach the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3989120d7e6d

Comment 4

[(no longer active)]

Comment on attachment 8613569 [details] [diff] [review]
Part 1: Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +1077,5 @@
>  
>  NS_IMETHODIMP
> +nsPermissionManager::RemoveFromPermission(nsIPermission* aPerm)
> +{
> +  nsCString host;

Nit: nsAutoCString, so that we'd avoid allocating a buffer for small strings.

@@ +1085,5 @@
> +  nsCOMPtr<nsIPrincipal> principal;
> +  rv = GetPrincipal(host, getter_AddRefs(principal));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCString type;

Ditto.

@@ +1089,5 @@
> +  nsCString type;
> +  rv = aPerm->GetType(type);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return RemoveFromPrincipal(principal, type.get());

This does something very different than what the comment says!

::: netwerk/base/nsIPermissionManager.idl
@@ +114,5 @@
>     * @param type   a case-sensitive ASCII string, identifying the consumer. 
>     *               The type must have been previously registered using the
>     *               add() method.
>     */
> +  void remove(in nsIURI uri,

Please rev the uuid of this interface.  You can use the ./mach update-uuids command for that purpose.

@@ +131,5 @@
> +   * This must be an exact permission, returned from, for example, the enumerator.
> +   *
> +   * @param perm   a permission returned from, for example, the enumerator
> +   */
> +  void removeFromPermission(in nsIPermission perm);

Nit: I think removePermission is a slightly better name.  Do you agree?

Comment 5

[(no longer active)]

Also, note that this needs tests!

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

Updated due to changes in Part 1

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8616888 [details] [diff] [review]
Part 1: Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string

Writing tests before review

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string

Added a test to ensure that nsIPermissionManager::RemovePermisison() only removes 1 permission at a time.

Comment 10

[(no longer active)]

Comment on attachment 8620403 [details] [diff] [review]
Part 1: Change the API for nsIPermissionManager::Remove() to accept a URI instead of a string

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

Looks great!

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

This fixes another use which was added since I wrote the patch initially

Comment 12

[Ian Nartowicz]

How are addons supposed to know which API to use? An addon, unlike Firefox core code, needs to support multiple versions of interfaces simultaneously, at a minimum over the lifespan of an ESR. Shouldn't this be a different method such as removeURI()?

Comment 13

[Nika Layzell [:nika] (ni? for response)]

This bug is part of the movement towards bug 1165263 - which would mean that a remove() method which accepts a host could no longer be supported, as it would not be meaningful (as internally we wouldn't be using a host anymore).

We can't support the old API for remove, so we are changing it to a new one which we will be able to support now and into the future.

Unfortunately, this change will break addons, but I don't think that there is a better way to do this.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8623869 [details] [diff] [review]
Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

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

::: browser/components/preferences/advanced.js
@@ +567,5 @@
>      // remove the permission
>      var pm = Components.classes["@mozilla.org/permissionmanager;1"]
>                         .getService(Components.interfaces.nsIPermissionManager);
> +    pm.removePermission(perm);
> +    pm.removePermission(perm);

This line shouldn't be in here twice. Does pm.removePermission(perm) remove both the nsIPermissionManager.ALLOW_ACTION and nsIOfllineCacheUpdateService.ALLOW_NO_WARN permissions?

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

I looked back at advanced.js file, and apparently it was late when I worked on it, because I did _everything_ wrong. Here's a new patch which should not have the problems of the old version. (hopefully)

With regard to your question about nsIPermissionManager.ALLOW_ACTION and nsIOfflineCacheUpdateService.ALLOW_NO_WARN, pm.remove() only takes 2 arguments. Those extra arguments to the remove method are actually ignored by the xpcom translation layer. (so in the old code, the same method was also being invoked twice).  This new version only invokes the method once, but performs some ugly code to get a URI back out of the host object. I improve it (in my opinion) in bug 1173523 when I get access to principals on nsIPermissions, which have better properties for this kind of stuff.

I'd appreciate it if you could take another look and make sure that I didn't do anything obviously wrong.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8624720 [details] [diff] [review]
Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

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

I only looked at advanced.js.

::: browser/components/preferences/in-content/advanced.js
@@ +606,5 @@
>      var pm = Components.classes["@mozilla.org/permissionmanager;1"]
>                         .getService(Components.interfaces.nsIPermissionManager);
> +    let uri;
> +    try {
> +      uri = ios.newURI(host, null, null);

Please add a comment that file:// URIs are stored with their scheme. Might also swap these (try putting the http:// scheme on first and if that throws then attempt without, since the http:// will be more common than file://).

Comment 17

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

Add comment about file:// URIs to advanced.js.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Ian Nartowicz from comment #12)
> How are addons supposed to know which API to use? An addon, unlike Firefox
> core code, needs to support multiple versions of interfaces simultaneously,
> at a minimum over the lifespan of an ESR. Shouldn't this be a different
> method such as removeURI()?

If I understand correctly, the add-on could attempt to use the new API in a try-catch block, and if that throws then in the catch block they could attempt to use the function with the old arguments. Does that help?

Comment 19

[Nika Layzell [:nika] (ni? for response)]

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

Comment 20

[Carsten Book [:Tomcat]]

part 2 failed to apply: renamed 1170200 -> Bug-1170200---Part-2-Update-JavaScript-consumers-o.patch
applying Bug-1170200---Part-2-Update-JavaScript-consumers-o.patch
patching file browser/base/content/urlbarBindings.xml
Hunk #1 FAILED at 2395
1 out of 1 hunks FAILED -- saving rejects to file browser/base/content/urlbarBindings.xml.rej

could you take a look, thanks!

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Update JavaScript consumers of nsIPermissionManager::Remove

Rebased. Hopefully the patch should apply now :)

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3309511f1db6
https://hg.mozilla.org/integration/mozilla-inbound/rev/65b6fc37a39a

Comment 23

[(no longer active)]

This broke browser-chrome tests: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&author=eakhgari%40mozilla.com

Comment 24

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/057f0bd4075c

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01c9c0047419
https://hg.mozilla.org/integration/mozilla-inbound/rev/d716c5d113d8

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/01c9c0047419
https://hg.mozilla.org/mozilla-central/rev/d716c5d113d8

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Closing old needinfo request...