Mounir Lamouri (:mounir) Reporter
	Description • 12 years ago

      No description provided.

	Jason Duell Assignee
	Comment 1 • 12 years ago

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

	Andrew Overholt [:overholt]
	Comment 2 • 12 years ago

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

	Andrew Overholt [:overholt]
	Comment 3 • 12 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 4 • 12 years ago

(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

	Honza Bambas (:mayhemer)
	Comment 5 • 12 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 6 • 12 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 7 • 12 years ago

So we'll need something like:

nsIApplicationCacheService.evictByAppID(aAppId, aOnlyBrowserData)

	Honza Bambas (:mayhemer)
	Comment 8 • 12 years ago

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.

	Jason Duell Assignee
	Comment 9 • 12 years ago

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

	Dietrich Ayala (:dietrich)
	Comment 10 • 12 years ago

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

	Jason Duell Assignee
	Comment 11 • 12 years ago

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

	Honza Bambas (:mayhemer)
	Comment 12 • 12 years ago

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

	Jason Duell Assignee
	Comment 13 • 12 years ago

Just fixed rename and comments from my review of v1.

	Jason Duell Assignee
	Comment 14 • 12 years ago

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

	Jason Duell Assignee
	Comment 15 • 12 years ago

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

	Jason Duell Assignee
	Comment 16 • 12 years ago

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.

	Honza Bambas (:mayhemer)
	Comment 17 • 12 years ago

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

Thanks!

	Honza Bambas (:mayhemer)
	Comment 18 • 12 years ago

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.

	Honza Bambas (:mayhemer)
	Comment 19 • 12 years ago

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.

	Jason Duell Assignee
	Comment 20 • 12 years ago

> All this code should be in a anonymous namespace, right?

I think anonymous namespaces are almost not worth the bother.  But for you... :)

	Jason Duell Assignee
	Comment 21 • 12 years ago

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.

	Jason Duell Assignee
	Comment 22 • 12 years ago

ah, qref...

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 23 • 12 years ago

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?

	Honza Bambas (:mayhemer)
	Comment 24 • 12 years ago

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

	Jason Duell Assignee
	Comment 25 • 12 years ago

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.

	Jason Duell Assignee
	Comment 26 • 12 years ago

needs test.

	Ed Morley [:emorley]
	Comment 27 • 12 years ago

Backed out for:
https://tbpl.mozilla.org/php/getParsedLog.php?id=15645718&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/551152b8c271

	Ed Morley [:emorley]
	Comment 28 • 12 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 29 • 12 years ago

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?

	Honza Bambas (:mayhemer)
	Comment 30 • 12 years ago

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)

	Honza Bambas (:mayhemer)
	Comment 31 • 12 years ago

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

	Jason Duell Assignee
	Comment 32 • 12 years ago

Try looks good:

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

	Ed Morley [:emorley]
	Comment 33 • 12 years ago

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

	Jason Smith [:jsmith]
	Comment 34 • 12 years ago

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?