Comment on attachment 656022 [details] [diff] [review] Patch Fabrice, could you review the dom/apps/ and dom/interfaces/apps/ changes? I tried to keep them as less invasive as possible so we can merge easily with your pending changes.

Comment on attachment 656079 [details] [diff] [review] Patch Asking Justin to do the review in case of he can do it before Jonas.

Comment on attachment 656079 [details] [diff] [review] Patch Looking at this...

Comment on attachment 656079 [details] [diff] [review] Patch >diff --git a/extensions/cookie/nsPermissionManager.cpp b/extensions/cookie/nsPermissionManager.cpp >--- a/extensions/cookie/nsPermissionManager.cpp >+++ b/extensions/cookie/nsPermissionManager.cpp >@@ -19,18 +19,21 @@ > #include "nsAppDirectoryServiceDefs.h" > #include "prprf.h" > #include "mozilla/storage.h" > #include "mozilla/Attributes.h" > #include "nsXULAppAPI.h" > #include "nsIPrincipal.h" > #include "nsContentUtils.h" > #include "nsIScriptSecurityManager.h" >+#include "nsIAppsService.h" >+#include "mozIApplication.h" > > static nsPermissionManager *gPermissionManager = nullptr; >+nsCOMPtr<nsPermissionManager::AppUninstallObserver> nsPermissionManager::sAppUninstallObserver = nullptr; StaticRefPtr, please. Although it's not clear to me why you have to do it this way. Why not just let the observer service keep a strong ref to the object? Then you don't need this at all. Or alternatively, nsPermissionManager itself is an nsIObserver. Could you add the uninstall observer directly on nsPermissionManager? >@@ -234,16 +237,49 @@ NS_IMETHODIMP DeleteFromMozHostListener: >+// AppUninstallObserver implementation >+nsPermissionManager::AppUninstallObserver::AppUninstallObserver() >+{ >+ nsCOMPtr<nsIObserverService> observerService = do_GetService("@mozilla.org/observer-service;1"); >+ NS_ASSERTION(observerService, "No observer service!?"); If we're going to crash on the next line, I don't think you need to assert. :) >+ observerService->AddObserver(this, "webapps-uninstall", false); Would you mind mollifying my pet peeve by doing (this, "webapps-uninstall", /* holdsWeak = */ false)? http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html >+NS_IMETHODIMP >+nsPermissionManager::AppUninstallObserver::Observe(nsISupports *aSubject, const char *aTopic, const PRUnichar *someData) s/someData/aData/? >+{ >+ MOZ_ASSERT(!nsCRT::strcmp(aTopic, "webapps-uninstall")); >+ >+ nsCOMPtr<nsIPermissionManager> permManager = do_GetService("@mozilla.org/permissionmanager;1"); >+ MOZ_ASSERT(permManager); I don't think this is particularly useful, since dereferencing a null pointer will cause us to crash. >+ nsCOMPtr<nsIAppsService> appsService = do_GetService("@mozilla.org/AppsService;1"); >+ MOZ_ASSERT(appsService); Ditto. >+ nsCOMPtr<mozIApplication> app; >+ appsService->GetAppFromObserverMessage(nsAutoString(someData), getter_AddRefs(app)); >+ NS_ENSURE_TRUE(app, NS_ERROR_UNEXPECTED); >+ >+ uint32_t appId; >+ app->GetLocalId(&appId); >+ >+ return permManager->RemovePermissionsForApp(appId); >+} >+ >+NS_IMPL_ISUPPORTS1(nsPermissionManager::AppUninstallObserver, nsIObserver) >@@ -1066,16 +1102,91 @@ NS_IMETHODIMP nsPermissionManager::Obser >+NS_IMETHODIMP >+nsPermissionManager::RemovePermissionsForApp(uint32_t aAppId) >+{ >+ ENSURE_NOT_CHILD_PROCESS; >+ >+ // We begin by removing all the permissions from the DB. >+ // This is not using a mozIStorageStatement because removing an app should be >+ // rare enough to not have to worry too much about performance. >+ // After clearing the DB, we are calling AddInternal() to make sure that all >+ // processes are aware of that change and the representation of the DB in >+ // memory is updated. s/are calling/call "to make sure that all processes are aware of /this/ change and /update their in-memory copy of the data/." >+ nsCAutoString sql; >+ sql.AppendLiteral("DELETE FROM moz_hosts WHERE appId="); >+ sql.AppendInt(aAppId); >+ >+ nsresult rv = mDBConn->ExecuteSimpleSQL(sql); >+ NS_ENSURE_SUCCESS(rv, rv); I'm concerned about doing sync I/O here, mainly because this will freeze the whole phone until it completes, right? And the phones have super-slow I/O systems? >+ GetPermissionsForAppStruct data(aAppId); >+ mPermissionTable.EnumerateEntries(GetPermissionsForApp, &data); >+ >+ for (int32_t i=0; i<data.permissions.Count(); ++i) { Could you add a comment indicating that you're doing it this way because AddInternal modifies mPermissionTable, so we can't call it during EnumerateEntries? >+ nsCAutoString host; >+ bool isInBrowserElement; >+ nsCAutoString type; >+ >+ data.permissions[i]->GetHost(host); >+ data.permissions[i]->GetIsInBrowserElement(&isInBrowserElement); >+ data.permissions[i]->GetType(type); >+ >+ nsCOMPtr<nsIPrincipal> principal; >+ if (NS_FAILED(GetPrincipal(host, aAppId, isInBrowserElement, >+ getter_AddRefs(principal)))) { >+ NS_ERROR("GetPrincipal() failed!"); >+ continue; >+ } >+ >+ gPermissionManager->AddInternal(principal, Isn't gPermissionManager == this? >+ type, >+ nsIPermissionManager::UNKNOWN_ACTION, >+ 0, >+ nsIPermissionManager::EXPIRE_NEVER, >+ 0, >+ nsPermissionManager::eNotify, >+ nsPermissionManager::eNoDBOperation); >diff --git a/extensions/cookie/nsPermissionManager.h b/extensions/cookie/nsPermissionManager.h >@@ -185,17 +186,34 @@ public: > const nsAFlatCString &aType, > uint32_t aPermission, > int64_t aID, > uint32_t aExpireType, > int64_t aExpireTime, > NotifyOperationType aNotifyOperation, > DBOperationType aDBOperation); > >+ static void AppUninstallObserverInit() { >+ sAppUninstallObserver = new AppUninstallObserver(); >+ } Nit: In the name of loose coupling, it's probably better to call this StaticInit() or something -- the code invoking this method shouldn't care exactly what it's doing. >+ static void AppUninstallObserverShutdown() { >+ sAppUninstallObserver = nullptr; >+ } If you get rid of the static pointer, you don't need this method. > private: >+ class AppUninstallObserver : public nsIObserver { >+ public: >+ NS_DECL_ISUPPORTS >+ NS_DECL_NSIOBSERVER >+ >+ AppUninstallObserver(); >+ }; If you get rid of the static pointer, you probably don't need to declare this in the header. >diff --git a/extensions/cookie/test/test_app_uninstall.html b/extensions/cookie/test/test_app_uninstall.html I didn't look at this, but OK. I guess these are mostly nits, so r=me.

This doesn't seem like something we would hold a release for, as compared to the other clearing bugs where there's privacy sensitive data that must be cleared. But, this looks ready to land, so we'll likely get this either way!