783408 - Delete cookiejar when we remove a B2G app

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Jason Duell]

I assume we have a nsIObserver notification when an app is uninstalled.  We should remove its cookies from the parent nsCookieService hash and the sqlite DB at that time.

Comment 1

[Chris Lee [:clee]]

Marking as blocking and can you fill out the LOE in the whiteboard?  Thanks.

Comment 2

[Jason Duell]

jlebar, cjones, sicking: If someone can point me at how we provide notification for 'app-uninstall' I can make quick work of this.

Comment 3

[Justin Lebar (not reading bugmail)]

(In reply to Jason Duell (:jduell) from comment #2)
> jlebar, cjones, sicking: If someone can point me at how we provide
> notification for 'app-uninstall' I can make quick work of this.

Fabrice or Mounir might know better.

Comment 4

[[:fabrice] Fabrice Desré]

We fire an observer notification: 

Services.obs.notifyObservers(this, "webapps-sync-uninstall", app);

|app| has a localId property which is the appId we probably need.

Comment 6

[Mounir Lamouri (:mounir)]

Jason, did you begin working on that? I had a look and I have a clear idea about how to implement this feature. Do you want me to go ahead or do you have anything ready?

Comment 7

[Mounir Lamouri (:mounir)]

Taking this bug given that I just finished writing the patches.

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - GetCookiesForApp

Add a method returning all cookies for a given app. If InBrowserElement is true, it will only returned cookies for browser elements inside the app.

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - RemoveCookiesForApp

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Hook to webapps-uninstall

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Part 4 - Tests

Comment 12

[Mounir Lamouri (:mounir)]

With these patches included, you can easily remove cookies for a given app or for browsers of a given app. However, there are only hooks for app uninstall. Hooks for browser private data removal will have to happen in another bug but that should be pretty trivial.

Comment 13

[Mounir Lamouri (:mounir)]

Try server: https://tbpl.mozilla.org/?tree=Try&rev=c7917f934542

Comment 14

[Justin Lebar (not reading bugmail)]

Comment on attachment 663416 [details] [diff] [review]
Part 3 - Hook to webapps-uninstall

> +} // anonymous namespace
> +
> +NS_IMPL_ISUPPORTS1(AppUninstallObserver, nsIObserver)

I guess it doesn't matter, but can you move the NS_IMPL_ISUPPORTS1 into the anonymous namespace?

> +   * Initialize the "webapp-uninstall" observing.
> +   * Will create a nsCookieService instance if needed.
> +   * That way, we can prevent have nsCookieService created at startup just
> +   * to be able to clear data when an application is uninstalled.

I think I understand what you're saying, but it's kind of confusing.  How about something like:

  Start watching the observer service for messages indicating that an app has been uninstalled.  When an app is uninstalled, we get the cookie service (thus instantiating it, if necessary) and clear all the cookies for that app.

Looks great aside from those nits!

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - GetCookiesForApp

New version with minor changes.

Comment 16

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - RemoveCookiesForApp

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Hook to webapps-uninstall (r=jlebar)

With jlebar's comments applied.

Comment 18

[Mounir Lamouri (:mounir)]

Attachment: Part 4 - Tests

Adding a test to make sure that we don't actually delete non-app cookies when uninstalling an app.

Comment 19

[Jason Duell]

Comment on attachment 663675 [details] [diff] [review]
Part 2 - RemoveCookiesForApp

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

Thanks for jumping into this Mounir.

The approach here is both more complicated and inefficient than it needs to be :)   We don't need the enumerator functions in the IDL (getCookiesForApp).   Just have the removeCookiesForApp, and have it 1) remove the entry in the nsCookieService::hostTable hashtable, and 2) issue a SQL statement that deletes the cookies from the database.  That will be faster, easier, and you won't have to infer appId/inBrowser from nsCookies.

I'm happy to look at the next rev of this, but for final review we'll need a cookie peer (or permission for me to review from one).  mconnor is probably the best choice.

Comment 20

[Jason Duell]

