Delete app-cache related to an app when uninstalled

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mounir, Assigned: jduell.mcbugs)

Tracking

Trunk
mozilla18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:S][WebAPI:P1][qa-])

Attachments

(3 attachments, 4 obsolete attachments)

Reporter

Description

7 years ago
No description provided.
Assignee

Comment 1

7 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)
This sounds like a privacy-related issue so let's block on it.
blocking-basecamp: ? → +
Whiteboard: [LOE:S]
Jason, can you do this work?  Feel free to suggest a better owner if it's not you.
Assignee: nobody → jduell.mcbugs
Reporter

Comment 4

7 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
Whiteboard: [LOE:S] → [LOE:S][WebAPI:P1]
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.
Attachment #663161 - Flags: feedback?(jduell.mcbugs)
Assignee

Comment 9

7 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
Attachment #663161 - Flags: feedback?(jduell.mcbugs) → feedback+
Hi, we've only got a couple of days left before we want to freeze feature development. What's the status of this bug?
Assignee

Comment 11

7 years ago
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.
Assignee

Comment 13

7 years ago
Just fixed rename and comments from my review of v1.
Attachment #663161 - Attachment is obsolete: true
Attachment #665340 - Flags: review+
Assignee

Comment 14

7 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!).
Attachment #665341 - Flags: review?(honzab.moz)
Assignee

Comment 15

7 years ago
This uses the new notification, which has an arg that switches between full uninstall and just clearing inBrowser data.
Attachment #665342 - Flags: review?(honzab.moz)
Assignee

Comment 16

7 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.
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.
Attachment #665341 - Flags: review?(honzab.moz)
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.
Attachment #665342 - Flags: review?(honzab.moz)
Assignee

Comment 20

7 years ago
Posted patch Part 2, v3 (obsolete) — Splinter Review
> All this code should be in a anonymous namespace, right?

I think anonymous namespaces are almost not worth the bother.  But for you... :)
Attachment #665341 - Attachment is obsolete: true
Attachment #665342 - Attachment is obsolete: true
Attachment #665713 - Flags: review?(honzab.moz)
Assignee

Comment 21

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

Comment 22

7 years ago
ah, qref...
Attachment #665713 - Attachment is obsolete: true
Attachment #665713 - Flags: review?(honzab.moz)
Attachment #665752 - Flags: review?(honzab.moz)
Assignee

Updated

7 years ago
Blocks: 795203
Assignee

Updated

7 years ago
Blocks: 794023
Reporter

Updated

7 years ago
No longer blocks: 795203
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 !!!
Attachment #665752 - Flags: review?(honzab.moz) → review+
Assignee

Comment 25

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

Comment 26

7 years ago
needs test.
Flags: in-testsuite?
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?
Assignee

Updated

7 years ago
Depends on: 796164
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)
Attachment #667030 - Flags: review?(jduell.mcbugs)
Attachment #667030 - Attachment is patch: true
Assignee

Updated

7 years ago
Attachment #667030 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/ad81145cad9d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Keywords: verifyme
QA Contact: jsmith
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?
Flags: needinfo?(jduell.mcbugs)
Keywords: verifyme
Whiteboard: [LOE:S][WebAPI:P1] → [LOE:S][WebAPI:P1][qa verification blocked]
Flags: needinfo?(jduell.mcbugs)
Whiteboard: [LOE:S][WebAPI:P1][qa verification blocked] → [LOE:S][WebAPI:P1]
Keywords: verifyme
Keywords: verifyme
Whiteboard: [LOE:S][WebAPI:P1] → [LOE:S][WebAPI:P1][qa-]
You need to log in before you can comment on or make changes to this bug.