What notification do we send out for app uninstalls? (forgive me if I've asked this before: can't find in my notes)

This sounds like a privacy-related issue so let's block on it.

Jason, can you do this work? Feel free to suggest a better owner if it's not you.

(In reply to Jason Duell (:jduell) from comment #1) > What notification do we send out for app uninstalls? (forgive me if I've > asked this before: can't find in my notes) "webapps-uninstall", see dom/apps/src/Webapps.jsm

We currently don't have an API to delete an appcache by appID - bug 756717 is not introducing any. Since the group id (id of an appcache) format is changing to http://path/to/manifest#appID+{t or f} and it is stored as a value in a single column, we will just do a LIKE query that can be somehow optimized in a followup bug. The new API will be something like nsIApplicationCacheService.evictByAppID(aAppId).

Actually, our requirements have expanded slightly. We're going to add a observerservice notification for "remove data" which will contain an app ID and a browser flag. When the browserflag is true, we should only remove the appcaches with the specific AppID which have the browserflag set to true. When the browserflag is false, we should remove the appcaches with the specific AppID no mater what the browserflag is set to. We need this in order to support the "clear private data" feature which has become a hard v1 requirement.

So we'll need something like: nsIApplicationCacheService.evictByAppID(aAppId, aOnlyBrowserData)

This builds but I never ran the code. Jason, when you get to manually testing this, check the following in the /path/to/profile/OfflineCache/index.sqlite: - when you install an app with, say, appid=123, then: - in moz_cache_groups table you should see a row with "http://example.com/cache.manifest#123+f" in the first column (groupid) and a similarly looking clientid in a second row - in moz_cache table there should be rows for each file in the manifest with reference to the clientid you see in the moz_cache_groups table - files for each entry are in subdirectory structure under the OfflineCache dir like /path/to/profile/OfflineCache/8/A/ABFC83FC8 ; you have to find them there - when you uninstall, all these rows has to disappear as well as all the files with them Make sure to test with a "normal" offline cache content and also some secondary app to check they will be left unattended.

Comment on attachment 663161 [details] [diff] [review] Part1, WIP1 - offline cache API to delete by appid+isinbrowser Review of attachment 663161 [details] [diff] [review]: ----------------------------------------------------------------- Looks good--just some little naming/comment nits. ::: netwerk/base/public/nsIApplicationCacheService.idl @@ +65,5 @@ > /** > + * Delete all cache entries belonging to a particular appID and > + * in-browser state. > + */ > + void discardByAppId(in int32_t appID, in boolean isInBrowser); Change boolean parameter name to "discardOnlyInBrowserEntries"? Change comment to /** * Deletes some or all of an application's cache entries. * * @param appId * The mozIApplication.localId of the application. * * @param discardOnlyBrowserEntries * If true, only entries marked as 'inBrowserElement' are deleted * (this is used by browser applications to delete user browsing * data/history.). If false, *all* entries for the given appId are * deleted (this is used for application uninstallation). */ ::: netwerk/cache/nsDiskCacheDeviceSQL.cpp @@ +2408,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + } > + > + if (!isInBrowser) { > + // Delete also all that has been installed under a browser change to // If deleting app, delete any 'inBrowserElement' entries too

Hi, we've only got a couple of days left before we want to freeze feature development. What's the status of this bug?

I'm writing patch now, should be done today.

Jason, I will wait with updating my part of the patch after you check it works. I expect more comments from you anyway.

Just fixed rename and comments from my review of v1.

This patch uses the existing "webapps-uninstall" notification to detect uninstall time. That's going away in bug 784378 and bug 794563 (only one of which has landed so far), but I wanted to test manually, so I use the existing notification. (The next patch will upgrade to the new notification). The good news: this compiles, and when I run desktop B2G and do an app install and then uninstall via http://people.mozilla.org/~fdesre/openwebapps/test.html I see my Observe code getting triggered during uninstall and we call DiscardByAppId. The bad news: I'm not seeing any entries in the appcache (sqlite db is empty, no cached files in profile/OfflineCache) when the app is installed :( so I can't tell if the deletion code Honza wrote works. (I also tried to install a web app in the B2G browser (http://baronbuddy.com/) but it also doesn't create any appcache entries (it also doesn't pop up an install permission prompt--I'm not sure B2G Firefox handles regular HTML5 offline app installs right now). We obviously need to figure out what's going on here--it seems like either bug 756717 needs some more work, or the openwebapps test is installing something other than a regular appcache app. (there are two buttons: one for an "app" and one for a "packaged app", so I was expecting the former to be a regular offline cache app, but I haven't looked into it). The missing appcache entries aren't the scope of the bug, ultimately, and I'd be OK with landing this bug and filing a followup to fix the appcache (and write tests--we need test coverage!).

This uses the new notification, which has an arg that switches between full uninstall and just clearing inBrowser data.

Comment on attachment 665341 [details] [diff] [review] Part2, v1: uses existing uninstall notification Review of attachment 665341 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/build/nsLayoutStatics.cpp @@ +101,5 @@ > #include "mozilla/dom/ipc/ProcessPriorityManager.h" > #include "nsPermissionManager.h" > > extern void NS_ShutdownChainItemPool(); > +extern void ApplicationCacheUninstallObserverInit(); This approach of initializing an observer during nsLayoutStatic::Init is basically copy-and-pasted from mounir's patch for 783408, which jlebar +r'd. I'm not thrilled with it--I'd have preferred to init this within necko, but didn't find a way to do that. (:bent suggested I init by adding ApplicationCacheService to http://mxr.mozilla.org/mozilla-central/source/netwerk/build/nsNetModule.cpp#383 but it didn't work when I tried--perhaps I did it wrong). Anyway I'm fine with it for now--it works. I wound up just forward-declaring a global function rather than including nsApplicationCacheService.h because that winds up sucking in a lot of header files from /netwerk/cache that we'd have to start exporting, which seemed more gross. ::: netwerk/base/public/nsNetUtil.h @@ +1376,5 @@ > + * Gets appId from TOPIC_WEB_APP_UNINSTALL notification's 'aData' argument > + * (which is a JSON string). > + */ > +inline nsresult > +NS_GetAppIdFromUninstallNotification(const PRUnichar *aData, uint32_t *aAppID) Note: the reason to put this in nsNetUtil is that the cookie code will need to parse the same event the same way--right now it's duplicated in mounir's patch for bug 783408, but we'll change that.

Comment on attachment 665340 [details] [diff] [review] Part1, v2: nits fixed Thanks!

Comment on attachment 665341 [details] [diff] [review] Part2, v1: uses existing uninstall notification Review of attachment 665341 [details] [diff] [review]: ----------------------------------------------------------------- Dropping r for now. ::: netwerk/cache/nsApplicationCacheService.cpp @@ +175,5 @@ > +//----------------------------------------------------------------------------- > +// App Uninstall observer: handles clearing appcache data for app uninstall and > +// clearing user data events. > +//----------------------------------------------------------------------------- > +class AppCacheUninstallObserver MOZ_FINAL : public nsIObserver { All this code should be in a anonymous namespace, right? @@ +208,5 @@ > + do_GetService("@mozilla.org/observer-service;1"); > + if (observerService) { > + observerService->AddObserver(new AppCacheUninstallObserver(), > + TOPIC_WEB_APP_UNINSTALL, > + /*holdsWeak=*/ false); Please go thought nsRefPtr with AppCacheUninstallObserver instance.

Comment on attachment 665342 [details] [diff] [review] Part 3, v1: upgrade to use new "webapps-clear-data" notification Review of attachment 665342 [details] [diff] [review]: ----------------------------------------------------------------- Please merge this with the Part 2 patch, it looks much nicer and simpler then. Otherwise looks good. But I would like to see the merged patch ones again.

> All this code should be in a anonymous namespace, right? I think anonymous namespaces are almost not worth the bother. But for you... :)

