786301 - Delete localstorage related to an app when uninstalled

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrew Overholt [:overholt]]

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

Comment 2

[Andrew Overholt [:overholt]]

Jan, can you give us a hand with this bug?

Comment 3

[Jan Varga [:janv]]

sure, will need more details
who should I talk to ?

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - RemoveAllForApp()

Add a method to remove all data in persist DB related to an app.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Hook with webapps-uninstall

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Tests

Comment 7

[Mounir Lamouri (:mounir)]

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

(the errors you can see are from patches in bug 773373)

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 663685 [details] [diff] [review]
Part 2 - Hook with webapps-uninstall

>diff --git a/dom/src/storage/nsDOMStorage.h b/dom/src/storage/nsDOMStorage.h
>--- a/dom/src/storage/nsDOMStorage.h
>+++ b/dom/src/storage/nsDOMStorage.h
>@@ -86,24 +86,38 @@ public:
> 
>   nsresult ClearAllStorages();
> 
>   static nsresult Initialize();
>   static nsDOMStorageManager* GetInstance();
>   static void Shutdown();
>   static void ShutdownDB();
> 
>+  static void AppUninstallObserverInit();

Why not do this as part of Initialize()?  Does that get called lazily or
something?  If so, can you please put a comment here indicating what's going
on?

>   /**
>    * Checks whether there is any data waiting to be flushed from a temp table.
>    */
>   bool UnflushedDataExists();
> 
>   static nsDOMStorageManager* gStorageManager;
> 
> protected:
>+  /**
>+   * This class is observing webapps removal and will take care of deleting
>+   * all DOM storage data regarding this app.
>+   *
>+   * This observer wants to access gStorageDB so it can't be in an anonymous
>+   * namespace and has to be part of a friend of DOMStorageImpl.
>+   */
>+  class AppUninstallObserver MOZ_FINAL : public nsIObserver {
>+  public:
>+    NS_DECL_ISUPPORTS
>+    NS_DECL_NSIOBSERVER
>+  };

You never use this outside nsDOMStorage.cpp, so why declare it in the header
instead of putting it in the cpp file (in an anonymous namespace)?

But it's also not clear why you can't just register the observer on
nsDOMStorageManager.  It appears to be a singleton.

>diff --git a/dom/src/storage/nsDOMStorage.cpp b/dom/src/storage/nsDOMStorage.cpp

