Invalid usage patterns for calling TestPermissionFromPrincipal()

ASSIGNED
Assigned to

Status

()

Core
General
ASSIGNED
5 years ago
4 years ago

People

(Reporter: d0kt0r1, Assigned: d0kt0r1)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130627161625

Steps to reproduce:

There are some places where the permission isn't preloaded and the return value isn't checked. 



Expected results:

TestPermissionFromPrincipal should return NS_OK. aPermission default value should be set to DENY_ACTION.
I'm trying to get you editbugs permission, which will allow you to assign this bug to yourself.  But in the meantime, feel free to work on it.
Okay, you should have editbugs permission now.
(Assignee)

Updated

5 years ago
Assignee: nobody → genti.tola
(Assignee)

Comment 3

5 years ago
Created attachment 785744 [details] [diff] [review]
bug-899495-fix.patch
Comment on attachment 785744 [details] [diff] [review]
bug-899495-fix.patch

A few comments:

* One of the more unfriendly things about contributing to Mozilla is that you need to mark a patch for review? or feedback? of a specific person (e.g. me), otherwise your patch will just be overlooked.

* If you have

  rv = Foo();
  NS_ENSURE_SUCCESS(rv, rv);

and you get rid of the second line, you should also change the first line into

  Foo();

* I think it would be better to adopt idiom (2) from bug 792178; that is, to do

    uint32_t permission = DENY_ACTION;
    TestPermissionFromPrincipal(..., &permission);

It can technically be safe if you don't have the "= DENY_ACTION" (or UNKNOWN_ACTION) there, but XPCOM calling conventions dictate that you should either assign all outparams a value before calling the fn, or check the rv.  So we should do one or the other.

* We prefer early returns to if/else.  So you'll need to change the changes made in TestExactPermanentPermission, and probably elsewhere too.
(Assignee)

Comment 5

5 years ago
Thank you for the feedback Justin.
One quick question, what is the motivation behind the early return? Is it because of the increase of the indentation level?
> One quick question, what is the motivation behind the early return? Is it because of the increase of 
> the indentation level?

Yes, and also just stylistic consistency (i.e., we had to pick one or the other way of doing it).
(Assignee)

Comment 7

5 years ago
Created attachment 786179 [details] [diff] [review]
bug-899495-fix-v2.patch

A reviewed version of the previous patch
Attachment #785744 - Attachment is obsolete: true
Attachment #786179 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 8

5 years ago
Justin, I've got something else to bug you :)

In order to see if I produced build error free code I just ran the just-print-mozilla-build.py. Is that the right tool in this case?

Moreover, I did not run any tests. Are there any general sanity checks tests that you have to run each time you make changes to different files in m-c?
> I just ran the just-print-mozilla-build.py. Is that the right tool in this case?

I've never used eclipse, and I don't quite understand what that script does.  It looks like the script doesn't actually build anything, so you probably should do a build.  I can't help a lot more, but jwatt probably can; you can find him on IRC.

What I'll do is push your changes to tryserver.  That will run (a lot of) tests, and will compile the changes on all our platforms.
I tried to apply this patch so I could push to tryserver, but I got merge conflicts.

I think you'll need to update your tree and try again.  Sorry about that.

patching file dom/devicestorage/nsDeviceStorage.cpp
Hunk #1 FAILED at 2456
1 out of 1 hunks FAILED -- saving rejects to file dom/devicestorage/nsDeviceStorage.cpp.rej
unable to find 'security/manager/boot/src/nsStrictTransportSecurityService.cpp' for patching
3 out of 3 hunks FAILED -- saving rejects to file security/manager/boot/src/nsStrictTransportSecurityService.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
Attachment #786179 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 11

5 years ago
Created attachment 786807 [details] [diff] [review]
bug-899495-fix-v3.patch

Reupdated the patch to resolve the conflicts
Attachment #786179 - Attachment is obsolete: true
Attachment #786807 - Flags: review?(justin.lebar+bug)
Okay, I've pushed this to try.  (Sorry for the delay, I was out for a few days.)

You can see the results here.  Expect there to be a few test failures (oranges); it takes some practice to tell what's a legitimate failure and what is a known intermittent.

