564535 - permission manager needs to be remoted

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Doug Turner (:dougt)]

we use the permission manager in both chrome and content.  it would be good to have the data in sync.

Comment 1

[Doug Turner (:dougt)]

bsmedberg and I discussed this.  It isn't exactly clear why a child process needs access to the permission manager.  Before moving forward with any work, we probably want to audit users.

Comment 2

[Mike Kristoffersen (:MikeK)]

fwiw - both the main (fennec) process and plugin-container are using nsContentBlocker that uses the permission manager - I can't jugde if this is fair use?  Or am I completely missing the point of the previous comment?

Comment 3

[Doug Turner (:dougt)]

ah.  Either remote nsContentBlocker or permissions.

Comment 4

[Benjamin Smedberg]

Yeah, sounds better to remote pieces of nsContentBlocker to me, although I'm not clear on exactly what it does.

Comment 5

[Mike Kristoffersen (:MikeK)]

Trying to do a little writeup on the different solution models making it more easy to evaluate which one will be best, stay tuned for that... 

One interesting thing that I have noted is that each element that is checked, seems to be checked at least one time from both the child process and from the main process - I have an example where its checked once from the child process and 3 times from the main process.  

Among the things I'm looking into, is if we can remove the check in the child process, which might simplify things and speed execution up...

Comment 6

[Mike Kristoffersen (:MikeK)]

The solution models as I see them:

1)
Keep one instance of the permission manager in each process, but keep the data base in sync between the processes.
This will have minimal code impact, but take a bit more resources runtime - the synchronization will be asynchronous and hence not instant between the processes, but I think the delay will be short enough as to not impact the user.

2) 
Eliminate the need for having the permission manager in the child process, we are currently doing the check in both the main and child process, this seems like doing double work.  This will probably not be a simple change - but at this time I don't have the complete overview to really judge the impact of this change.

3)
Use synchronous or rpc from the permission manager in the child process to access the database in the main process - this will keep all the interfaces intact and have low impact but doesn't give us the full benefit of having a child process (as it will be blocked when waiting for the database access)

4) 
Create an asynchronous interface on the permission manager (remote part of the permission manager) - this will have code impact on the users of the permission manager - but is probably the way we would have done it, if we designed from scratch.

5) 
Only have one instance of the permission manager in the main process, but instead "remote pieces of nsContentBlocker" if we do that asynchronous then the interface to nsContentBlocker will also have to be asynchronous, meaning the users of it will have to be changed.

6) 
Same as 5) but with synchronous messages or rpc - meaning no interface changes, but same problem as with 3).

Without having the complete understanding of the code yet, my initial thought is to go for 1) or 2), assuming we don't like anything synchronous between the processes, but would really like some feedback on it...

That being said, I'll not be able to do anymore on this until next week.

Comment 7

[Josh Matthews [:jdm]]

Synchronous messaging from child -> parent is allowed, and usually easier.

Comment 8

[Mike Kristoffersen (:MikeK)]

(In reply to comment #7)
> Synchronous messaging from child -> parent is allowed, and usually easier.

If that is not something that we should try to avoid, then I guess its a question of choosing between 2) and 6) ?

Comment 9

[Alon Zakai (:azakai)]

It looks like the permission manager is definitely used in the child process by

* nsContentBlocker.cpp
* nsOfflineCacheUpdate.cpp
* nsPopupWindowManager.cpp

It is also referred to in the following file, but does not seem to be used in the child process (although, perhaps my tests were not exhaustive),

* nsCookiePermission.cpp

and is referred to, but does not seem to be used in either parent or child process (but again, perhaps my tests were not exhaustive), by

* nsDOMStorage.cpp

So given that, perhaps the simplest option is 3 (synchronously remote the permission manager calls from child to parent)?

Comment 10

[Doug Turner (:dougt)]

what of enumeration?  does that have to be remoted too?

Comment 11

[Alon Zakai (:azakai)]

