599428 - Optimize Permissions Messaging

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Alon Zakai (:azakai)]

Attachment: Profiling of messages from child process

Profiling in bug 588050 shows that permissions sync messages account for 5-15% of latency. In the attachment you can visually see that 2 such messages add about 0.75 seconds in that particular run. (Red are permissions, X axis is time, each grey line is 0.5 second, Y axis is messages, one message per line.)

We should find a way to improve this.

Comment 1

[Alon Zakai (:azakai)]

Attachment: Cache duplicates

Not sure this is a correct approach, since I'm not sure how often permission tests need to be re-done. The patch caches all requests in the child, so it does not send duplicate requests to the parent at all.

This saves 46% of the permissions IPC messages, and reduces latency from 0.52 seconds to 0.30 seconds (out of 5.75 seconds to load cnn.com).

Comment 2

[[e-mail]]

OK, so. I think we should follow a model similar to prefs here: push the permission set over to the child and then allow asynchronous updates to it as things change on the parent. Caching would also work fine, provided we have an observer mechanism in place.

I think dougt was talking about having a sqlite reader in the child. That might work, too, though waiting for disk I/O before the data shows up in the child might add a lot of latency to when the change is reflected. And it wouldn't help for the case where a permission was updated in the parent and has to be pushed across -- the child would have to query from the db every time, which sucks.

So I'm fine with the caching here, but we need to add an observer mechanism.

Comment 3

[Alon Zakai (:azakai)]

(In reply to comment #2)
> OK, so. I think we should follow a model similar to prefs here: push the
> permission set over to the child and then allow asynchronous updates to it as
> things change on the parent.

Sounds good, I'm curious though what kind of numbers we expect here. How many permissions would we be sending at startup, and how often are they updated? For example cnn.com seems to have > 100 unique permission checks during load, if a similar amount is saved for multiple websites, and it's all saved between sessions, then it sounds like it could be a lot?

Comment 4

[[e-mail]]

Well... that's a pretty hard-to-quantify question. As you say, for a given pageload we'll need to test against many different hosts. We have a few cases:

1) The user has no permissions, so all the tests return default.
2) The user has a permission for the root domain, so all the tests return that value.
3) The user has many permissions for subdomains of the root, so the tests return a variety of values.

1 and 2 would argue for caching the permission(s) that are actually responsible for the results of the various tests -- for instance, if we query www.cnn.com and the parent finds a permission for *.cnn.com, then we send *.cnn.com back to the child so it can obtain all the necessary results from it. But this gets tricky pretty fast.

3 would argue for serializing the entire list on child creation.

I think it's simpler to do the entire list, just like prefs.

Comment 5

[[e-mail]]

As for updates, they could occur at any time, but in general should be rare. For instance, user clicks "page info" and "block images from this domain", "block cookies" etc. Or an addon adds a ton of permission entries for various sites, a la Adblock Plus. Or cookie prompting results in a whole ton of permissions being created per pageload.

Comment 6

[Alon Zakai (:azakai)]

How long are permissions retained? (between sessions? forever?)

Comment 7

[[e-mail]]

Generally forever.

Comment 8

[Alon Zakai (:azakai)]

Attachment: Read sqlite in child on load, get ipc updates later

WIP patch (buggy/incomplete). The child process reads the SQLite DB on load (like the parent), and requests the parent to send it updates on further changes to permissions (to keep it in sync). This avoid inventing a serialization format for the permissions and the use of IPC. Does that approach make sense?

Side question, what's a simple way to test permissions (that is, what's an easy way to modify a permission, reset it later, etc.)?

Comment 9

[[e-mail]]

For 5-15%, we should block beta2 on this. (Precedent: bug 599545.)

Comment 10

[[e-mail]]

Comment on attachment 478920 [details] [diff] [review]
Read sqlite in child on load, get ipc updates later

So, reading the db in the child won't work. We have session-only permissions that never hit the disk.

This needs to be done using a pref-like approach: serialize the whole thing across to the child, and notify it of updates. I put some suggestions on this for prefs in https://bugzilla.mozilla.org/show_bug.cgi?id=599545#c4; I think the same applies here.

Comment 11

[Alon Zakai (:azakai)]

Attachment: Patch v2

Ok, I hope this is the proper way to do it...

Comment 12

[Alon Zakai (:azakai)]

Attachment: Tests

Tests for patch v2.

Comment 13

[[e-mail]]

Comment on attachment 480315 [details] [diff] [review]
Patch v2

>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp

>+bool
>+ContentChild::RecvAddPermissions(const nsTArray<IPC::Permission>& permissions)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIPermissionManager> pm = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
>+  NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), 
>+                   "We have no permissionManager in the Content process !");
>+
>+  for (int i = 0; i < permissions.Length(); i++) {

PRUint32 here.

>+    const IPC::Permission &perm = permissions[i];
>+    nsCOMPtr<nsIURI> uri;
>+    nsCOMPtr<nsIIOService> io = mozilla::services::GetIOService();
>+    nsCString host;
>+    host.Append("http://");
>+    host.Append(perm.host);
>+    nsresult rv = io->NewURI(host, nsnull, nsnull, getter_AddRefs(uri));

Hmm, this won't be cheap at all. And the permissionmanager API sucks for this kind of thing. :(

Let's do it this way instead. Make AddInternal be a public method on nsPermissionManager. Then add an XPCOM getter that bypasses GetService and returns an nsPermissionManager* directly. (For example, see http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/CookieServiceParent.cpp#52 and http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#574). Then, just call AddInternal directly and give it the raw strings from your IPC::Permission.

>+    NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "Must have valid urls for permissions");
>+    pm->Add(uri, perm.type.get(), perm.capability, perm.expireType, perm.expireTime);
>+  }
>+  return true;
>+}

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

>+bool
>+ContentParent::RecvReadPermissions(nsTArray<IPC::Permission>* aPermissions)
> {

>+        if (!hasMore)
>+            break;
>+        nsPermission *perm;
>+        enumerator->GetNext((nsISupports**)&perm);

You should QI to nsIPermission here, not cast to nsPermission.

>+        aPermissions->AppendElement(IPC::Permission(host, type, capability, expireType, expireTime));
>     }
>+
>+    // Ask for future changes
>+    permissionService->ChildRequestPermissions();

Hmm. Not exactly elegant, but OK. Stick this as a public method on nsPermissionManager, similar to above?

>diff --git a/dom/ipc/Makefile.in b/dom/ipc/Makefile.in

>+		-I$(topsrcdir)/extensions/cookie \

Heh, extensions/cookie is not actually an extension anymore, and is built way above the dom tier. This surprised me. :)

All the dom/ code, however, will break if MOZ_PERMISSIONS is not set. You should wrap the relevant direct uses of nsPermissionManager.h with an ifdef, and have them be nops otherwise. Should just be a few places.

>diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl

>@@ -76,33 +78,33 @@ child:

>+    // PermissionsManager messages

'nsIPermissionManager'?

>+    AddPermissions(Permission[] permissions);

>@@ -110,15 +112,18 @@ parent:

>+    // PermissionsManager messages

'nsIPermissionManager'.

>+    sync ReadPermissions() returns (Permission[] permissions);

>diff --git a/extensions/cookie/PermissionsMessageUtils.h b/extensions/cookie/PermissionsMessageUtils.h

I'd just stick all this code in NeckoMessageUtils.h; nsIPermissionManager lives there anyway.