* If onlyBrowserElement is set to true, the method will only remove the
* cookies marked as part of a browser element inside the app.
*
* Special app id values are not allowed (NO_APP_ID or UNKNOWN_APP_ID for example).
	
Oh, also: B2G firefox appears to be using a nonzero appId for the app, but zero (aka NO_APP_ID) for the mozbrowser process's appID (I consider this to be an insane design decision, FWIW). So we'll need to either change that, or accept NO_APP_ID.   Given that allowing NO_APP_ID would mean that we'd probably allow the firefox process to delete the OS's cookies (if it can pass NO_APP_ID and somehow also set inBrowser=false), we should probably change it.  Do you know who made this decision?  Fabrice didn't when I last asked him.

Comment 21

[Jason Duell]

Comment on attachment 663674 [details] [diff] [review]
Part 1 - GetCookiesForApp

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

As mentioned I don't think we need the enum approach here.  This code might come in handy at some future point when we want to expose a way for apps to enumerate through their own cookies.

::: netwerk/cookie/nsCookieService.cpp
@@ +3767,5 @@
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  NS_ENSURE_TRUE(aAppId != NECKO_NO_APP_ID && aAppId != NECKO_UNKNOWN_APP_ID,
> +                 NS_ERROR_INVALID_ARG);

IIRC the mozbrowser process for B2G firefox is using NECKO_NO_APP (aka appID=0) for some reason (which really seems a bad idea to me), so we'd better either change that or accept it.

::: netwerk/cookie/nsICookieManager2.idl
@@ +110,5 @@
> +
> +  /**
> +   * Returns an enumerator of all cookies that are related to a specific app.
> +   *
> +   * If the onlyBrowserELement parameter is set to true, only cookies part of

onlyBrowserELement--lowercase the "L".  Also "only cookies *that are* part of ..."

@@ +112,5 @@
> +   * Returns an enumerator of all cookies that are related to a specific app.
> +   *
> +   * If the onlyBrowserELement parameter is set to true, only cookies part of
> +   * a browser element inside the app will be returned. If set to false, all
> +   * cookies will be returned, regardless of their browserElement flag.

I'm not sure the semantic here is optimal--there's no way to enumerate through just the app's cookies, you have to get its inBrowser ones mixed in.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Jason Duell (:jduell) from comment #20)
> Oh, also: B2G firefox appears to be using a nonzero appId for the app, but
> zero (aka NO_APP_ID) for the mozbrowser process's appID (I consider this to
> be an insane design decision, FWIW).

This wasn't a design decision, it's just a bug.

Comment 23

[Jason Duell]

Sorry, some earlier review comments got left in that last comment--ignore since I don't think we need the patch at all.

Comment 24

[Jason Duell]

Comment on attachment 663675 [details] [diff] [review]
Part 2 - RemoveCookiesForApp

Hmm, it does look like the enumerate function might be useful just for the test--I'll look over that tomorrow.  I still think the actual deletion shouldn't use enumerate.

Comment 25

[Mounir Lamouri (:mounir)]

(In reply to Jason Duell (:jduell) from comment #24)
> Comment on attachment 663675 [details] [diff] [review]
> Part 2 - RemoveCookiesForApp
> 
> Hmm, it does look like the enumerate function might be useful just for the
> test--I'll look over that tomorrow.  I still think the actual deletion
> shouldn't use enumerate.

Indeed, that's exactly why I used that function: it helps testing and isn't really bad for performances. My plan was to have a follow-up to have the delete method not using this method (as said in a comment).

Comment 26

[Justin Lebar (not reading bugmail)]

> Oh, also: B2G firefox appears to be using a nonzero appId for the app, but zero (aka NO_APP_ID) for 
> the mozbrowser process's appID (I consider this to be an insane design decision, FWIW). So we'll 
> need to either change that, or accept NO_APP_ID.   Given that allowing NO_APP_ID would mean that 
> we'd probably allow the firefox process to delete the OS's cookies (if it can pass NO_APP_ID and 
> somehow also set inBrowser=false), we should probably change it.  Do you know who made this 
> decision?  

cjones said elsewhere and I agree that it's a bug.  Let's change it.  It looks like the dom/ipc/AppProcessPermissions.cpp is already written correctly, so hopefully this won't even break anything.

Comment 27

[Mounir Lamouri (:mounir)]

Those patches pass try:
https://tbpl.mozilla.org/?tree=Try&rev=0171dfd142ac

Comment 28

[Jason Duell]

Comment on attachment 663676 [details] [diff] [review]
Part 3 - Hook to webapps-uninstall (r=jlebar)

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

This also touches some cookie stuff--mostly uncontroversial IMO.

::: netwerk/cookie/nsCookieService.cpp
@@ +566,5 @@
> +    MOZ_ASSERT(appId != NECKO_NO_APP_ID);
> +
> +    nsCOMPtr<nsICookieManager2> cookieManager = do_GetService(NS_COOKIEMANAGER_CONTRACTID);
> +    return cookieManager->RemoveCookiesForApp(appId, false);
> +  }

IIUC if this happens to actually initialize the cookie manager for the first time, none of the cookies will be loaded yet, so when RemoveCookiesForApp enums there will be no cookies yet and nothing will get deleted.  It seems quite unlikely that a user could actually navigate to app uninstall before the DB is loaded, however (the cookieService is loaded when the first HTTP channel is opened, including appcache and cache hits).  So it's probably fine, for now at least.

Comment 29

[Dietrich Ayala (:dietrich)]

Mconnor/Jduell - what's the ETA on reviewing this?

Comment 30

[Mike Connor [:mconnor]]

Soon(tm), hopefully today.

Comment 31

[Mike Connor [:mconnor]]

Comment on attachment 663674 [details] [diff] [review]
Part 1 - GetCookiesForApp

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

good enough for now, would like to see a followup on the get API at least to allow retrieving just the app cookies.

::: netwerk/cookie/nsCookieService.cpp
@@ +3739,5 @@
> +
> +} // anonymous namespace
> +
> +/* static */ PLDHashOperator
> +nsCookieService::GetCookiesForApp(nsCookieEntry* entry, void* arg)