https://tbpl.mozilla.org/?tree=Try&rev=97c213214ff0
For my reference, here's the build this try push is based off: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2f7112835ad1
Comment on attachment 786807 [details] [diff] [review]
bug-899495-fix-v3.patch

This looks really good.  Mostly I have some minor cosmetic nits, but there also
is one more-important issue below.

First, some overall questions:

* This patch changes TestPermissionForPrincipal and a lot of other methods,
  such as TestExactPermissionFromPrincipal, to be infallible (i.e. never return
  anything other than NS_OK), but the patch doesn't chave callers of these
  other methods.

  Is that something you also want to change?  You don't have to do it in this
  patch (or at all, for that matter).

* Can you please change nsIPermissionManager.idl to indicate which methods
  never throw an exception (i.e., which ones always return NS_OK)?

># HG changeset patch
># User "Genti Tola <genti.tola@live.com>"
># Date 1375865560 -7200
># Node ID b314dcb01597ae3f7f576849559ea415605a34e2
># Parent  47bd850cb89b3cd03b180d75fba6670ed40701a4
>Fix Test* methods and their invocation pattern

When you eventually attach a patch for checkin, you'll need to add "Bug XXX - "
to the beginning of the commit message and "r=jlebar" to the end.

Also, maybe you can be a bit more specific about what you're changing; there
are lots of methods which match "Test*".

>diff --git a/content/base/src/nsObjectLoadingContent.cpp b/content/base/src/nsObjectLoadingContent.cpp
>--- a/content/base/src/nsObjectLoadingContent.cpp
>+++ b/content/base/src/nsObjectLoadingContent.cpp

