209475 - Make nsIPermissionManager more flexible for extensions

Status: RESOLVED FIXED

(Core :: Graphics: Image Blocking, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

Currently, there are 3 constants defined for different types in
nsIPermissionManager (cookies, images and popups). The internal storage allows
for 16 different types. But an extension can not be sure if it is the only one
using type 8. If it wants to be sure that it is unique, it needs to update
nsIPermissionManager.idl. This means that it can never be frozen. So there needs
to be some way of making this more flexible.

One way would be to use strings instead of numbers to define the different
types. This would make it more easy to find something you can be resonably sure
of that it will be unique.
It would be even faster if the consumer of nsIPermissionManager would use a
string to get a number, and use that number to get and set permissions.
The string to number mapping can be stored in a header in cookperm.txt. For the
permissions in the file, the current format can be used.

Another thing is that nsIPermissionManager can store more then the current
permissions. At the moment, there are only constants for accept and reject. But
internally, it can hold more values. There can be general values, like prompt,
or type specific, like only session cookies.
The general values can be added as a constant, but the type-specifix ones should
depent on the consumer. At least there needs to be more documentation that it
is possible to store your own values.

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: First suggestion for additions to nsIPermissionManager.idl

Of course, this still needs changes to the backend :)

Comment 2

[Nicholas Allen]

mvl-

The comment you've got is that nsIPermissionManager allows 16 types with 16
actions but in the implementation I see it's 8 types with 256 actions. 
Shouldn't these match up?

Also, I'm having trouble parsing the sentence:

+   *                                       The number returned will
+   * be the same for each profile, but not neccesary for each profile.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Yeah, there are currently only 8 types allowed. I remembered it wrong. Same for
the number of permissions. But that was not intended from the start, so i don't
want to guarantee 256 for the future. And 16 sounds enough :)

The number of permissions is something that the interface should not worry
about. It should only say that you might not get a new number, as they are limited.

And yes, that sentence is bad :)
   * The number returned will
   * be the same each time the same profile is used, but not neccesary
   * for different profiles.
might be better.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v1

This makes it possible to register your type with a string to the permission
manager.
The old COOKE_TYPE and frines are still there, but you shouldn't use them
anymore. When removing them, things like the UI will break.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 126637 [details] [diff] [review]
patch v1

Asking for some review... dunno who might be a good sr, just guessing :)

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

"a good sr" as in actually interrested in this stuff :)

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v1.1

Some small updates. A comment in the .idl, initializing an outparam, some typos
in comments.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126642 [details] [diff] [review]
patch v1.1

>Index: extensions/cookie/nsIPermissionManager.idl

>+   * Allowed return values from the testPermission method. A consumer can
>+   * in addition use its own numbers

"In addition, a consumer can use ...."

> as long as they are between 3 and 16.

Should we document that restriction on testPermission as well or something?

>+   * These values are the different types of permissions supported.
>+   * Those are here for backwards compatibility. For new types, use
>+   * getType below.

"These are here", not "Those are here".

For that matter, how much do we care about backwards compat here?  Any reason
not to move to the new system, except for the fact that it's cumbersome?

