827050 - Clear cookies and stored data in the browser does not clear remember my choice permissions for PROMPT_ACTION WebAPIs

Status: RESOLVED FIXED

(Core :: Permission Manager, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G 18 1/5/2013
Device: Unagi

Steps:

1. Go to http://mozqa.com/data/firefox/geolocation/position.html in the browser
2. Select remember my choice and deny
3. Reload the page - confirm that remember my choice is now set to deny
4. Go to browser settings and select "Clear cookies and stored data"
5. Go back to http://mozqa.com/data/firefox/geolocation/position.html

Expected:

A permission prompt should appear asking for geolocation permissions, as the remember my choice decision should have been removed upon completing step #4.

Actual:

The remember my choice decision activates to deny access to geolocation on that site.

Additional Notes:

This is a big problem - if a user makes the mistake of doing remember my choice by accident to allow or deny for a site access to a PROMPT_ACTION WebAPI in the browser, but needs to recover from their mistake, they won't be able to recover at all unless they do an entire factory reset of the phone (which isn't an acceptable work-around). There needs to be some way to allow a user to recover from a bad remember my choice decision. Otherwise, such bad situations can happen such as:

* I accidentally hit deny with remember my choice on a site I want to use in the future, so now I can never use that site's use of that WebAPI ever again on the phone
* I accidentally hit allow with remember my choice on a site I think is dangerous by accident, so now anytime I get back to this site, the WebAPI access will immediately be given, even though I don't think it's safe. And I can't recover from it.

Noming to block as I believe there needs to be some work-around to allow a user to revert a remember my choice decision in the browser. 3rd party apps have the work-around by controlling the permissions in settings app, but the browser does not have ability. The acceptable approach we can at least do for v1 is allow clearing of the remember my choice settings when a user selects to clear stored data in the browser.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch 1

Comment 2

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

Comment on attachment 698458 [details] [diff] [review]
patch 1

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

This will remove all permissions both for the app itself, when the user presses the "clear private data" button. We only want to clear the data for that appid which has the browserflag set to true.

The mozIApplicationClearPrivateDataParams object has a "browserOnly" flag. When that flag is false you should do what RemoveExpiredPermissionsForApp(appId) does today. But when that flag is true, you only want to clear the data which has the isInBrowserElement flag set to true.

Comment 3

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

Oh, wait, I don't think RemoveExpiredPermissionsForApp(appId) is the right function here at all!

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch b

I don't know if this is what you mean, but probably yes :)

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch b

My changes were not applied in the previous patch.

Comment 6

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

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

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

Ship it!

Comment 7

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66a04865fe51

Comment 8

[Mounir Lamouri (:mounir)]

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

