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.

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

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

(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.

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.

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?

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

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.

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.

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

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!

New version with minor changes.

With jlebar's comments applied.

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

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.

* 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 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.

(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.

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

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.

(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).

> 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.

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

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.

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

Soon(tm), hopefully today.

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 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 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.

(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?

(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.

(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 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 on attachment 663677 [details] [diff] [review] Part 4 - Tests No need for two reviews for tests.

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 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.

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

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

> 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

(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.

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