>+  /**
>+   * Get a type number based on the string passed it. For the best
>+   * speed, the consumer should cache this. The number returned will
>+   * remain the same for a given profile, but may be different across 
>+   * profiles.

What is this type number used for?  Perhaps you should clearly state that any
cache that's kept must be cleared on profile switch?

>+   * This may fail, as the storage space might be limited.

What should consumers do in that case?	Should they just give up on being able
to check permissions for their type?  What exceptions could this method throw? 
Could we have some Javadoc-style @param, @return, @throws comments here?  I
assume that the return value could be any PRUint32?  Are there any "reserved"
values that a caller could init a static so as to be able to tell when it needs
to call getType (as in, when the static is not initialized)?

Are there any restrictions on typeString?  Does it have to be pure-ascii?  Does
it need to not contain any nulls?

>Index: extensions/cookie/nsPermissionManager.cpp

>+      if (i >= 3 && (firstEmpty == -1)) // Dont overrule the old types, for backwards compatibility
>+        firstEmpty = i;

How about a |break;| here?  Also, please do not overparenthesize the == test.

>+    // Can't user ToNewCSTring here, other strings in the array are allocated

"use"

>+    mTypeArray[firstEmpty] = PL_strdup(PromiseFlatCString(aTypeString).get());
>+    *aType = firstEmpty;
>+    return NS_OK;

Is it still NS_OK if PL_strdup returned null?

It sounds like case matters for type strings.  Document this in your idl, if
relevant...

>+  // (% sign is to distinguish tpye string lines from permission lines)

"type"

>+      bufferedOutputStream->Write(kTypeSign, sizeof(kTypeSign) - 1, &rv);

Shouldn't you check rv at some point here instead of just calling Write() after
the previous Write() failed?

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v2

I removed the GetType() method, and made testPermission() and add() to take
strings for the type. This would make the interface much more user-friendly. No
more problems when the profile change.
It might be slower, but a quick test showed that it is almost not noticable. In
same cases it might be faster, when you never added anything for a certain
type, it will no longer do a hash lookup.

I also fices Read and Write to actually save permissions other then accept and
deny.

Comment 10

[[e-mail]]

Comment on attachment 127075 [details] [diff] [review]
patch v2

I'll take a look at this tomorrow morning... until then, requesting sr=bz just
in case he has a few minutes to kill. :)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127075 [details] [diff] [review]
patch v2

>Index: extensions/cookie/nsCookiePermission.cpp

>+        mPermissionManager->Add(aURI, NS_LITERAL_CSTRING("Cookie"), 

Why capitalize?  I'm fine with doing it, as long as there is a good reason...

>Index: extensions/cookie/nsCookieService.cpp

>+            permissionManager->Add(uri, NS_LITERAL_CSTRING("Cookie"), nsIPermissionManager::DENY_ACTION);

Same.

>Index: extensions/cookie/nsIPermission.idl

>-    readonly attribute PRUint32 type;
>+    readonly attribute ACString type;

You should probably also mention that the type is case-sensitive here...

>Index: extensions/cookie/nsIPermissionManager.idl
>-   * @param permission is one of the enumerated permission actions above

You removed this, yet did not replace it with any new documentation... Please
document this arg.

>+   * @param type   a case-sensitive ASCII string, identifying the consumer.

If it's an ASCII string, it should be a |string|, not an |ACString|, in my
opinion.

>+   * @return       an integer from 1 to 15, representing the desired 

The method is declared void.  There should be no @return, hence... is this
supposed to be the @param permission documentation?

>   /**
>    * Test whether a website has permission to perform the given action.
>-   * @param  uri   the website to be tested
>-   * @param  type  one of the enumerated types above
>-   * @return       one of the enumerated permission actions above
>+   * @param uri     the uri to be tested
>+   * @param type    a case-sensitive ASCII string, identifying the consumer
>+   * @param return  see add(), param type. returns UNKNOWN_ACTION when there

You mean "param permission"?

What if there are no permissions stored for that type?

>Index: extensions/cookie/nsPermissionManager.cpp

>-inline void 
>-nsHostEntry::SetPermission(PRUint32 aType, PRUint32 aPermission)
>+void 
>+nsHostEntry::SetPermission(PRInt32 aType, PRUint32 aPermission)

Why this change?  This function looks perfect for inlining...

>-inline PRUint32 
>-nsHostEntry::GetPermission(PRUint32 aType) const
>+PRUint32 
>+nsHostEntry::GetPermission(PRInt32 aType) const

Same.  In fact, why not just put the impl in the header?

>+        permission = permissionString.CharAt(index) - kFirstLetter;

kFirstLetter is 'a', so this could end up being negative on the right...
|permission| is unsigned.  Is that really what we want?

>Index: extensions/cookie/tests/Makefile.in

> 		  pref \
>+                  cookie \

Odd indent?  Spaces instead of tabs or something?

The rest looks fine; please fix those nits...

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v2.5

Updated to the comments.
The comments int he .idl are updated, the cookie type string is now lowercase,
the strings are now string, not ACString. (it is still ACString in
nsPermission.idl, to prevent trouble freeing the string again)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127269 [details] [diff] [review]
patch v2.5

>+// This is more then required by the .idl, but it improves speed.
>+// The method used to save can only take 26 permissions, so using > 26 will
>+// fail. nsIPermissionManager.idl specifies 16 as the allowed number.

"than", not "then".  It's not really clear to me what this comment is saying
(what exactly will fail?)

Fix that comment to be readable, and sr=me

Comment 14

[[e-mail]]

Comment on attachment 127269 [details] [diff] [review]
patch v2.5

>Index: extensions/cookie/nsPermissionManager.cpp
>===================================================================
> inline PRBool 
> nsHostEntry::PermissionsAreEmpty() const
> {

we can move that into the header also...

>-      PRUint32 permission = (permissionString.CharAt(index) == 'T') ? nsIPermissionManager::ALLOW_ACTION : nsIPermissionManager::DENY_ACTION;
>+      PRUint32 permission = 0;
>+      if (permissionString.CharAt(index) == kTrue)
>+        permission = nsIPermissionManager::ALLOW_ACTION;
>+      else if (permissionString.CharAt(index) == kFalse)
>+        permission = nsIPermissionManager::DENY_ACTION;
>+      else
>+        permission = permissionString.CharAt(index) - kFirstLetter;
>+    
>+      // Bounds check
>+      if (permission < 0 || permission > 15)
>+        continue;

|permission| is unsigned, so no need for the < 0 check.

with those and bz's nits fixed, r=dwitte. mvl's asleep now, so i'll post a new
patch with these addressed, and see if I can find some camino/firebird folk to
check in the relevant parts.

Comment 15

[[e-mail]]

Attachment: patch v2.6

This does a couple extra things over what I mentioned in my last comment. We
weren't doing any bounds-checking on the |permission| parameter, either when
coming from the file (in nsPermissionManager::Read) or from the interface
method (nsPermissionManager:Add). So, I added a NUMBER_OF_PERMISSIONS #define
and put bounds checking in both of those consumers.

In addition, I shifted the bounds check for the |type| parameter out of
nsPermissionManager:AddInternal and into nsPermissionManager::Read, since we
want to continue looping if one type is bad - not abort the entire file being
read. (nsPermissionManager::Add does its own bounds-check, so removing this
from AddInternal is fine).

Comment 16

[[e-mail]]

Attachment: patch v2.61

er... fixes a silly mistake.

Comment 17

[[e-mail]]

checked in, with sr=bz on irc for the additional bits. thanks to bryner for
landing the browser/ portions.