using "arg" as an argument name bugs me a little, on general principle.

@@ +3767,5 @@
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  NS_ENSURE_TRUE(aAppId != NECKO_NO_APP_ID && aAppId != NECKO_UNKNOWN_APP_ID,
> +                 NS_ERROR_INVALID_ARG);

Is there a bug on the NECKO_NO_APP thing discused in comments?

::: netwerk/cookie/nsICookieManager2.idl
@@ +112,5 @@
> +   * Returns an enumerator of all cookies that are related to a specific app.
> +   *
> +   * If the onlyBrowserELement parameter is set to true, only cookies part of
> +   * a browser element inside the app will be returned. If set to false, all
> +   * cookies will be returned, regardless of their browserElement flag.

I agree strongly here, but not enough to block forward progress.  I know this is just reflecting the delete options, but if you're sticking a get API in the IDL... let's just do it right.

Comment 32

[Mike Connor [:mconnor]]

Comment on attachment 663675 [details] [diff] [review]
Part 2 - RemoveCookiesForApp

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

I think this is ok.  I'm not sure why you inserted new arguments in the middle of existing ones, rather than just appending, but it doesn't really matter with only two consumers.

Comment 33

[Mike Connor [:mconnor]]

Comment on attachment 663676 [details] [diff] [review]
Part 3 - Hook to webapps-uninstall (r=jlebar)

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

::: netwerk/cookie/nsCookieService.cpp
@@ +566,5 @@
> +    MOZ_ASSERT(appId != NECKO_NO_APP_ID);
> +
> +    nsCOMPtr<nsICookieManager2> cookieManager = do_GetService(NS_COOKIEMANAGER_CONTRACTID);
> +    return cookieManager->RemoveCookiesForApp(appId, false);
> +  }

As pointed out on IRC, if the device starts in airplane mode... we may have a problem.