This patch should have been minus'd in my opinion. It has few serious problems that were not mentioned during the review:
- nsPermissionManager now listen to webapps-clear-data *and* webapps-uninstall which means that it will try to delete twice app's data when an app is uninstall. Listening  only to webapps-clear-data would have been simpler and easier to implement;
- we are duplicating code for nothing given that webapps-clear-data and webapps-uninstall should have been merged and there handling is roughly the same;
- webapps-clear-data should not be observed at ::Init() but before because the permission manager is initialized lazily which means if we happen to call webapps-clear-data without having created an instance of the permission manager, the call will be ignored (agreed that this is purely theoretical);
- we could easily have tested that but there are no tests... :(

Comment 9

[Ed Morley [:emorley]]

Backed out for xpcshell orange:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=66a04865fe51

https://hg.mozilla.org/integration/mozilla-inbound/rev/510b15b4d139

Please use Try before landing again - ideally also hold off landing until there are tests + the rest of comment 8 has been addressed.

Comment 10

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

(In reply to Mounir Lamouri (:mounir) from comment #8)
> Comment on attachment 698564 [details] [diff] [review]
> patch b
> 
> This patch should have been minus'd in my opinion. It has few serious
> problems that were not mentioned during the review:
> - nsPermissionManager now listen to webapps-clear-data *and*
> webapps-uninstall which means that it will try to delete twice app's data
> when an app is uninstall. Listening  only to webapps-clear-data would have
> been simpler and easier to implement;

webapps-clear-data is for clearing private data, which you in theory could do for whole apps in addition to just the browser-content. But clearing private data for permissions doesn't mean removing all permissions, it means resetting to what we had just after the app was installed. Doing that is fairly complex though, and not something that's hooked up to public APIs.

But note that we're not duplicating any actual clearing. There's an early-return which prevents data from being cleared if you try to clear all app data, which is what happens during uninstall.

> - we are duplicating code for nothing given that webapps-clear-data and
> webapps-uninstall should have been merged and there handling is roughly the
> same;

See above.

> - webapps-clear-data should not be observed at ::Init() but before because
> the permission manager is initialized lazily which means if we happen to
> call webapps-clear-data without having created an instance of the permission
> manager, the call will be ignored (agreed that this is purely theoretical);
> - we could easily have tested that but there are no tests... :(

Good catch.

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #10)
> (In reply to Mounir Lamouri (:mounir) from comment #8)
> > Comment on attachment 698564 [details] [diff] [review]
> > patch b
> > 
> > This patch should have been minus'd in my opinion. It has few serious
> > problems that were not mentioned during the review:
> > - nsPermissionManager now listen to webapps-clear-data *and*
> > webapps-uninstall which means that it will try to delete twice app's data
> > when an app is uninstall. Listening  only to webapps-clear-data would have
> > been simpler and easier to implement;
> 
> webapps-clear-data is for clearing private data, which you in theory could
> do for whole apps in addition to just the browser-content. But clearing
> private data for permissions doesn't mean removing all permissions, it means
> resetting to what we had just after the app was installed. Doing that is
> fairly complex though, and not something that's hooked up to public APIs.

I do not understand your point. When an app is uninstalled, we want to remove all permissions that have been changed during the lifetime of the application. When data is requested to be cleared, we also want to remove all permissions that have been changed during the lifetime of the application. The only difference between the two events is that in one case we know that the app will continue to live and in the other, it is uninstalled.

BTW, all of this discussion is purely theoretical. Right now, no data storage except the permission manager is listening to "webapps-uninstall" (that was by design at the time). IIRC, when "webapps-clear-data" has been introduced, all data storage switch from listening to "webapps-uninstall" to "webapps-clear-data". Doing the same thing nsPermissionManager would make sense.
The main point here is to save code complexity. If we listen to "webapps-clear-data" only, we simply have to s/AppUninstallObserver/AppClearDataObserver/, s/webapps-uninstall/webapps-clear-data/, add a boolean value to RemovePermissionsForApp() (or a new method that will not be in nsIPermissionManager if we really care) and change the content of AppClearDataObserver::Observe to get the arguments correctly.

> But note that we're not duplicating any actual clearing. There's an
> early-return which prevents data from being cleared if you try to clear all
> app data, which is what happens during uninstall.

Yes, I saw that. Removing the early-return would just waste CPU but should be a behavioural no-op.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch c

Comment 13

[Mounir Lamouri (:mounir)]

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

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

It seems that you have copied the tests. Could you use `hg cp` so it would be easier to understand what you changed.

Note: backup then revert your changes then you can hg cp and copy back your changes. hg will produce a diff between the original file and the new one.

::: extensions/cookie/nsPermissionManager.cpp
@@ +153,3 @@
>  
>      nsCOMPtr<nsIPermissionManager> permManager = do_GetService("@mozilla.org/permissionmanager;1");
> +    nsPermissionManager* pm = static_cast<nsPermissionManager*>(permManager.get());

Don't static_cast<> here...

@@ +1177,2 @@
>    ENSURE_NOT_CHILD_PROCESS;
> +  NS_ENSURE_TRUE(mDBConn, NS_ERROR_FAILURE);

nsPermissionManager never checks if mDBConn is initialized. It should always be.

IOW, you can change that for a MOZ_ASSERT if you want.

@@ +1191,5 @@
>    sql.AppendInt(aAppId);
>  
> +  if (aBrowserOnly) {
> +    sql.AppendLiteral(" AND isInBrowserElement=");
> +    sql.AppendInt(true);

Why don't you simply do:
  sql.AppendLiteral(" AND isInBrowserElement=1");
It seems useless to have a .AppendInt() call with a constant value there.

::: extensions/cookie/nsPermissionManager.h
@@ +201,5 @@
>     * to be able to clear data when an application is uninstalled.
>     */
> +  static void AppClearDataObserverInit();
> +
> +  nsresult RemovePermissionsForApp(uint32_t aAppId, bool aBrowserOnly);

Please change the IDL to add a boolean attribute. The fact that you static_cast<> to nsPermissionManager to be able to call that method is wrong. Check the callers of RemovePermissionsForApp and add |false| to their arguments.

Comment 14

[Andrea Marchesini [:baku]]

> > +  NS_ENSURE_TRUE(mDBConn, NS_ERROR_FAILURE);

This is needed for an xpcshell test. it seems that xpcshell doesn't have access to the profile directory. So the InitDB fails and mDBConn is null.

New patch is coming.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: patch d

Comment 16

[Mounir Lamouri (:mounir)]

(In reply to Andrea Marchesini (:baku) from comment #14)
> > > +  NS_ENSURE_TRUE(mDBConn, NS_ERROR_FAILURE);
> 
> This is needed for an xpcshell test. it seems that xpcshell doesn't have
> access to the profile directory. So the InitDB fails and mDBConn is null.

As I said, mDBConn being null is a bug.

Is it still crashing if you add this at the beginning of the test (in run_test):
  do_get_profile();

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch e

Comment 18

[Mounir Lamouri (:mounir)]

Comment on attachment 698693 [details] [diff] [review]
patch e

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

r=me with the comments applied.

::: extensions/cookie/test/test_app_uninstall_permissions.html
@@ +177,4 @@
>      for (i in this.result) {
>        var app = this.result[i];
>        if (app.manifestURL == gManifestURL) {
> +        is(getPermissionCountForApp(gTestAppId), 0, "App should have 0 permissions");

Please, also check that the number of permissions at the end of the test is equivalent to the number of permissions at the beginning of the test. Otherwise, something deleting all permissions would pass the test.

::: netwerk/base/public/nsIPermissionManager.idl
@@ +191,5 @@
>    readonly attribute nsISimpleEnumerator enumerator;
>  
>    /**
>     * Remove all permissions associated with a given app id.
>     */

nit: please document the two parameters.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch f

Comment 20

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

(In reply to Mounir Lamouri (:mounir) from comment #11)
> > webapps-clear-data is for clearing private data, which you in theory could
> > do for whole apps in addition to just the browser-content. But clearing
> > private data for permissions doesn't mean removing all permissions, it means
> > resetting to what we had just after the app was installed. Doing that is
> > fairly complex though, and not something that's hooked up to public APIs.
> 
> I do not understand your point. When an app is uninstalled, we want to
> remove all permissions that have been changed during the lifetime of the
> application.

Yup.

> When data is requested to be cleared, we also want to remove
> all permissions that have been changed during the lifetime of the
> application.

No. When data is requested to be cleared, we want to *reset* all permission-decisions for the app. Not remove all permissions stored for an app.

For example, if the user installs a privileged app which has something like the following in it's manifest:

permissions: {
  "device-storage:music": {
    access: "readonly",
    description: "..."
  },
  "tcp-socket": {
    description: "..."
  }
}

We end up with two entries in the permissions database. One for device-storage:music-read set to PROMPT_ACTION and one for tcp-socket set to ALLOW_ACTION.

If the user then uses the app and at some point gets a prompt due to device storage being used, to which the user answers "yes" and "remember this decision" we end up with the device-storage:music-read permission also set to ALLOW_ACTION in the database.

At this point, if we get a webapps-uninstall notification, we should remove both of the above entries for the database.

But if we get a webapps-clear-data notification, with the browserOnly flag set to true, we should set the device-storage:music-read permission back to PROMPT_ACTION. However we shouldn't remove either entry from the permissions database.

Fortunately we currently only set webapps-clear-data when uninstalling, so it's not a big deal that we don't reset the data appropriately since it's removed right away afterwards.

> BTW, all of this discussion is purely theoretical. Right now, no data
> storage except the permission manager is listening to "webapps-uninstall"
> (that was by design at the time). IIRC, when "webapps-clear-data" has been
> introduced, all data storage switch from listening to "webapps-uninstall" to
> "webapps-clear-data". Doing the same thing nsPermissionManager would make
> sense.

Yes, the discussion is theoretical right now. But only because we don't expose any public APIs for clearing app data. We only have APIs for clearing just the browser-data.

Comment 21

[Andrea Marchesini [:baku]]

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

Comment 22

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/76100f3a59f0 - it took forever to see it and see where it came from, but every debug Linux and Windows xpcshell run was failing netwerk/test/unit_ipc/test_cache_jar_wrap.js with 

child: TEST-INFO | (xpcshell/head.js) | exiting test
[Child 22316] WARNING: An event was posted to a thread that will never run it (rejected): file ../../../xpcom/threads/nsThread.cpp, line 368
[Child 22316] WARNING: unable to post continuation event: file ../../../xpcom/io/nsStreamUtils.cpp, line 445
!!! error running onStopped callback: TypeError: callback is not a function
[Child 22316] WARNING: Startup cache is only available in the chrome process: file ../../startupcache/StartupCache.cpp, line 93
[Child 22316] ###!!! ASSERTION: cannot set pref from content process: 'Error', file ../../../../modules/libpref/src/nsPrefBranch.cpp, line 133
...
Crash reason:  SIGSEGV
Crash address: 0x0

Thread 0 (crashed)
 0  libmozalloc.so!mozalloc_abort(char const*) [mozalloc_abort.cpp : 30 + 0x0]
    eip = 0x00357f15   esp = 0xbfbb7440   ebp = 0xbfbb7458   ebx = 0x00359130
    esi = 0x00c59844   edi = 0xbfbb7488   eax = 0x0000000a   ecx = 0x00000001
    edx = 0x00c5a32c   efl = 0x00210246
    Found by: given as instruction pointer in context
 1  libxul.so!NS_DebugBreak_P [nsDebugImpl.cpp : 422 + 0x5]
    eip = 0x02150d7f   esp = 0xbfbb7460   ebp = 0xbfbb7898   ebx = 0x0318231c
    esi = 0xbfbbbe14   edi = 0xbfbb7488
    Found by: call frame info
 2  libxul.so!nsPrefBranch::SetBoolPref(char const*, bool) [nsPrefBranch.cpp : 133 + 0x23]
    eip = 0x011558e5   esp = 0xbfbb78a0   ebp = 0xbfbb78e8   ebx = 0x0318231c
    esi = 0x08e904e0   edi = 0x090ee488
    Found by: call frame info
 3  libxul.so!mozilla::Preferences::SetBoolPref(char const*, bool) [Preferences.h : 48 + 0x13]
    eip = 0x0115a79a   esp = 0xbfbb78f0   ebp = 0xbfbb7908   ebx = 0x0318231c
    esi = 0xbfbb79e8   edi = 0x00000000
    Found by: call frame info
 4  libxul.so + 0x14e2593
    eip = 0x02162594   esp = 0xbfbb7910   ebp = 0xbfbb7938   ebx = 0x0318231c
    esi = 0xbfbb79e8   edi = 0x00000000
    Found by: call frame info
 5  libxul.so!CallMethodHelper::Invoke() [XPCWrappedNative.cpp : 3085 + 0xf]
    eip = 0x01b36507   esp = 0xbfbb7940   ebp = 0xbfbb7958
    Found by: previous frame's frame pointer
...
27  libxul.so!nsObserverList::NotifyObservers(nsISupports*, char const*, unsigned short const*) [nsObserverList.cpp : 99 + 0xe]
    eip = 0x021171fc   esp = 0xbfbb9070   ebp = 0xbfbb90a8   ebx = 0x0318231c
    esi = 0x00000001   edi = 0xbfbb909c
    Found by: call frame info
28  libxul.so!nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned short const*) [nsObserverService.cpp : 161 + 0xa]
    eip = 0x02117664   esp = 0xbfbb90b0   ebp = 0xbfbb90e8   ebx = 0x0318231c
    esi = 0x090fd180   edi = 0x00000000
    Found by: call frame info
...

like https://tbpl.mozilla.org/php/getParsedLog.php?id=18577018&full=1&branch=mozilla-inbound

Comment 23

[Andrea Marchesini [:baku]]

Attachment: patch + xpcshell

This is the previous patch plus:
1. process check in the ClearDataObserver
2. do_get_profile here and there in some xpcshell tests
3. a test of the process in the test_cache_jar.js because it is used in main and child processes.

Comment 24

[Mounir Lamouri (:mounir)]

Attachment: Test crash fix

That patch fix the test crash. For some reasons, I do not need to add do_get_profile() to test_cache_jar_wrap.js. At least, it is working locally.

Comment 25

[Andrea Marchesini [:baku]]

Attachment: inter-diff

Comment 26

[Mounir Lamouri (:mounir)]

Comment on attachment 699169 [details] [diff] [review]
inter-diff

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +139,5 @@
> +    // The PermissionManager is able to remove permissions only
> +    // when running on the main thread.
> +    if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +      return NS_OK;
> +    }

Handling of "webapps-clear-data" in the content process is indeed an issue but ignoring it doesn't seem to be the best solution for sure. We could also send the message to the parent process. It would be better IMO. Anyway, I think it is better to do nothing here, as long as the test passes and open a follow-up for a real fix.

::: netwerk/test/unit/test_cache_jar.js
@@ +87,5 @@
> +  if (Cc["@mozilla.org/xre/app-info;1"]
> +        .getService(Ci.nsIXULRuntime).processType ==
> +        Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
> +    do_get_profile();
> +  }

Please use my fix instead.

Comment 27

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

> Handling of "webapps-clear-data" in the content process is indeed an issue
> but ignoring it doesn't seem to be the best solution for sure. We could also
> send the message to the parent process. It would be better IMO. Anyway, I
> think it is better to do nothing here, as long as the test passes and open a
> follow-up for a real fix.

No, I don't think we should add code that isn't needed just for the sake that it looks good.

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 699166 [details] [diff] [review]
Test crash fix

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

thanks!

::: testing/xpcshell/head.js
@@ +738,5 @@
>   * @return nsILocalFile of the profile directory.
>   */
>  function do_get_profile() {
> +  if (!runningInParent) {
> +    return;

would you want to log something here to aid in debugging if something goes wrong?

Comment 29

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #27)
> > Handling of "webapps-clear-data" in the content process is indeed an issue
> > but ignoring it doesn't seem to be the best solution for sure. We could also
> > send the message to the parent process. It would be better IMO. Anyway, I
> > think it is better to do nothing here, as long as the test passes and open a
> > follow-up for a real fix.
> 
> No, I don't think we should add code that isn't needed just for the sake
> that it looks good.

Hmm, I'm not sure what you meant here. You said no and then agreed with me. I don't think we should add any code here for the moment.

Comment 30

[Mounir Lamouri (:mounir)]

(In reply to Joel Maher (:jmaher) from comment #28)
> Comment on attachment 699166 [details] [diff] [review]
> Test crash fix
> 
> Review of attachment 699166 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> thanks!

Thank you for the quick review.

> ::: testing/xpcshell/head.js
> @@ +738,5 @@
> >   * @return nsILocalFile of the profile directory.
> >   */
> >  function do_get_profile() {
> > +  if (!runningInParent) {
> > +    return;
> 
> would you want to log something here to aid in debugging if something goes
> wrong?

I've added a TEST-INFO about the do_get_profile call being ignored because it has been made from the content process.

Comment 31

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

Ah, sorry, I misread your comment. But I don't even think it's worth filing a followup since I think what we're doing is the right thing to do.

But we can debate in the followup :)

Comment 32

[Mounir Lamouri (:mounir)]

Sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=294ff598bda7

Comment 33

[Mounir Lamouri (:mounir)]

Oups, I took the patch Gregor landed instead of the one Andrea landed.
New try run: https://tbpl.mozilla.org/?tree=Try&rev=27c8771da4f1

Comment 34

[Mounir Lamouri (:mounir)]

Pushed in m-i:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2ace4a46ea5e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/232411218f21

Comment 35

[Mounir Lamouri (:mounir)]

Pushed in b2g18:
remote:   https://hg.mozilla.org/releases/mozilla-b2g18/rev/f3a70cea8559
remote:   https://hg.mozilla.org/releases/mozilla-b2g18/rev/03cdcbfecb2c

Comment 36

[Johnny Stenback (:jst)]

Marking FIXED as this is fixed on both inbound and b2g18.

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2ace4a46ea5e
https://hg.mozilla.org/mozilla-central/rev/232411218f21

Comment 38

[Ali Kasimoglu]

We are impelemting in our project GeckoView. But we have a problem with clear cookies.
Can somebody say, what class clear cookies in geckoview? Urgent! Best Regards

Comment 39

[Ali Kasimoglu]

*implementing

Comment 40

[Johnny Stenback (:jst)]

Ali, your question would be better posted to the dev-platform newsgroup which has a much broader audience than this old fixed bug. Thanks.