>+struct Permission
>+{
>+  nsCString host, type;
>+  PRUint32 capability, expireType;
>+  PRInt64 expireTime;
>+
>+  Permission() { }
>+  Permission(nsCString aHost,
>+             nsCString aType,

References here.

>+  static bool Read(const Message* aMsg, void** aIter, Permission* aResult)
>+  {
>+    if (!ReadParam(aMsg, aIter, &aResult->host))
>+      return false;
>+    if (!ReadParam(aMsg, aIter, &aResult->type))
>+      return false;
>+    if (!ReadParam(aMsg, aIter, &aResult->capability))
>+      return false;
>+    if (!ReadParam(aMsg, aIter, &aResult->expireType))
>+      return false;
>+    if (!ReadParam(aMsg, aIter, &aResult->expireTime))
>+      return false;

Can chain all these together and just say 'return ReadParam(...) && ReadParam(...)'.

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

> #define ENSURE_NOT_CHILD_PROCESS \
>   PR_BEGIN_MACRO \
>   if (IsChildProcess()) { \
>-    NS_ERROR("cannot set permission from content process"); \
>-    return NS_ERROR_NOT_AVAILABLE; \
>+    NS_RUNTIMEABORT("Cannot perform action in content process"); \

Is there a particular reason for this change?

>@@ -157,19 +177,37 @@ nsPermissionManager::Init()

> #ifdef MOZ_IPC
>-  // Child will route messages to parent, so no need for further initialization
>-  if (IsChildProcess())
>+  if (IsChildProcess()) {
>+    // Do this before we read the permissions DB, so we get all required
>+    // updates.

Comment is stale -- we're not reading in the DB at all.

>+    nsTArray<IPC::Permission> perms;
>+    ChildProcess()->SendReadPermissions(&perms);
>+
>+    nsCOMPtr<nsIIOService> io = mozilla::services::GetIOService();
>+    for (int i = 0; i < perms.Length(); i++) {
>+      const IPC::Permission &perm = perms[i];
>+      nsCOMPtr<nsIURI> uri;
>+      nsCString host;
>+      host.Append("http://");
>+      host.Append(perm.host);
>+      nsresult rv = io->NewURI(host, nsnull, nsnull, getter_AddRefs(uri));
>+      NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "Must have valid urls for permissions");
>+      Add(uri, perm.type.get(), perm.capability, perm.expireType, perm.expireTime);
>+    }

You should be doing basically the same thing as in ContentChild::RecvAddPermissions() here -- nice and direct.

> NS_IMETHODIMP
> nsPermissionManager::Add(nsIURI     *aURI,
>                          const char *aType,
>                          PRUint32    aPermission,
>                          PRUint32    aExpireType,
>                          PRInt64     aExpireTime)
> {
>-#ifdef MOZ_IPC
>-  ENSURE_NOT_CHILD_PROCESS;
>-#endif

This means that code in the child can call Add(), which will call through to AddInternal(), which will then happily write the addition to the child's table but not tell the parent. This doesn't sound good.

If you call AddInternal directly everywhere else, you should be able to leave this in.

>@@ -394,16 +428,32 @@ nsPermissionManager::AddInternal(const n

>+#ifdef MOZ_IPC
>+  if (IsChildProcess())
>+    // When updates arrive, we will get Add() commands, which by default
>+    // ask to update the DB. In the child process this is of course unnecessary.
>+    aDBOperation = eNoDBOperation;

You shouldn't need this piece of code if you call AddInternal directly from ContentChild::RecvAddPermissions, since you can pass eNoDBOperation as an arg.

>+  else {
>+    // In the parent, send the update now, if the child is ready
>+    if (mUpdateChildProcess) {
>+      nsTArray<IPC::Permission> additions;

Just send a single IPC::Permission rather than an array, and have the receiver do likewise?

>+NS_IMETHODIMP
>+nsPermissionManager::ChildRequestPermissions()
>+{
>+  mUpdateChildProcess = PR_TRUE;
>+  return NS_OK;
>+}

>diff --git a/netwerk/base/public/nsIPermissionManager.idl b/netwerk/base/public/nsIPermissionManager.idl

>+  /**
>+   * Causes the parent process to start sending updates to the child process,
>+   * whenever in the future there is a change to a permission.
>+   */
>+  void childRequestPermissions();

We don't want to change this interface at this point in Firefox 4, and especially not without revving the uuid. ;)

Want to see another patch.

Comment 14

[Alon Zakai (:azakai)]

Attachment: Patch v3

New patch, fixing the issues mentioned. Except for:

> 
> All the dom/ code, however, will break if MOZ_PERMISSIONS is not set. You
> should wrap the relevant direct uses of nsPermissionManager.h with an ifdef,
> and have them be nops otherwise. Should just be a few places.

Hmm, MOZ_PERMISSIONS is not set, as far as I can tell. I added commented-out ifdefs in the patch so it would work. What am I missing here...?

> 
> > #define ENSURE_NOT_CHILD_PROCESS \
> >   PR_BEGIN_MACRO \
> >   if (IsChildProcess()) { \
> >-    NS_ERROR("cannot set permission from content process"); \
> >-    return NS_ERROR_NOT_AVAILABLE; \
> >+    NS_RUNTIMEABORT("Cannot perform action in content process"); \
> 
> Is there a particular reason for this change?

Just that it should really halt and fail, not only give a warning, I think. Or not?

Comment 15

[[e-mail]]

(In reply to comment #14)

> Hmm, MOZ_PERMISSIONS is not set, as far as I can tell. I added commented-out
> ifdefs in the patch so it would work. What am I missing here...?

http://mxr.mozilla.org/mozilla-central/search?string=MOZ_PERMISSIONS

It's apparently added to DEFINES here: http://mxr.mozilla.org/mozilla-central/source/toolkit/library/libxul-config.mk#225

which will set it for compilation of the relevant source file (in this case, toolkit/library/nsStaticXULComponents.cpp). What you need to do is something similar in the Makefile that builds the relevant files.

> Just that it should really halt and fail, not only give a warning, I think. Or
> not?

Well, I don't really think we want to abort the app from someone making a silly mistake. Take prefs as precedent: http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#206

r=dwitte with those fixes.

Comment 16

[[e-mail]]

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

Canceling review pending new patch.

Comment 17

[Alon Zakai (:azakai)]

Attachment: Patch v4

Fixed the RUNTIMEABORT issue, and the NS_PERMISSIONS stuff. Carried over r+.

Comment 18

[Alon Zakai (:azakai)]

http://hg.mozilla.org/mozilla-central/rev/e14e2cc7d786

Comment 19

[Rich Walsh]

The following broke my non-IPC build because |mUpdateChildProcess| is not wrapped with an #ifdef MOZ_IPC (extensions/cookie/nsPermissionManager.cpp line#175).

>  nsPermissionManager::nsPermissionManager()
>   : mLargestID(0)
> + , mUpdateChildProcess(PR_FALSE)
>  {
>  }

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> The following broke my non-IPC build

I filed bug 603139.

Please fix or backout asap!

Comment 21

[Alon Zakai (:azakai)]

Attachment: missing MOZ_IPC ifdef

Sorry about the MOZ_IPC problem.

Here is the patch to fix it. I can't push it myself right now, if someone else wants to do that go right ahead. Otherwise I'll push it as soon as I can, which will likely be tomorrow.

Comment 22

[Alon Zakai (:azakai)]

Found a way to have the opportunity to push the MOZ_IPC fix today,

http://hg.mozilla.org/mozilla-central/rev/ab6e1a17eb09