Closed Bug 786296 Opened 9 years ago Closed 9 years ago

Delete permissions related to an app when uninstalled

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp -

People

(Reporter: mounir, Assigned: mounir)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch Patch (obsolete) — Splinter Review
Attachment #656022 - Flags: review?(jonas)
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.
Attachment #656022 - Flags: review?(fabrice)
Attached patch PatchSplinter Review
Attachment #656022 - Attachment is obsolete: true
Attachment #656022 - Flags: review?(jonas)
Attachment #656022 - Flags: review?(fabrice)
Attachment #656079 - Flags: review?(jonas)
Attachment #656079 - Flags: review?(fabrice)
Attachment #656079 - Flags: review?(fabrice) → review+
Comment on attachment 656079 [details] [diff] [review]
Patch

Asking Justin to do the review in case of he can do it before Jonas.
Attachment #656079 - Flags: review?(justin.lebar+bug)
Comment on attachment 656079 [details] [diff] [review]
Patch

Looking at this...
Attachment #656079 - Flags: review?(jonas)
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.
Attachment #656079 - Flags: review?(justin.lebar+bug) → review+
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!
blocking-basecamp: ? → -
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4eaa2264afe
Flags: in-testsuite+
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/b4eaa2264afe
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.