>@@ -3024,21 +3024,20 @@ nsObjectLoadingContent::ShouldPlay(Fallb
>   // that maintains current behavior and we have tests that expect this.
>   // What we really should do is disable plugins entirely in pages that use
>   // the system principal, i.e. in chrome pages. That way the click-to-play
>   // code here wouldn't matter at all. Bug 775301 is tracking this.
>   if (!nsContentUtils::IsSystemPrincipal(topDoc->NodePrincipal())) {
>     nsAutoCString permissionString;
>     rv = pluginHost->GetPermissionStringForType(mContentType, permissionString);
>     NS_ENSURE_SUCCESS(rv, false);
>-    uint32_t permission;
>-    rv = permissionManager->TestPermissionFromPrincipal(topDoc->NodePrincipal(),
>+    uint32_t permission = nsIPermissionManager::DENY_ACTION;
>+    permissionManager->TestPermissionFromPrincipal(topDoc->NodePrincipal(),
>                                                         permissionString.Data(),
>                                                         &permission);

Nit: Fix indentation.

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>--- a/dom/ipc/ContentParent.cpp
>+++ b/dom/ipc/ContentParent.cpp

>@@ -2435,20 +2435,18 @@ bool
> ContentParent::RecvTestPermissionFromPrincipal(const IPC::Principal& aPrincipal,
>                                                const nsCString& aType,
>                                                uint32_t* permission)
> {
>     nsCOMPtr<nsIPermissionManager> permissionManager =
>         do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
>     NS_ENSURE_TRUE(permissionManager, false);
> 
>-    nsresult rv = permissionManager->TestPermissionFromPrincipal(aPrincipal,
>-                                                                 aType.get(),
>-                                                                 permission);
>-    NS_ENSURE_SUCCESS(rv, false);
>+    permissionManager->TestPermissionFromPrincipal(aPrincipal,aType.get(),
>+                                                   permission);

Nit: Space after the first ",".

>diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp
>--- a/dom/workers/WorkerPrivate.cpp
>+++ b/dom/workers/WorkerPrivate.cpp

>@@ -3272,20 +3272,20 @@ WorkerPrivate::CheckXHRParamsAllowed(nsP
>-  uint32_t permission;
>-  nsresult rv = permMgr->TestPermissionFromPrincipal(doc->NodePrincipal(),
>-                                                     "systemXHR", &permission);
>-  if (NS_FAILED(rv) || permission != nsIPermissionManager::ALLOW_ACTION) {
>+  uint32_t permission = nsIPermissionManager::DENY_ACTION;
>+  permMgr->TestPermissionFromPrincipal(doc->NodePrincipal(),"systemXHR",
>+                                        &permission);

Nit: Indentation, space after first comma.

>+  if (permission != nsIPermissionManager::ALLOW_ACTION) {
>     return false;
>   }
> 
>   return true;
> }

>diff --git a/extensions/cookie/nsPermissionManager.cpp b/extensions/cookie/nsPermissionManager.cpp
>--- a/extensions/cookie/nsPermissionManager.cpp
>+++ b/extensions/cookie/nsPermissionManager.cpp

>@@ -942,39 +942,45 @@ nsPermissionManager::TestExactPermission
> 
> NS_IMETHODIMP
> nsPermissionManager::TestExactPermissionFromPrincipal(nsIPrincipal* aPrincipal,
>                                                       const char* aType,
>                                                       uint32_t* aPermission)
> {
>   NS_ENSURE_ARG_POINTER(aPrincipal);
> 
>+  *aPermission = nsIPermissionManager::DENY_ACTION;
>+
>   if (nsContentUtils::IsSystemPrincipal(aPrincipal)) {
>     *aPermission = nsIPermissionManager::ALLOW_ACTION;
>     return NS_OK;
>   }
> 
>-  return CommonTestPermission(aPrincipal, aType, aPermission, true, true);
>+  CommonTestPermission(aPrincipal, aType, aPermission, true, true);
>+  return NS_OK;
> }

Can we make CommonTestPermission return void?

This is also a bit funky in that CommonTestPermission sets the permission to
UNKNOWN_ACTION, not DENY_ACTION.  Can you look through the hg blame to see who
wrote that?  We should ask them if switching this to DENY_ACTION is reasonable;
I think it probably is.

>diff --git a/extensions/cookie/nsPopupWindowManager.cpp b/extensions/cookie/nsPopupWindowManager.cpp
>--- a/extensions/cookie/nsPopupWindowManager.cpp
>+++ b/extensions/cookie/nsPopupWindowManager.cpp

>@@ -64,27 +64,26 @@ nsPopupWindowManager::Init()
> 
> NS_IMETHODIMP
> nsPopupWindowManager::TestPermission(nsIPrincipal* aPrincipal,
>                                      uint32_t *aPermission)
> {
>   NS_ENSURE_ARG_POINTER(aPrincipal);
>   NS_ENSURE_ARG_POINTER(aPermission);
> 
>-  uint32_t permit;
>+  uint32_t permit = nsIPermissionManager::DENY_ACTION;
>   *aPermission = mPolicy;
> 
>   if (mPermissionManager) {
>-    if (NS_SUCCEEDED(mPermissionManager->TestPermissionFromPrincipal(aPrincipal, "popup", &permit))) {
>-      // Share some constants between interfaces?
>-      if (permit == nsIPermissionManager::ALLOW_ACTION) {
>-        *aPermission = ALLOW_POPUP;
>-      } else if (permit == nsIPermissionManager::DENY_ACTION) {
>-        *aPermission = DENY_POPUP;
>-      }
>+    mPermissionManager->TestPermissionFromPrincipal(aPrincipal, "popup", &permit);
>+    // Share some constants between interfaces?
>+    if (permit == nsIPermissionManager::ALLOW_ACTION) {
>+      *aPermission = ALLOW_POPUP;
>+    } else if (permit == nsIPermissionManager::DENY_ACTION) {
>+      *aPermission = DENY_POPUP;
>     }
>   }
> 
>   return NS_OK;
> }

It's not your fault that it was wrong before, but please move |permit| into the
|if| statement while you're here.
Attachment #786807 - Flags: review?(justin.lebar+bug) → feedback+
(Assignee)

Comment 15

5 years ago
Thank you for the detailed answer Justin. Also, sorry for my late reply as I was on holidays and did not leave any message. By the way, is there any way to specify the availability status on Bugzilla?

I have a few comments also:

> * This patch changes TestPermissionForPrincipal and a lot of other methods,
>   such as TestExactPermissionFromPrincipal, to be infallible (i.e. never
> return
>   anything other than NS_OK), but the patch doesn't chave callers of these
>   other methods.
what do you mean by "chave callers of these methods"? In fact I modified the callers not to check for the return value at any case any of these methods were called.

> 
>   Is that something you also want to change?  You don't have to do it in this
>   patch (or at all, for that matter).
> 
> * Can you please change nsIPermissionManager.idl to indicate which methods
>   never throw an exception (i.e., which ones always return NS_OK)?
I'm getting a little bit confused by the exception throwing mechanism. Does a method that always returns NS_OK mean that it never throws an error? For instance:

NS_IMETHODIMP
nsPermissionManager::TestPermission(nsIURI     *aURI,
                                    const char *aType,
                                    uint32_t   *aPermission)
{
  nsCOMPtr<nsIPrincipal> principal;
  nsresult rv = GetPrincipal(aURI, getter_AddRefs(principal));
  NS_ENSURE_SUCCESS(rv, rv);

  return TestPermissionFromPrincipal(principal, aType, aPermission);
}

Even though it returns NS_OK(because TestPermissionFromPrincipal always returns NS_OK), it might happen that NS_ENSURE_SUCCESS throws an exception? Or that's not the case?
>  By the way, is there any way to specify the availability status on Bugzilla?

People sometimes change their bugzilla username (via preferences) to indicate that they'll be away, although since you're not an employee, there's no expectation that you'll be available.

> what do you mean by "chave callers of these methods"?

Sorry, I meant *change* callers of these methods.

> In fact I modified the callers not to check for the return value at any case any of 
> these methods were called.

Hm, I was saying that I didn't think you did this.  For example, there is more than one call to TestExactPermissionFromPrincipal in my tree, but you changed only one in this patch.  Maybe my tree is newer than yours.

> Does a method that always returns NS_OK mean that it never throws an error?

Yes, but see below about implicit |return| statements.

> Even though it returns NS_OK(because TestPermissionFromPrincipal always returns 
> NS_OK), it might happen that NS_ENSURE_SUCCESS throws an exception

Yes.  NS_ENSURE_SUCCESS(x,y) is shorthand for

if (NS_FAILED(x)) { return y; }
(Assignee)

Comment 17

5 years ago
I ran hg blame to find out that Mounir(mounir@lamouri.fr) was the author of the lines that predefined UNKNOWN_ACTION as default. It's been two days since I sent him an email and I have no response. What should I do?
Let's cc him on this bug and give him a few more days.  He may be on vacation.

Note that my last day at Mozilla is on Friday; you'll need to find someone else to finish mentoring you here.  Mounir may be able to do it.
Flags: needinfo?(mounir)
It is not clear to me what this patch is actually trying to achieve. The fact that we use UNKNOWN_ACTION by default is not a bug. UNKNOWN_ACTION roughly means that the user took no decision on that matter (at least, this is the meaning of it in Firefox Desktop and Mobile). The calling site might show a UI to ask the user to whether ALLOW/DENY the action if the result in UNKNOWN_ACTION. If the result is DENY_ACTION is calling site might just assume that the user previously denied it.
Flags: needinfo?(mounir)
(Assignee)

Comment 20

5 years ago
(In reply to Mounir Lamouri (:mounir) from comment #19)
> It is not clear to me what this patch is actually trying to achieve. The
This bug is related to this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=792178
In addition to that, we are also changing the callers of these methods to meet the second option as suggested by mikeh.

> fact that we use UNKNOWN_ACTION by default is not a bug. UNKNOWN_ACTION
> roughly means that the user took no decision on that matter (at least, this
> is the meaning of it in Firefox Desktop and Mobile). The calling site might
> show a UI to ask the user to whether ALLOW/DENY the action if the result in
> UNKNOWN_ACTION. If the result is DENY_ACTION is calling site might just
> assume that the user previously denied it.
Predefining DENY_ACTION at all Test* methods, is not a must. It was suggested by Justin, to have a unified manner of calling such methods. I believe, in such case we should not change the default UNKNOWN_ACTION.(by the way, since I am a newcomer I cannot think of a real scenario of this happening, can you give me an example) This was a mentored bug and Justin(that was my mentor) left. I do not know if I should prepare another patch or not? Will you be able to push my commits than?
Flags: needinfo?(mounir)
Genti, as far as I understand it, there was a misunderstanding in bug 792178. I do not think you should set permission to DENY_ACTION but to UNKNOWN_ACTION by default. You might want to change the call sites instead of the method. In other words, you might want to fix the two places that, according to bug 792178 comment 0 are mis-using TestPermissionFromPrincipal (have not checked if that was actually a correct assessment).

You should also not that, per XPIDL convention, when you call a method, it will always set the out arguments unless there is an exception (ie. the return value is not NS_OK).
Flags: needinfo?(mounir)
(Assignee)

Comment 22

5 years ago
Created attachment 805259 [details] [diff] [review]
bug-899495-fix.patch
Attachment #786807 - Attachment is obsolete: true
Attachment #805259 - Flags: review?(mounir)
Comment on attachment 805259 [details] [diff] [review]
bug-899495-fix.patch

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

r=me with the comments fixed.

::: dom/src/storage/DOMStorage.cpp
@@ +259,1 @@
>                                                   kPermissionType, &perm);

Please, fix the indentation.

@@ +259,2 @@
>                                                   kPermissionType, &perm);
> +  NS_ENSURE_SUCCESS(rv, rv);

This method returns a boolean so this should be:
NS_ENSURE_SUCCESS(rv, false);
Attachment #805259 - Flags: review?(mounir) → review+
(Assignee)

Comment 24

4 years ago
Created attachment 814059 [details] [diff] [review]
Reviewed the patch
Attachment #805259 - Attachment is obsolete: true
Attachment #814059 - Flags: review?(mounir)
(Assignee)

Comment 25

4 years ago
In reply to Mounir Lamouri (:mounir) from comment #23)

> @@ +259,2 @@
> >                                                   kPermissionType, &perm);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> This method returns a boolean so this should be:
> NS_ENSURE_SUCCESS(rv, false);

OK, I made those changes. However, throughout the mozilla-central I can see many examples of NS_ENSURE_SUCCES(rv,rv). 
Can you tell me what's the difference between them? 
Any other information to better understand this error handling system would be more than welcome!
Flags: needinfo?(mounir)
(In reply to Genti Tola [:d0kt0r1] from comment #25)
> In reply to Mounir Lamouri (:mounir) from comment #23)
> 
> > @@ +259,2 @@
> > >                                                   kPermissionType, &perm);
> > > +  NS_ENSURE_SUCCESS(rv, rv);
> > 
> > This method returns a boolean so this should be:
> > NS_ENSURE_SUCCESS(rv, false);
> 
> OK, I made those changes. However, throughout the mozilla-central I can see
> many examples of NS_ENSURE_SUCCES(rv,rv). 
> Can you tell me what's the difference between them? 
> Any other information to better understand this error handling system would
> be more than welcome!

rv is of type nsresult but here, the method returns a bool if I remember correctly. The conversion from nsresult to bool might not work exactly as you want.
Flags: needinfo?(mounir)
Attachment #814059 - Flags: review?(mounir) → review+
Status: UNCONFIRMED → ASSIGNED
Component: General → General
Ever confirmed: true
Keywords: checkin-needed
Product: Boot2Gecko → Core
Version: unspecified → Trunk
(Assignee)

Comment 29

4 years ago
Mounir, does this come from the change that you proposed about the usage of NS_ENSURE_SUCCESS?(see https://tbpl.mozilla.org/php/getParsedLog.php?id=28873468&tree=Mozilla-Inbound)
I still do not understand the proper usage of this macro. Can anyone give me some hints? By looking through its implementation I can see that the macro returns if something fails, but I do not understand what happens to the control afterwards? Also, why at some case it is used like NS_ENSURE_SUCCESS(rv,true/false) and at other cases NS_ENSURE_SUCCESS(rv,rv)
Flags: needinfo?(mounir)
Genti, the following macro NS_ENSURE_SUCCESS(_test, _return) is equivalent to:
if (NS_FAILED(_test)) {
  return _return;
}
Flags: needinfo?(mounir)
You need to log in before you can comment on or make changes to this bug.