I'm fine with treating that as a followup, it's a corner case at best, but we should file and triage appropriately.

Comment 34

[Mounir Lamouri (:mounir)]

(In reply to Mike Connor [:mconnor] from comment #31)
> @@ +3767,5 @@
> > +    return NS_ERROR_NOT_AVAILABLE;
> > +  }
> > +
> > +  NS_ENSURE_TRUE(aAppId != NECKO_NO_APP_ID && aAppId != NECKO_UNKNOWN_APP_ID,
> > +                 NS_ERROR_INVALID_ARG);
> 
> Is there a bug on the NECKO_NO_APP thing discused in comments?

Do you mean bug 794023 about NECKO_UNKNOWN_APP_ID?

Comment 35

[Mounir Lamouri (:mounir)]

(In reply to Mike Connor [:mconnor] from comment #31)
> ::: netwerk/cookie/nsCookieService.cpp
> @@ +3739,5 @@
> > +
> > +} // anonymous namespace
> > +
> > +/* static */ PLDHashOperator
> > +nsCookieService::GetCookiesForApp(nsCookieEntry* entry, void* arg)
> 
> using "arg" as an argument name bugs me a little, on general principle.

I agree with that principle but in that case we have a void* that means nothing. We have to static_cast it to something. IMO, |arg| or |aData| would be the best names but both are vague names.

Comment 36

[Mounir Lamouri (:mounir)]

(In reply to Mike Connor [:mconnor] from comment #32)
> Comment on attachment 663675 [details] [diff] [review]
> Part 2 - RemoveCookiesForApp
> 
> Review of attachment 663675 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this is ok.  I'm not sure why you inserted new arguments in the
> middle of existing ones, rather than just appending, but it doesn't really
> matter with only two consumers.

Because { host, appId, browserElementFlag } is the tuple that identifies a cookie. I think it's better if they are one after each other in the argument list.

Comment 37

[Mike Connor [:mconnor]]

Comment on attachment 663677 [details] [diff] [review]
Part 4 - Tests

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

Subject to biesi's request to not have mochitest in /netwerk, this looks good to me.  r/sr=me (and really, that can apply to every patch on this bug, if jduell wants to mark r+ on these).

Comment 38

[Mounir Lamouri (:mounir)]

Comment on attachment 663677 [details] [diff] [review]
Part 4 - Tests

No need for two reviews for tests.

Comment 39

[Jason Duell]

Comment on attachment 663674 [details] [diff] [review]
Part 1 - GetCookiesForApp

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

so this is now +r/sr between mconnor and I.

Comment 40

[Jason Duell]

Comment on attachment 663675 [details] [diff] [review]
Part 2 - RemoveCookiesForApp

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

ditto.   So all we need is the mochitest to move from /netwerk for this to land.   We can clean up the code in followups.

Comment 41

[Mounir Lamouri (:mounir)]

Thank you guys for those quick reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd91830d06d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa1746e4ec72
https://hg.mozilla.org/integration/mozilla-inbound/rev/40cb18805291
https://hg.mozilla.org/integration/mozilla-inbound/rev/397e4cc473be

Comment 42

[Mike Connor [:mconnor]]

Please be sure to file the followups before closing this bug, thanks!

Comment 43

[Jason Duell]

> As pointed out on IRC, if the device starts in airplane mode... we may have a problem.

What's the problem with airplane mode?  nsHttpChannel addsCookies to channels even when offline during asyncOpen (observers need to see them), so cookie svc should still be present just as early:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#4392

Comment 44

[Mounir Lamouri (:mounir)]

(In reply to Mike Connor [:mconnor] from comment #42)
> Please be sure to file the followups before closing this bug, thanks!

I think all the follow-ups are already filed.

Comment 45

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bd91830d06d1
https://hg.mozilla.org/mozilla-central/rev/aa1746e4ec72
https://hg.mozilla.org/mozilla-central/rev/40cb18805291
https://hg.mozilla.org/mozilla-central/rev/397e4cc473be