(In reply to comment #10)
> what of enumeration?  does that have to be remoted too?

I did some tests on that now, but again, I cannot be sure they are 100% exhaustive. With that disclaimer, it looks like the enumeration is used in the parent, and not in the child.

Comment 12

[[e-mail]]

OK, let's remote it. I think we should be able to get away with just remoting the Test*Permission methods, not the mutation method(s).

Shouldn't need to remote the enumerator. It's used for UI and perhaps extension purposes.

Comment 14

[Alon Zakai (:azakai)]

Code to fix this bug now appears in bug 576161. That bug suggests a new ServiceCaller service which would make remoting services lik the permissions manager much easier (just a few lines of code - no new files, no new IPDL protocols, etc.).

The patch in that bug includes working code to remote the permissions manager, as a concrete example of the motivation for the ServiceCaller. So, this bug can be closed as soon as that bug is.

Comment 15

[Alon Zakai (:azakai)]

Attachment: Patch

Since the other bug I mentioned earlier might be controversial, in order to not delay this bug from being resolved, here is a patch.

Permission checks are remoted; other operations in the child process are guarded against.

Comment 16

[[e-mail]]

Comment on attachment 455764 [details] [diff] [review]
Patch

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

>+#ifdef MOZ_IPC
>+PRBool IsChildProcess()

Declare 'static' please.

>+/**
>+ * @returns The child process object, or if we are not in the child
>+ *          process, nsnull.
>+ */
>+mozilla::dom::ContentProcessChild* ChildProcess()

'static'

>+{
>+  if (IsChildProcess()) {
>+    mozilla::dom::ContentProcessChild* cpc =
>+      mozilla::dom::ContentProcessChild::GetSingleton();
>+    NS_ASSERTION(cpc, "Content Process is NULL!");

This should NS_RUNTIMEABORT.

>+    return cpc;
>+  } else {

No need for 'else' after 'return'.

>+#define ENSURE_NOT_CHILD_PROCESS \
>+NS_ASSERTION(!IsChildProcess(), "Should not be called in the child process!");

This needs to do more than assert. You could make it:

  PR_BEGIN_MACRO \
  if (IsChildProcess()) { \
    NS_ERROR("cannot set pref from content process"); \
    return NS_ERROR_NOT_AVAILABLE; \
  } \
  PR_END_MACRO

>@@ -534,7 +574,14 @@
>                                          const char *aType,
>                                          PRUint32   *aPermission)
> {
>-  return CommonTestPermission(aURI, aType, aPermission, PR_TRUE);
>+#ifdef MOZ_IPC
>+  mozilla::dom::ContentProcessChild* cpc = ChildProcess();
>+  if (cpc) {
>+    return cpc->SendTestPermission(IPC::URI(aURI), nsCString(aType), PR_TRUE,

nsDependentCString(aType)

>+      aPermission) ? NS_OK : NS_ERROR_FAILURE;
>+  } else

No need for the 'else' after 'return'.

>+#endif
>+    return CommonTestPermission(aURI, aType, aPermission, PR_TRUE);
> }

>@@ -542,7 +589,14 @@
>                                     const char *aType,
>                                     PRUint32   *aPermission)
> {
>-  return CommonTestPermission(aURI, aType, aPermission, PR_FALSE);
>+#ifdef MOZ_IPC
>+  mozilla::dom::ContentProcessChild* cpc = ChildProcess();
>+  if (cpc) {
>+    return cpc->SendTestPermission(IPC::URI(aURI), nsCString(aType), PR_FALSE,
>+      aPermission) ? NS_OK : NS_ERROR_FAILURE;
>+  } else
>+#endif
>+    return CommonTestPermission(aURI, aType, aPermission, PR_FALSE);

Same comments as above.

In nsPermissionManager::Init(), you should test whether we're in the content process. If so, return right after the 'mHostTable.Init()' part.

You should add an ENSURE_NOT_CHILD_PROCESS to nsPermissionManager::Observe(), in case someone tries to manually call it.

r=dwitte with changes.

Comment 17

[Alon Zakai (:azakai)]

Attachment: Patch v2

Fixed version after comments.

Comment 18

[Josh Matthews [:jdm]]

>#include "mozilla/dom/ContentProcessChild.h"

This needs to be in a MOZ_IPC block.

Comment 19

[Alon Zakai (:azakai)]

Attachment: Patch v3

(In reply to comment #18)
> >#include "mozilla/dom/ContentProcessChild.h"
> 
> This needs to be in a MOZ_IPC block.

Good catch, fixed.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Is this ready to land?

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 455883 [details] [diff] [review]
Patch v3

moving dwitte's r+ forward

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mozilla-central/rev/9944d9dd6aaf

Comment 23

[Philip Chee]

This appears to have broken SeaMonkey: Bug 579747 Comment 0:

> Error: Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER)
> [nsIPermissionManager.add]
> Source file: chrome://navigator/content/pageinfo/permissions.js
> Line: 175
> 
> 
> Line 175:
> 
>   permissionManager.add(gPermURI, aPartId, permission);

Is there something we need to do on our side of things?

Comment 24

[Josh Matthews [:jdm]]

This patch shouldn't have caused any problems like this.  I assume that Seamonkey isn't using a content process, so these changes won't affect any of the regular code path.  Do you know if Seamonkey builds with --enable-ipc?

Comment 25

[Philip Chee]

> This patch shouldn't have caused any problems like this.  I assume that
> Seamonkey isn't using a content process, so these changes won't affect any of
> the regular code path.

Reading through the patch here I thought so too but this is the most likely bug in the pushlog in the correct time frame.

> Do you know if Seamonkey builds with --enable-ipc?
Nope. We can't build with ipc at the moment without significant changes in the shared code with Thunderbird.

Comment 26

[Josh Matthews [:jdm]]

If that's the case, this patch can't be at fault.  Every change is hidden behind MOZ_IPC, which is not defined without --enable-ipc.

Comment 27

[Philip Chee]

Thanks. Removing dependency.

p.s. any likely changes in mozilla-central I should be looking at?

Comment 28

[azrdev]

This issue is fixed, does this mean there is now a javascript API (in the add-on SDK) replacing XPCOM nsIPermissionManager access through chrome?