Closed
Bug 786301
Opened 12 years ago
Closed 12 years ago
Delete localstorage related to an app when uninstalled
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
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)
(deleted),
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•12 years ago
|
||
This sounds like a privacy-related issue so let's block on it.
blocking-basecamp: ? → +
Updated•12 years ago
|
Whiteboard: [LOE:S]
Comment 3•12 years ago
|
||
sure, will need more details
who should I talk to ?
Updated•12 years ago
|
Whiteboard: [LOE:S] → [LOE:S][WebAPI:P1]
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee: Jan.Varga → mounir
Assignee | ||
Comment 4•12 years ago
|
||
Add a method to remove all data in persist DB related to an app.
Attachment #663684 -
Flags: review?
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #663685 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #663686 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•12 years ago
|
Attachment #663684 -
Flags: review? → review?(honzab.moz)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [LOE:S][WebAPI:P1] → [LOE:S][WebAPI:P1][needs review]
Assignee | ||
Comment 7•12 years ago
|
||
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•12 years ago
|
||
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 9•12 years ago
|
||
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+
Assignee | ||
Comment 10•12 years ago
|
||
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 11•12 years ago
|
||
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 12•12 years ago
|
||
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-
Assignee | ||
Comment 13•12 years ago
|
||
Tests are more verbose now.
Attachment #663686 -
Attachment is obsolete: true
Attachment #665455 -
Flags: review?(honzab.moz)
Comment 14•12 years ago
|
||
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+
Assignee | ||
Comment 15•12 years ago
|
||
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: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [LOE:S][WebAPI:P1][needs review] → [LOE:S][WebAPI:P1]
Target Milestone: --- → mozilla18
Updated•12 years ago
|
Whiteboard: [LOE:S][WebAPI:P1] → [LOE:S][WebAPI:P1][qa-]
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•