>+NS_IMETHODIMP
>+nsDOMStorageManager::AppUninstallObserver::Observe(nsISupports *aSubject,
>+                                                   const char *aTopic,
>+                                                   const PRUnichar *data)
>+{
>+  MOZ_ASSERT(!nsCRT::strcmp(aTopic, "webapps-uninstall"));

I'm confused...why are we listening to webapps-uninstall here, when bug 784378
is sending webapps-clear-data?  Can't we just send webapps-clear-data
on app uninstall?

>+  nsCOMPtr<nsIAppsService> appsService = do_GetService("@mozilla.org/AppsService;1");
>+  nsCOMPtr<mozIApplication> app;
>+
>+  appsService->GetAppFromObserverMessage(nsAutoString(data), getter_AddRefs(app));
>+  NS_ENSURE_TRUE(app, NS_ERROR_UNEXPECTED);
>+
>+  uint32_t appId;
>+  app->GetLocalId(&appId);
>+  MOZ_ASSERT(appId != nsIScriptSecurityManager::NO_APP_ID);
>+
>+  if (!DOMStorageImpl::gStorageDB) {
>+    if (NS_FAILED(DOMStorageImpl::InitDB())) {
>+      return NS_ERROR_UNEXPECTED;
>+    }
>+  }
>+
>+  return DOMStorageImpl::gStorageDB->RemoveAllForApp(appId, false);

Please comment what the boolean parameter means.  Or better yet, split
RemoveAllForApp into RemoveAllForApp and RemoveAllForAppBrowser, thus
eliminating the boolean parameter altogether.

>@@ -189,16 +219,23 @@ nsDOMStorageManager::Shutdown()

>+/* static */ void
>+nsDOMStorageManager::AppUninstallObserverInit()
>+{
>+  nsCOMPtr<nsIObserverService> observerService = do_GetService("@mozilla.org/observer-service;1");
>+  observerService->AddObserver(new AppUninstallObserver(), "webapps-uninstall", /* holdsWeak= */ false);
>+}

Nit: Wrap lines here.

Definitely not a nit: Hold a strong reference to the |new
AppUninstallObserver()| when you call AddObserver!

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 663684 [details] [diff] [review]
Part 1 - RemoveAllForApp()

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

Looks good to me.

::: dom/src/storage/nsDOMStorageDBWrapper.cpp
@@ +206,5 @@
> +nsDOMStorageDBWrapper::RemoveAllForApp(uint32_t aAppId, bool aOnlyBrowserElement)
> +{
> +  // We only care about removing the permament storage. Temporary storage such
> +  // as session storage or private browsing storage will not be re-used anyway
> +  // and will be automatically deleted at some point.

Yes, but this should affect them even during the PB session IMO.

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Hook with webapps-uninstall

I thought I checked if nsDOMStorageManager was lazily initialized or not... Sorry for the confusion.
Regarding the notification it is listening to: this is the current notification sent when an app is uninstalled. We could change that in a follow-up if we want to handle other stuff. For the moment that bug is only about deleting private data when an app is uninstalled and Ben's patches haven't landed yet so no need to depend on them.

Comment 11

[Justin Lebar (not reading bugmail)]

Comment on attachment 664920 [details] [diff] [review]
Part 2 - Hook with webapps-uninstall

Oh wow, this is now a lot nicer than I expected!

>@@ -302,16 +306,33 @@ nsDOMStorageManager::Observe(nsISupports
>         DOMStorageImpl::gStorageDB->FlushAndDeleteTemporaryTables(true);
>       NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
>                        "DOMStorage: temporary table commit failed");
>     }
>   } else if (!strcmp(aTopic, "last-pb-context-exited")) {
>     if (DOMStorageImpl::gStorageDB) {
>       return DOMStorageImpl::gStorageDB->DropPrivateBrowsingStorages();
>     }
>+  } else if (!strcmp(aTopic, "webapps-uninstall")) {
>+    if (!DOMStorageImpl::gStorageDB) {
>+      return NS_OK;
>+    }

I'm not sure this is right -- it looks like if nobody has invoked InitDB() yet,
then the DB won't exist, and we won't delete the data, which is wrong.  See
some of the other observers we have.

>+    nsCOMPtr<nsIAppsService> appsService = do_GetService("@mozilla.org/AppsService;1");
>+    nsCOMPtr<mozIApplication> app;
>+
>+    appsService->GetAppFromObserverMessage(nsAutoString(aData), getter_AddRefs(app));

nsDependentString?

r=me with the InitDB() business figured out.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 663686 [details] [diff] [review]
Part 3 - Tests

It took a while even to just spot the test entry point.  I see no comments or hints on how the test steps execute one by one.

Please add:
- description and a list of all steps the test is doing like 1. install an app, 2. confirm popup 3. store some data to localStorage etc...
- restructure the code to be more readable say 'as a book' if possible or at least add comments like "this will invoke event X that calls method Y" or so to easily track (in best case do both)

I cannot review this code as is now easily and honestly these comments would be review requirements anyway.

Thanks.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Tests

Tests are more verbose now.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 665455 [details] [diff] [review]
Part 3 - Tests

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

Thanks!

r=honzab.

Comment 15

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/2310fb3518ee
https://hg.mozilla.org/mozilla-central/rev/205fdb51a6e3
https://hg.mozilla.org/mozilla-central/rev/497e8e4a5e91