Note for uninstall to work with this bug now we need bent's changes in bug 794563 that make webapps-clear-data handle both uninstall and clear user data. That should be in the tree soon... I'd mark as a dependency except bugzilla hits a circular dependency and won't let me.

ah, qref...

Comment on attachment 665752 [details] [diff] [review] Part 2, v4: now with actual changes Review of attachment 665752 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/apps/mozIApplicationClearPrivateDataParams.idl @@ +7,5 @@ > > #include "nsISupports.idl" > > [scriptable, uuid(ba0e6c8e-8c03-4b9b-8f9b-4fb14216f56e)] > +interface mozIApplicationClearPrivateDataParams : nsISupports FYI I fixed this already. ::: netwerk/base/public/nsNetUtil.h @@ +1333,5 @@ > > return true; > } > > +#define TOPIC_WEB_APP_CLEAR_DATA "webapps-clear-data" Can we make this live in mozIApplicationClearDataParams.idl instead (in a C++ block)? This doesn't really belong in necko, it's more generic than that. @@ +1341,5 @@ > + * nsIObserverService notification. Used when clearing user data or > + * uninstalling web apps. > + */ > +inline nsresult > +NS_GetAppInfoFromClearDataNotification(nsISupports *aSubject, This is also something that needs a more generic home... Somewhere in xpcom/glue maybe?

