Bug 1522918 - Fully remove optional permission data upon uninstall r=rpl
authorRob Wu <rob@robwu.nl>
Tue, 12 Feb 2019 14:04:39 +0000
changeset 459067 5542b48b6dfe
parent 459066 84d60f483f42
child 459068 788e4338d31f
push id111913
push usershindli@mozilla.com
push dateThu, 14 Feb 2019 05:01:59 +0000
treeherdermozilla-inbound@a0752d7e8073 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrpl
bugs1522918
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1522918 - Fully remove optional permission data upon uninstall r=rpl Differential Revision: https://phabricator.services.mozilla.com/D18217
toolkit/components/extensions/ExtensionParent.jsm
toolkit/components/extensions/ExtensionPermissions.jsm
toolkit/components/extensions/test/xpcshell/test_ext_permissions_uninstall.js
--- a/toolkit/components/extensions/ExtensionParent.jsm
+++ b/toolkit/components/extensions/ExtensionParent.jsm
@@ -1776,18 +1776,19 @@ class CacheStore {
     let [store] = await this.getStore();
 
     return new Map(store);
   }
 
   async delete(path) {
     let [store, key] = await this.getStore(path);
 
-    store.delete(key);
-    StartupCache.save();
+    if (store.delete(key)) {
+      StartupCache.save();
+    }
   }
 }
 
 for (let name of StartupCache.STORE_NAMES) {
   StartupCache[name] = new CacheStore(name);
 }
 
 var ExtensionParent = {
--- a/toolkit/components/extensions/ExtensionPermissions.jsm
+++ b/toolkit/components/extensions/ExtensionPermissions.jsm
@@ -58,27 +58,36 @@ var ExtensionPermissions = {
   },
 
   async _get(extensionId) {
     await lazyInit();
 
     let perms = prefs.data[extensionId];
     if (!perms) {
       perms = emptyPermissions();
-      prefs.data[extensionId] = perms;
     }
 
     return perms;
   },
 
   async _getCached(extensionId) {
     return StartupCache.permissions.get(extensionId,
                                         () => this._get(extensionId));
   },
 
+  /**
+   * Retrieves the optional permissions for the given extension.
+   * The information may be retrieved from the StartupCache, and otherwise fall
+   * back to data from the disk (and cache the result in the StartupCache).
+   *
+   * @param {string} extensionId The extensionId
+   * @returns {object} An object with "permissions" and "origins" array.
+   *   The object may be a direct reference to the storage or cache, so its
+   *   value should immediately be used and not be modified by callers.
+   */
   get(extensionId) {
     return this._getCached(extensionId);
   },
 
   /**
    * Add new permissions for the given extension.  `permissions` is
    * in the format that is passed to browser.permissions.request().
    *
@@ -149,20 +158,20 @@ var ExtensionPermissions = {
       this._saveSoon(extensionId);
       if (emitter) {
         emitter.emit("remove-permissions", removed);
       }
     }
   },
 
   async removeAll(extensionId) {
-    let perms = await this._getCached(extensionId);
-
-    if (perms.permissions.length || perms.origins.length) {
-      Object.assign(perms, emptyPermissions());
+    await lazyInit();
+    StartupCache.permissions.delete(extensionId);
+    if (prefs.data[extensionId]) {
+      delete prefs.data[extensionId];
       prefs.saveSoon();
     }
   },
 
   // This is meant for tests only
   async _uninit() {
     if (!_initPromise) {
       return;
--- a/toolkit/components/extensions/test/xpcshell/test_ext_permissions_uninstall.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_permissions_uninstall.js
@@ -12,16 +12,50 @@ const observer = {
   observe(subject, topic, data) {
     if (topic == "webextension-optional-permission-prompt") {
       let {resolve} = subject.wrappedJSObject;
       resolve(true);
     }
   },
 };
 
+// Look up the cached permissions, if any.
+async function getCachedPermissions(extensionId) {
+  const NotFound = Symbol("extension ID not found in permissions cache");
+  try {
+    return await ExtensionParent.StartupCache.permissions.get(extensionId, () => {
+      // Throw error to prevent the key from being created.
+      throw NotFound;
+    });
+  } catch (e) {
+    if (e === NotFound) {
+      return null;
+    }
+    throw e;
+  }
+}
+
+// Look up the permissions from the file. Internal methods are used to avoid
+// inadvertently changing the permissions in the cache or JSON file.
+async function getStoredPermissions(extensionId) {
+  // The two _get calls that follow are expected to return the same object if
+  // the entry exists in the JSON file, otherwise we expect two different
+  // objects (with the same default properties).
+  let perms1 = await ExtensionPermissions._get(extensionId);
+  let perms2 = await ExtensionPermissions._get(extensionId);
+  if (perms1 === perms2) {
+    // There is an entry in the file.
+    return perms1;
+  }
+  // Sanity check: The returned object should be empty.
+  Assert.deepEqual(perms1, perms2, "Expected same permission values");
+  Assert.deepEqual(perms1, {origins: [], permissions: []}, "Expected empty permissions");
+  return null;
+}
+
 add_task(async function setup() {
   Services.prefs.setBoolPref("extensions.webextOptionalPermissionPrompts", true);
   Services.obs.addObserver(observer, "webextension-optional-permission-prompt");
   await AddonTestUtils.promiseStartupManager();
   registerCleanupFunction(async () => {
     await AddonTestUtils.promiseShutdownManager();
     Services.obs.removeObserver(observer, "webextension-optional-permission-prompt");
     Services.prefs.clearUserPref("extensions.webextOptionalPermissionPrompts");
@@ -57,14 +91,36 @@ add_task(async function test_permissions
     let result = await extension.awaitMessage("request.result");
     equal(result, true, "request() for optional permissions succeeded");
   });
 
   let id = extension.id;
   let perms = await ExtensionPermissions.get(id);
   equal(perms.permissions.length, 1, "optional permission added");
 
+  Assert.deepEqual(await getCachedPermissions(id), {
+    permissions: ["idle"],
+    origins: [],
+  }, "Optional permission added to cache");
+  Assert.deepEqual(await getStoredPermissions(id), {
+    permissions: ["idle"],
+    origins: [],
+  }, "Optional permission added to persistent file");
+
   await extension.unload();
 
+  // Directly read from the internals instead of using ExtensionPermissions.get,
+  // because the latter will lazily cache the extension ID.
+  Assert.deepEqual(await getCachedPermissions(id), null, "Cached permissions removed");
+  Assert.deepEqual(await getStoredPermissions(id), null, "Stored permissions removed");
+
   perms = await ExtensionPermissions.get(id);
   equal(perms.permissions.length, 0, "no permissions after uninstall");
   equal(perms.origins.length, 0, "no origin permissions after uninstall");
+
+  // The public ExtensionPermissions.get method should not store (empty)
+  // permissions in the persistent JSON file. Polluting the cache is not ideal,
+  // but acceptable since the cache will eventually be cleared, and non-test
+  // code is not likely to call ExtensionPermissions.get() for non-installed
+  // extensions anyway.
+  Assert.deepEqual(await getCachedPermissions(id), perms, "Permissions cached");
+  Assert.deepEqual(await getStoredPermissions(id), null, "Permissions not saved");
 });