Closed Bug 786301 Opened 7 years ago Closed 7 years ago

Delete localstorage related to an app when uninstalled

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: mounir, Assigned: mounir)

References

(Depends on 1 open bug)

Details

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

Attachments

(3 files, 2 obsolete files)

No description provided.
This sounds like a privacy-related issue so let's block on it.
blocking-basecamp: ? → +
Whiteboard: [LOE:S]
Jan, can you give us a hand with this bug?
Assignee: nobody → Jan.Varga
sure, will need more details
who should I talk to ?
Whiteboard: [LOE:S] → [LOE:S][WebAPI:P1]
Keywords: feature
Status: NEW → ASSIGNED
Depends on: 773373
Assignee: Jan.Varga → mounir
Add a method to remove all data in persist DB related to an app.
Attachment #663684 - Flags: review?
Attachment #663685 - Flags: review?(justin.lebar+bug)
Attached patch Part 3 - Tests (obsolete) — Splinter Review
Attachment #663686 - Flags: review?(honzab.moz)
Attachment #663684 - Flags: review? → review?(honzab.moz)
Whiteboard: [LOE:S][WebAPI:P1] → [LOE:S][WebAPI:P1][needs review]
Those patches pass try:
https://tbpl.mozilla.org/?tree=Try&rev=329a70c7ec9d

(the errors you can see are from patches in bug 773373)
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!
Attachment #663685 - Flags: review?(justin.lebar+bug) → review-
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.
Attachment #663684 - Flags: review?(honzab.moz) → review+
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.
Attachment #663685 - Attachment is obsolete: true
Attachment #664920 - Flags: review?(justin.lebar+bug)
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.
Attachment #664920 - Flags: review?(justin.lebar+bug) → review+
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.
Attachment #663686 - Flags: review?(honzab.moz) → review-
Attached patch Part 3 - TestsSplinter Review
Tests are more verbose now.
Attachment #663686 - Attachment is obsolete: true
Attachment #665455 - Flags: review?(honzab.moz)
Comment on attachment 665455 [details] [diff] [review]
Part 3 - Tests

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

Thanks!

r=honzab.
Attachment #665455 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/2310fb3518ee
https://hg.mozilla.org/mozilla-central/rev/205fdb51a6e3
https://hg.mozilla.org/mozilla-central/rev/497e8e4a5e91
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [LOE:S][WebAPI:P1][needs review] → [LOE:S][WebAPI:P1]
Target Milestone: --- → mozilla18
Whiteboard: [LOE:S][WebAPI:P1] → [LOE:S][WebAPI:P1][qa-]
Depends on: 794969
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.