Comment on attachment 665752 [details] [diff] [review] Part 2, v4: now with actual changes Review of attachment 665752 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab with the refptr fixed. ::: layout/build/nsLayoutStatics.cpp @@ +102,5 @@ > #include "nsPermissionManager.h" > #include "nsCookieService.h" > > extern void NS_ShutdownChainItemPool(); > +extern void ApplicationCacheUninstallObserverInit(); I think this can be turned to a static method of nsApplicationCacheService. The header doesn't need to be including nsCacheService.h, just declare class nsCacheService and include the interface header. But best would be to make this part of instance init. It is definitely doable ;) But I'm OK with this too, no need for further delays on this patch. ::: netwerk/cache/nsApplicationCacheService.cpp @@ +213,5 @@ > +{ > + nsCOMPtr<nsIObserverService> observerService = > + do_GetService("@mozilla.org/observer-service;1"); > + if (observerService) { > + nsCOMPtr<AppCacheUninstallObserver> obs nsRefPtr !!!

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6228bc28958 https://hg.mozilla.org/integration/mozilla-inbound/rev/bec7e68cad9a > nsRefPtr !!! I doubted this--see https://bugzilla.mozilla.org/show_bug.cgi?id=676603#c6 but after long chat on IRC with bent, I see the light, and we now have better nsRefPtr docs: https://developer.mozilla.org/en-US/docs/nsRefPtr Thanks for enlightening me. > I think this can be turned to a static method of nsApplicationCacheService Done > best would be to make this part of instance init. It is definitely doable ;) I tried that. I ran the debugger to see when appcacheSvc::Init gets run in B2G. I figured it would be right away--everything is apps--but it doesn't get hit for any of the builtin apps (I assume they're packaged and so aren't hitting the regular appcache?). So I got worried it might be possible to delete an app before appSvc::Init, in which case we'd miss the uninstall. Renamed s/AppUninstallObserver/AppClearDataObserver at mounir's suggestion.

needs test.

Backed out for: https://tbpl.mozilla.org/php/getParsedLog.php?id=15645718&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/551152b8c271

Also OS X debug xpcshell failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=15646473&tree=Mozilla-Inbound

Hey guys, we're trying to get approximate estimates for all feature work that didn't make 9/28. Would you guys have an ETA for this bug since it's part of the "clear private data" feature?

This is how the error looks like: WARNING: SQL statement 'SELECT GroupID, ActiveClientID FROM moz_cache_groups WHERE GroupID LIKE ?1;' was not finalized: file C:/Mozilla/src/mozilla-central/st orage/src/mozStorageConnection.cpp, line 712 ###!!! ASSERTION: sqlite3_close failed. There are probably outstanding statements that are listed above!: 'srv == SQLITE_OK', file C:/Mozilla/src/mozilla-cent ral/storage/src/mozStorageConnection.cpp, line 719 xul!mozilla::storage::Connection::Close+0x000000000000010C (c:\mozilla\src\mozilla-central\storage\src\mozstorageconnection.cpp, line 897) xul!nsOfflineCacheDevice::Shutdown+0x0000000000000370 (c:\mozilla\src\mozilla-central\netwerk\cache\nsdiskcachedevicesql.cpp, line 1444)

Whole series try run: https://tbpl.mozilla.org/?tree=Try&rev=e8ce732eb6e0

Try looks good: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad81145cad9d

Trying to think of a good way to verify this from an end-user perspective. I did try one case shown here - https://bugzilla.mozilla.org/show_bug.cgi?id=756717#c38 where I had a browser app that populated the appcache for a certain website and an app that also did the same for web content. Then, I uninstalled the app, went to the site in the browser, and still got to see the content without a network connection. That worked as expected. But the real test would be more require ensuring that the appcache for the app is truly gone on uninstall, which I'm not sure how I can do that from an end-user perspective. Any ideas?