811026 - [Permissions] Grant a prompted permission for the duration of the session.

Status: VERIFIED FIXED

(Core :: Permission Manager, defect, P1)

Description

[Gregor Wagner [:gwagner]]

Currently we need 2 roundtrips to the parent to prompt for a permission and actually perform the task. 
We have to remember the decision of the user in order to perform the actual task in the parent. 
The idea is to grant a permission for the duration of the session.

Comment 1

[Doug Turner (:dougt)]

This is blocking because, without this, if the user does not select "remember this decision" geolocation, desktop notifications and device storage (maybe others) will not work (assuming that we actually make checks in the parent for the right permissions).

Comment 2

[Gregor Wagner [:gwagner]]

Attachment: WiP

A first draft implementation

Comment 3

[Andrew Overholt [:overholt]]

How's this going, Gregor?

Comment 4

[Gregor Wagner [:gwagner]]

(In reply to Andrew Overholt [:overholt] from comment #3)
> How's this going, Gregor?

Working on it. It's not a trivial patch.

Comment 5

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 6

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 7

[Andrew Overholt [:overholt]]

Andrea, can you take this off Gregor's plate?

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch

This patch converts any prompted permission to a session one.
When a process quits, the permission manager refreshes its list of permission for that appId.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 691794 [details] [diff] [review]
patch

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

::: b2g/components/ContentPermissionPrompt.js
@@ +17,5 @@
>  const Cr = Components.results;
>  const Cu = Components.utils;
>  const Cc = Components.classes;
>  
> +const PROMPT_FOR_UNKNOWN = [];

This change doesn't look right. Why is this no longer needed?

@@ +110,5 @@
>        if (evt.detail.type == "permission-allow") {
>          if (evt.detail.remember) {
> +          rememberPermission(request.type, request.principal, false);
> +        } else {
> +          rememberPermission(request.type, request.principal, true);

rememberPermission(request.type, request.principal, !evt.detail.remember)

::: dom/ipc/TabParent.cpp
@@ +159,5 @@
> +  nsCOMPtr<nsIPermissionManager> permMgr =
> +    do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
> +  if (permMgr) {
> +    permMgr->ExpirePermissionsForApp(OwnAppId());
> +  }

Justin should review this part to make sure that this is the only place we need this call.

::: extensions/cookie/nsPermissionManager.cpp
@@ +1228,5 @@
> +    }
> +
> +    AddInternal(principal,
> +                type,
> +                nsIPermissionManager::UNKNOWN_ACTION,

Setting it back to "unknown" here is wrong. That will cause us to not bring up a prompt next time the API is used.

The way that most of these high-privileged APIs work is that you have to enumerate a permission in the app manifest in order to be even allowed to prompt for them. For example we want to have a review of an app before we even let it prompt about using the DeviceStorage API. So PROMPT_ACTION in the database means that the app is allowed to ask the user for permission. UNKNOWN_ACTION means that the app isn't allowed to use the API at all.

So to solve this, I think you need to pass in what value to reset the permission to when it expires. And use that value both here and elsewhere in the permission manager where we deal with expired permissions.

@@ +1236,5 @@
> +                nsPermissionManager::eNotify,
> +                nsPermissionManager::eNoDBOperation);
> +  }
> +
> +  return ReadForApp(aAppId);

Oh, I think I see what you are doing here. The idea is to reload from disk when expiring an app?

We don't really want to do that though because it will cause a lot of synchronous IO to happen whenever an app is closed. Instead pass in what the permission should be reset to when it expires.

Comment 10

[Andrea Marchesini [:baku]]

> Oh, I think I see what you are doing here. The idea is to reload from disk
> when expiring an app?

Yep. What this patch does is: when an app quits any permission is removed and reloaded from the database. I'm going to upload a patch that keeps all the permission in memory as a cache.
If you don't anything against it, I would continue removing all the permission for an app and reset them completely.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch b

Comment 12

[Mounir Lamouri (:mounir)]

Comment on attachment 693397 [details] [diff] [review]
patch b

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

I would like to review that.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't see why you couldn't store the "old" (pre expire-session) information in the existing hash table.

It seems like all you'd need to do is to change the code at [1] to store the pre-expire-session permission and expiration type in the nsPermission object.

Then make the new ExpirePermissionsForApp function enumerate the hash table and for all entries with session expiration copy information from the stored away information to the one exposed, and remove the entry if the stored away information was UNKNOWN_ACTION. And manually call NotifyObserversWithPermission as appropriate.

Note that it's already not supported to store a session expire permission with UNKNOWN_ACTION. So there's no need to handle session expirations in the eOperationRemoving branch of AddInternal.

[1] http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#756

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch c

Ok, this new patch uses a cache in the PermissionEntry object.

Comment 15

[Mounir Lamouri (:mounir)]

Comment on attachment 693833 [details] [diff] [review]
patch c

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

Caching the value wouldn't work well if EXPIRE_TIME is used but given that there is only one consumer of that, we can easily consider that EXPIRE_TIME and EXPIRE_SESSION will not be used on the same permission. Adding a MOZ_ASSERT wouldn't hurt though.

I'm not a big fan that we have to explicitly call a method to purge the expired permissions when we close an application. I think it would be better to do that when we *open* the application because we know we will always open the application but we might end up not properly closing it (extreme case is if I unplug the battery).

::: dom/ipc/TabParent.cpp
@@ +158,5 @@
> +
> +  nsCOMPtr<nsIPermissionManager> permMgr =
> +    do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
> +  if (permMgr) {
> +    permMgr->ExpirePermissionsForApp(OwnAppId());

nit: !permMgr should probably NS_ERROR because that means we are going to keep the temporary permission which is against the wish of the user.

::: extensions/cookie/nsPermissionManager.cpp
@@ +1213,5 @@
>  
> +PLDHashOperator
> +nsPermissionManager::ExpirePermissionsForAppEnumerator(nsPermissionManager::PermissionHashKey* entry, void* arg)
> +{
> +  uint32_t *appId = static_cast<uint32_t*>(arg);

nit: uint32_t* appId = ...;

@@ +1225,5 @@
> +        continue;
> +      }
> +
> +      if (permEntry.mCachedPermission != permEntry.mPermission) {
> +        permEntry.mPermission = permEntry.mCachedPermission;

I'm not sure how you can go there if you call |continue| after removing the element.

@@ +1234,5 @@
> +  }
> +
> +  return PL_DHASH_NEXT;
> +}
> +NS_IMETHODIMP

nit: leave an empty line.

@@ +1238,5 @@
> +NS_IMETHODIMP
> +nsPermissionManager::ExpirePermissionsForApp(uint32_t aAppId)
> +{
> +  ENSURE_NOT_CHILD_PROCESS;
> +  NS_ENSURE_ARG(aAppId != nsIScriptSecurityManager::NO_APP_ID);

nit: MOZ_ASSERT would be appropriate with a comment saying that we shouldn't call this for something else than an app.

::: netwerk/base/public/nsIPermissionManager.idl
@@ +187,5 @@
> +
> +  /**
> +   * Expire all permissions associated with a given app id if needed.
> +   */
> +  void expirePermissionsForApp(in unsigned long appId);

I think |removeExpiredPermissionsForApp| would be a better name.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: patch d

Comment 17

[Mounir Lamouri (:mounir)]

Your new patch didn't apply some comments without explaining why:

(In reply to Mounir Lamouri (:mounir) from comment #15)
> I'm not a big fan that we have to explicitly call a method to purge the
> expired permissions when we close an application. I think it would be better
> to do that when we *open* the application because we know we will always
> open the application but we might end up not properly closing it (extreme
> case is if I unplug the battery).

This.

> @@ +1225,5 @@
> > +        continue;
> > +      }
> > +
> > +      if (permEntry.mCachedPermission != permEntry.mPermission) {
> > +        permEntry.mPermission = permEntry.mCachedPermission;
> 
> I'm not sure how you can go there if you call |continue| after removing the
> element.

This.

> @@ +1238,5 @@
> > +NS_IMETHODIMP
> > +nsPermissionManager::ExpirePermissionsForApp(uint32_t aAppId)
> > +{
> > +  ENSURE_NOT_CHILD_PROCESS;
> > +  NS_ENSURE_ARG(aAppId != nsIScriptSecurityManager::NO_APP_ID);
> 
> nit: MOZ_ASSERT would be appropriate with a comment saying that we shouldn't
> call this for something else than an app.

That's a nit but a comment in the MOZ_ASSERT would be better ;)
MOZ_ASSERT(condition, "this is a comment");

Comment 18

[Andrea Marchesini [:baku]]

> > I'm not a big fan that we have to explicitly call a method to purge the
> > expired permissions when we close an application. I think it would be better
> > to do that when we *open* the application because we know we will always
> > open the application but we might end up not properly closing it (extreme
> > case is if I unplug the battery).


Right. I prefer to purge the session permissions when the app quits because it doesn't change the startup time of the app. About these extreme case, we don't store the session permissions in the db, so if the device reboots, the permissions will be read from the db.

> > @@ +1225,5 @@
> > > +        continue;
> > > +      }
> > > +
> > > +      if (permEntry.mCachedPermission != permEntry.mPermission) {
> > > +        permEntry.mPermission = permEntry.mCachedPermission;
> > 


We don't change the expireType of the permissions but we set it only when they are created.
When a permission type is PROMPT, the expireType is set to NEVER. When we change it, we keep the expireType, just changing the permission type. In this case the first |if| condition will be false, and we go to the second |if| statement.
>

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 693870 [details] [diff] [review]
patch d

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

r=me with the below things fixed.

::: extensions/cookie/nsPermissionManager.cpp
@@ +763,1 @@
>        entry->GetPermissions()[index].mPermission = aPermission;

Hmm.. I don't understand why the old code here wasn't setting the expiration type and the expiration time as well. That's a preexisting problem though.

But please file a followup on fixing this so that it's more correct. It's a somewhat bigger problem now that we reset session expirations while gecko is running. Not sure if we want to make such a change on the B2G branch or not.

@@ +1218,5 @@
> +
> +  for (uint32_t i = 0; i < entry->GetPermissions().Length();) {
> +    nsPermissionManager::PermissionEntry& permEntry = entry->GetPermissions()[i];
> +
> +    if (entry->GetKey()->mAppId == *appId) {

You should call NotifyObserversWithPermission in this loop as needed.

@@ +1229,5 @@
> +        permEntry.mPermission = permEntry.mCachedPermission;
> +      }
> +    }
> +
> +    ++i;

Nit: rather than putting the ++i here, you can keep it in the for-statement like normal, and do a --i before the |continue;|. Up to you.

@@ +1232,5 @@
> +
> +    ++i;
> +  }
> +
> +  return PL_DHASH_NEXT;

Return PL_DHASH_REMOVE if entry->GetPermissions().IsEmpty()?

Comment 20

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #19)
> Comment on attachment 693870 [details] [diff] [review]
> patch d
> 
> Review of attachment 693870 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the below things fixed.
> 
> ::: extensions/cookie/nsPermissionManager.cpp
> @@ +763,1 @@
> >        entry->GetPermissions()[index].mPermission = aPermission;
> 
> Hmm.. I don't understand why the old code here wasn't setting the expiration
> type and the expiration time as well. That's a preexisting problem though.

If it's a session expiration type, no need to set it (with the current Firefox Desktop architecture). Regarding time expiration type, honestly, I think it is completely broken and it has likely been made to work for the very specific use case that is currently using it. Ideally, we should remove EXPIRE_TIME from the permission manager and have that caller find another way to handle their use case.

> @@ +1218,5 @@
> > +
> > +  for (uint32_t i = 0; i < entry->GetPermissions().Length();) {
> > +    nsPermissionManager::PermissionEntry& permEntry = entry->GetPermissions()[i];
> > +
> > +    if (entry->GetKey()->mAppId == *appId) {
> 
> You should call NotifyObserversWithPermission in this loop as needed.

> @@ +1232,5 @@
> > +
> > +    ++i;
> > +  }
> > +
> > +  return PL_DHASH_NEXT;
> 
> Return PL_DHASH_REMOVE if entry->GetPermissions().IsEmpty()?

Hmmm, I should have seen that. When I wrote the RemovePermissionsForApp() method, to prevent to worry about all of that, the enumerator was just returning permissions to remove and I was calling AddInternal() so all the usual operations would happen (notification and removing the entry if needed).

What about the following:
- if mCachedPermission has no value, you AddInternal() a UNKNOWN_ACTION permission (with all attributes to the default values) so it will simply delete the permission;
- if mCachedPermission has a value, you AddInternal() the cached value permission with the appropriate other attributes so it will do the change automatically without worrying about it.

It requires a bit of re-write but I think it will make things easier in the long run and will prevent writing things twice.

Comment 21

[Mounir Lamouri (:mounir)]

(In reply to Andrea Marchesini (:baku) from comment #18)
> > > I'm not a big fan that we have to explicitly call a method to purge the
> > > expired permissions when we close an application. I think it would be better
> > > to do that when we *open* the application because we know we will always
> > > open the application but we might end up not properly closing it (extreme
> > > case is if I unplug the battery).
> 
> Right. I prefer to purge the session permissions when the app quits because
> it doesn't change the startup time of the app. About these extreme case, we
> don't store the session permissions in the db, so if the device reboots, the
> permissions will be read from the db.

Indeed, the reboot wasn't a good example. Note that changing the memory representation of the permission at the app startup is very unlikely significant.
I will not fight for that though given that the only case I was able to think of doesn't apply.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > Hmm.. I don't understand why the old code here wasn't setting the expiration
> > type and the expiration time as well. That's a preexisting problem though.
> 
> If it's a session expiration type, no need to set it (with the current
> Firefox Desktop architecture). Regarding time expiration type, honestly, I
> think it is completely broken

It's also not working if you are trying to override a EXPIRE_SESSION permission with a EXPIRE_NEVER permission.

And even when writing EXPIRE_SESSION permissions, it seems like a bad idea to have the data in the hash essentially be inaccurate.

> it has likely been made to work for the
> very specific use case that is currently using it.

Yup.

Comment 23

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #22)
> > > Hmm.. I don't understand why the old code here wasn't setting the expiration
> > > type and the expiration time as well. That's a preexisting problem though.
> > 
> > If it's a session expiration type, no need to set it (with the current
> > Firefox Desktop architecture). Regarding time expiration type, honestly, I
> > think it is completely broken
> 
> It's also not working if you are trying to override a EXPIRE_SESSION
> permission with a EXPIRE_NEVER permission.

Hmm? Don't we write to the disk the EXPIRE_NEVER permission in that case? So it will work as expected in that case, no?

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You mustn't add or remove any entries from the hashtable while iterating it. So it's a bit risky to call AddInternal with UNKNOWN since that can cause entries to get removed.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, even calling AddInternal with the cached value will assert. It will work since we never end up actually modifying the hash table, but it will assert because the hash code doesn't know that.

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/pldhash.cpp#574

(In reply to Mounir Lamouri (:mounir) from comment #23)
> > It's also not working if you are trying to override a EXPIRE_SESSION
> > permission with a EXPIRE_NEVER permission.
> 
> Hmm? Don't we write to the disk the EXPIRE_NEVER permission in that case? So
> it will work as expected in that case, no?

We will have the right data on disk. But the hash table will still contain invalid data. It's data that we currently don't use, so technically it's fine. But it still seems like a bad idea and could cause bugs in the future.

Comment 26

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #24)
> You mustn't add or remove any entries from the hashtable while iterating it.
> So it's a bit risky to call AddInternal with UNKNOWN since that can cause
> entries to get removed.

That's the purpose of it. If you have mCachedPermission pointing to nothing that means there was no permission before setting the SESSION one, right?

(In reply to Jonas Sicking (:sicking) from comment #25)
> Actually, even calling AddInternal with the cached value will assert. It
> will work since we never end up actually modifying the hash table, but it
> will assert because the hash code doesn't know that.

I didn't get that.

Comment 27

[Andrea Marchesini [:baku]]

Attachment: patch e

Same code but with notifications. Is it good enough?

Comment 28

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=843167e242cf let's wait for try before land it.

Comment 29

[Andrea Marchesini [:baku]]

Jonas, I think we have here the same problem of the bug 820704. We cannot use RemoveExpiredPermissionsForApp() when an app quits, because we can have multiple apps running with the same appId.
I'm trying to update the patch in order to have a counter of running apps.

Comment 30

[Andrea Marchesini [:baku]]

Attachment: patch f

Justin, in this patch can you review how I use the observer, the manifestURL for getting the appId, and if the logic of the counter of running app makes sense?

Comment 31

[Justin Lebar (not reading bugmail)]

Comment on attachment 694791 [details] [diff] [review]
patch f

btw, cjones said he's happy with me reviewing changes in dom/ipc that I'm
comfortable with, so no need to kick this sort of thing off to him.

>diff --git a/extensions/cookie/nsPermissionManager.h b/extensions/cookie/nsPermissionManager.h
>--- a/extensions/cookie/nsPermissionManager.h
>+++ b/extensions/cookie/nsPermissionManager.h
>@@ -263,31 +268,38 @@ private:
>+  /**
>+   * This method will restore the permissions when the session of an App ends.
>+   */
>+  static PLDHashOperator RemoveExpiredPermissionsForAppEnumerator(nsPermissionManager::PermissionHashKey* entry, void* nonused);

Nit: Would you mind wrapping this line?  I see that this file already has lines
over 80 chars, but this one is really long.  :)

>+  nsDataHashtable<nsUint32HashKey,int32_t> mApplications;

Nit: Foo<A, B>.

>@@ -1112,16 +1124,75 @@ NS_IMETHODIMP nsPermissionManager::Obser
>+  else if (!nsCRT::strcmp(aTopic, "ipc:content-created")) {
>+    // A new ipc content has been created.

"A new content process has been created." would be much clearer to me.

>+    nsCOMPtr<nsIObserver> cpo = do_QueryInterface(aSubject);
>+    ContentParent* cp = static_cast<ContentParent*>(cpo.get());
>+
>+    nsString manifestURL;
>+    cp->GetManifestURL(manifestURL);
>+
>+    nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
>+    if (appsService) {
>+      uint32_t appId;
>+      nsresult rv = appsService->GetAppLocalIdByManifestURL(manifestURL, &appId);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      if (appId != nsIScriptSecurityManager::NO_APP_ID) {
>+        int32_t counter;
>+        if (!mApplications.Get(appId, &counter)) {
>+          mApplications.Put(appId, 1);
>+        } else {
>+          mApplications.Put(appId, counter + 1);
>+        }
>+      }
>+    }
>+  }

As I understood it, in bug 820704  you were using ContentParent destruction as
a backstop.  There was a normal way that we'd clean up the audio stuff, and
then we had a fallback if a process died unexpectedly.

But in this bug, I think you want to listen to TabParent destruction.  Instead
of assuming that each ContentParent contains just one app, I think you should
watch TabParent shutdowns.  Each TabParent contains just one app.

Also, instead of keeping track of the number of processes with app-id X, I
think you need to keep track of the number of live TabParents with
(own-or-contianing-app-id, is-browser) = (x, y).

As currently written, I'm pretty sure that all browser processes will have an
empty app manifest, so therefore they'll all get lumped together, regardless of
which app launched the browser processes.  Using (own-or-containing-app-id,
is-browser) resolves this ambiguity.

(Since TabParent inherits from TabContext, you can get this data straight off a
TabParent object.)

>+  else if (!nsCRT::strcmp(aTopic, "ipc:content-shutdown")) {
>+    // A new ipc content has been destroyed, maybe we have to reset the session
>+    // for its application ID.
>+    nsCOMPtr<nsIPropertyBag2> props = do_QueryInterface(aSubject);
>+    if (!props) {
>+      NS_WARNING("ipc:content-shutdown message without property bag as subject");
>+      return NS_OK;
>+    }
>+
>+    nsString manifestURL;
>+    nsresult rv = props->GetPropertyAsAString(NS_LITERAL_STRING("manifestURL"),
>+                                             manifestURL);

Nit: alignment.

>@@ -1197,16 +1268,72 @@ nsPermissionManager::RemovePermissionsFo
>+PLDHashOperator
>+nsPermissionManager::RemoveExpiredPermissionsForAppEnumerator(nsPermissionManager::PermissionHashKey* entry, void* arg)
>+{
>+  uint32_t* appId = static_cast<uint32_t*>(arg);
>+
>+  for (uint32_t i = 0; i < entry->GetPermissions().Length(); ++i) {
>+    nsPermissionManager::PermissionEntry& permEntry = entry->GetPermissions()[i];
>+
>+    if (entry->GetKey()->mAppId == *appId) {

PermissionKey already has a mIsInBrowserElement field, so I guess that
validates what I'm saying here as not too crazy.  :)

Comment 32

[Andrea Marchesini [:baku]]

> But in this bug, I think you want to listen to TabParent destruction. 
> Instead
> of assuming that each ContentParent contains just one app, I think you should
> watch TabParent shutdowns.  Each TabParent contains just one app.

I don't understand completely why your approach is better.
My idea is to count the number of processes per appId. When the last process for an X appId quits, the permission manager resets the permissions for that app.

For my understanding, any child process is just 1 app, but 1 app can use multiple child processes. Then there are apps running in the main process.
The main process is not important at all, because if it quits, the permission manager quits too. So a counter for processes/appId should be enough for unknowing when the refresh of permissions is needed.

Comment 33

[Justin Lebar (not reading bugmail)]

> My idea is to count the number of processes per appId. When the last process for an X 
> appId quits, the permission manager resets the permissions for that app.

Consider the browser app, which is the canonical app which uses multiple processes.

Right now, the browser tabs, which run OOP from the browser "chrome", will have app-id 0.  So you can't really tell when all processes from the browser app quit.

You're also relying on the (currently true, but by no means necessarily true) assumption that we never run two apps in the same process.  If instead you use TabParents, this code will work in that case.

Note that we do exercise the capability to run multiple browser tabs in the same process, which is quite similar to running multiple apps in the same process.

Comment 34

[Andrea Marchesini [:baku]]

Attachment: patch g

Comment 35

[Justin Lebar (not reading bugmail)]

Comment on attachment 696360 [details] [diff] [review]
patch g

I talked to Andrea about this on IRC.

There are some problems with this patch:

 * It uses OwnAppId() instead of OwnOrContainingAppId(), so browser tabs always have app-id 0, which breaks the patch.

 * It clears all permissions for app X when all of app X's browser frames are closed, even if app X itself still has some app frames open.

But the bigger issue is that we don't deal with in-process apps properly, and this messes up the browser app (among other things).

We want a permission granted for the session to browser content to live for as long as the browser app lives.  That is, the session ends when the browser app closes.  But this patch only listens to TabParent creation/destruction, so that permission will go away when all browser processes go away (e.g. we close all tabs in the browser).

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Me and Justin talked this over on IRC.

Probably the best way to track when an app is closed, it by tracking it in nsFrameLoader. There is an nsFrameLoader created in the parent process every time when an app is started. And that nsFrameLoader sticks around as long as the app is running. When the app is closed the nsFrameLoader is destroyed (or at least is notified that the contained page is closed).

We shouldn't need to do any special handling for when an app crashes. We can simply treat it as the app is still running until gaia decides to close the <iframe> as normal.


However nsFrameLoader does not keep track of which appid/browserflag is currently running inside of it. Apparently it knows when the app is started, but we lose track of that after that. Justin suggested making the nsFrameLoader hold or inherit TabContext.

Comment 37

[Mounir Lamouri (:mounir)]

I haven't read in details the last comments but I feel that the issues is about cleaning the permissions on app closing. Can't we just delete the pref when the app is started instead of doing it when it is closed?

I would be really surprised if doing that would cost us performances (it's only doing r/w in memory). Granted that it's O(n) but O(n) on the permissions which are unlikely going to be numerous and we could likely improve that to be faster.

Comment 38

[Andrea Marchesini [:baku]]

(In reply to Mounir Lamouri (:mounir) from comment #37)
> I haven't read in details the last comments but I feel that the issues is
> about cleaning the permissions on app closing. Can't we just delete the pref
> when the app is started instead of doing it when it is closed?

This doesn't change the problem. Some app can run multiple instances at the same time: browser tabs for instance. We need to have a counter and reset the permission at the first start or at the last close.

Comment 39

[Mounir Lamouri (:mounir)]

(In reply to Andrea Marchesini (:baku) from comment #38)
> (In reply to Mounir Lamouri (:mounir) from comment #37)
> > I haven't read in details the last comments but I feel that the issues is
> > about cleaning the permissions on app closing. Can't we just delete the pref
> > when the app is started instead of doing it when it is closed?
> 
> This doesn't change the problem. Some app can run multiple instances at the
> same time: browser tabs for instance. We need to have a counter and reset
> the permission at the first start or at the last close.

My bad, I should have read more closely the discussion.

Comment 40

[Andrea Marchesini [:baku]]

Attachment: patch h

This new patch uses nsFrameLoader object.

Comment 41

[Justin Lebar (not reading bugmail)]

Comment on attachment 697917 [details] [diff] [review]
patch h

Okay!  I think this will work.  Thanks.

We need to be more clear in the documentation and naming, particularly about
the distinction between "own app id" and "own or containing app id".  This used
to be a huge mess, and I'm determined not to let it become one again.  :)

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

>   /**
>+   * Register/Unregister an appId. These 2 methods are useful for resetting
>+   * the sessions for applications.
>+   */
>+  void registerApp(in unsigned long appId);
>+  void unregisterApp(in unsigned long appId);

This isn't a good comment, because it's not clear to me what these functions
do, when I should call them, or whether appID is an "own app ID" or an "own or
containing app ID".  I'd prefer something like this:

     /**
      * Increment or decrement our "refcount" of an app id.
      *
      * We use this refcount to determine an app's lifetime.  When an app's
      * refcount goes to 0, we clear the permissions given to the app which are
      * set to expire at the end of its session.
      */
     void addrefAppId(in unsigned long appId);
     void releaseAppId(in unsigned long appId);

If you don't like the "addref/release" semantics, we can probably come up with
another name that's better than "register/unregister".

>diff --git a/content/base/src/nsFrameLoader.h b/content/base/src/nsFrameLoader.h
>--- a/content/base/src/nsFrameLoader.h
>+++ b/content/base/src/nsFrameLoader.h

>@@ -376,19 +376,23 @@ private:
>                               nsIDocShellTreeOwner* aOwner,
>                               int32_t aParentType,
>                               nsIDocShellTreeNode* aParentNode);
> 
>   nsIAtom* TypeAttrName() const {
>     return mOwnerContent->IsXUL() ? nsGkAtoms::type : nsGkAtoms::mozframetype;
>   }
> 
>+  // Register/unregister the current mOwnerContent to the permission manager
>+  void ResetPermissionManagerStatus();

I'd prefer something like

    // Update the permission manager's app-id refcount based on mOwnerContent's
    // own-or-containing-app.

>+  uint32_t mOwnerAppId;

Maybe mAppIdSentToPermissionManager?  (Definitely not mOwnerAppId, which sounds
too much like mOwnAppId, which this is not.)

We want to be clear what this is used for, because it's incorrect to modify
this variable outside ResetPermissionManagerStatus().  So please add a comment
indicating that it's incorrect to modify this variable outside
ResetPermissionManagerStatus().

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

>+void
>+nsFrameLoader::ResetPermissionManagerStatus()
>+{
>+  // Finding the new app Id.
>+  uint32_t appId = nsIScriptSecurityManager::NO_APP_ID;
>+  if (OwnerIsAppFrame()) {
>+    // You can't be both an app and a browser frame.
>+    MOZ_ASSERT(!OwnerIsBrowserFrame());
>+
>+    nsCOMPtr<mozIApplication> ownApp = GetOwnApp();
>+    MOZ_ASSERT(ownApp);
>+    uint32_t ownAppId = nsIScriptSecurityManager::NO_APP_ID;
>+    if (ownApp && NS_SUCCEEDED(ownApp->GetLocalId(&ownAppId))) {
>+      appId = ownAppId;
>+    }
>+  }
>+
>+  if (OwnerIsBrowserFrame()) {
>+    // You can't be both a browser and an app frame.
>+    MOZ_ASSERT(!OwnerIsAppFrame());
>+
>+    nsCOMPtr<mozIApplication> containingApp = GetContainingApp();
>+    uint32_t containingAppId = nsIScriptSecurityManager::NO_APP_ID;
>+    if (containingApp && NS_SUCCEEDED(containingApp->GetLocalId(&containingAppId))) {
>+      appId = containingAppId;
>+    }
>+  }

I think it would be a bit cleaner if you got the own/containing app and then
pulled the ID off it outside the two if statements.  (I know the code you
copied from -- which I wrote! -- doesn't do that, but it does do something
different at the end of the two if statements! :)

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

>@@ -37,23 +38,25 @@ public:
>   public:
>     PermissionEntry(int64_t aID, uint32_t aType, uint32_t aPermission,
>                     uint32_t aExpireType, int64_t aExpireTime)
>      : mID(aID)
>      , mType(aType)
>      , mPermission(aPermission)
>      , mExpireType(aExpireType)
>      , mExpireTime(aExpireTime)
>+     , mCachedPermission(aPermission)
>     {}
> 
>     int64_t  mID;
>     uint32_t mType;
>     uint32_t mPermission;
>     uint32_t mExpireType;
>     int64_t  mExpireTime;
>+    uint32_t mCachedPermission;

We normally say that a "cache" holds duplicate data which can be thrown away at
will; it's an optimization.  Think of a browser's cache, or a CPU's cache.

This isn't really a cached permission.  Even if we can recover it from
somewhere (I don't know if we can), what I think you mean here is that this is
the permission to restore when the session permission expires.

Maybe mNonSessionPermission would be a better name.

>@@ -261,33 +266,48 @@ private:
>     {}
>   };
> 
>   /**
>    * This method will return the list of all permissions that are related to a
>    * specific app.
>    * @param arg has to be an instance of GetPermissionsForAppStruct.
>    */
>-  static PLDHashOperator GetPermissionsForApp(nsPermissionManager::PermissionHashKey* entry, void* arg);
>+  static PLDHashOperator
>+  GetPermissionsForApp(PermissionHashKey* entry, void* arg);
>+
>+  /**
>+   * This method will restore the permissions when the session of an App ends.
>+   */
>+  static PLDHashOperator
>+  RemoveExpiredPermissionsForAppEnumerator(PermissionHashKey* entry,
>+                                           void* nonused);

    // This method restores an app's permissions when its session ends.

>   nsCOMPtr<nsIObserverService> mObserverService;
>   nsCOMPtr<nsIIDNService>      mIDNService;
> 
>   nsCOMPtr<mozIStorageConnection> mDBConn;
>   nsCOMPtr<mozIStorageAsyncStatement> mStmtInsert;
>   nsCOMPtr<mozIStorageAsyncStatement> mStmtDelete;
>   nsCOMPtr<mozIStorageAsyncStatement> mStmtUpdate;
> 
>   nsTHashtable<PermissionHashKey> mPermissionTable;
>   // a unique, monotonically increasing id used to identify each database entry
>   int64_t                      mLargestID;
> 
>   // An array to store the strings identifying the different types.
>   nsTArray<nsCString>          mTypeArray;
> 
>+  // A list of struct for counting applications
>+  struct ApplicationCounter {
>+    uint32_t mAppId;
>+    uint32_t mCounter;
>+  };
>+  nsTArray<ApplicationCounter> mApplications;

If you rename the methods "addrefAppId" and "releaseAppId", then maybe this
should be mAppIdRefcounts.

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

>+PLDHashOperator
>+nsPermissionManager::RemoveExpiredPermissionsForAppEnumerator(nsPermissionManager::PermissionHashKey* entry, void* arg)
>+{

The lines in this method (including the method definition) are way too long.
Please wrap to 80 chars here and elsewhere.  That might mean that you have to do

   foo->Bar(
     baz,
     bee);

   instead of the preferred

   foo->Bar(baz,
            bee);

   but fitting within 80 columns is much more important than that distinction, IMO.

>+  uint32_t* appId = static_cast<uint32_t*>(arg);
>+
>+  for (uint32_t i = 0; i < entry->GetPermissions().Length(); ++i) {
>+    nsPermissionManager::PermissionEntry& permEntry = entry->GetPermissions()[i];
>+
>+    if (entry->GetKey()->mAppId == *appId) {

Please change this to

      if (entry->GetKey()->mAppId != *appId) {
        continue;
      }

So as to get rid of an indentation level.  Also please move permEntry closer to
where it's used.

>@@ -1545,8 +1610,53 @@ nsPermissionManager::UpdateDB(OperationT
>     return;
>   }
> 
>   nsCOMPtr<mozIStoragePendingStatement> pending;
>   rv = aStmt->ExecuteAsync(nullptr, getter_AddRefs(pending));
>   MOZ_ASSERT(NS_SUCCEEDED(rv));
> }
> 
>+NS_IMETHODIMP
>+nsPermissionManager::RegisterApp(uint32_t aAppId)
>+{
>+  if (aAppId != nsIScriptSecurityManager::NO_APP_ID) {

Similarly here,

    if (aAppId == nsIScriptSecurityManager::NO_APP_ID) {
      return NS_OK;
    }

>+    bool found = false;
>+    for (uint32_t i = 0; i < mApplications.Length(); ++i) {
>+      if (mApplications[i].mAppId == aAppId) {
>+        ++mApplications[i].mCounter;
>+        found = true;
>+        break;
>+      }
>+    }
>+
>+    if (!found) {
>+      ApplicationCounter app = { aAppId, 1 };
>+      mApplications.AppendElement(app);
>+    }
>+  }
>+
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsPermissionManager::UnregisterApp(uint32_t aAppId)
>+{
>+  // A new ipc content has been destroyed, maybe we have to reset the session
>+  // for its application ID.

Please update this comment.

>+  if (aAppId != nsIScriptSecurityManager::NO_APP_ID) {

One last time, invert this if so as to reduce nesting.

Is this something we can write tests for?

Comment 42

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=2419faba9248 green on try

Comment 43

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 44

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ae74de1910

Comment 45

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/b5ae74de1910

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/57af5246f2c0
https://hg.mozilla.org/releases/mozilla-b2g18/rev/a1377f4d6b95

Comment 47

[Jason Smith [:jsmith]]

Verified through perms testing at the work week.