Bug 1477015 - Select storage.local backend on startup when the extension is not migrating its data. r=aswan,mixedpuppy

MozReview-Commit-ID: WzW2bFlYNg

--HG--
extra : rebase_source : 048dbd36e6bf1bfc64d02e11bf26af1392071139

toolkit/components/extensions/Extension.jsm

@@ -44,6 +44,7 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   ContextualIdentityService: "resource://gre/modules/ContextualIdentityService.jsm",
   ExtensionPermissions: "resource://gre/modules/ExtensionPermissions.jsm",
   ExtensionStorage: "resource://gre/modules/ExtensionStorage.jsm",
+  ExtensionStorageIDB: "resource://gre/modules/ExtensionStorageIDB.jsm",
   ExtensionTestCommon: "resource://testing-common/ExtensionTestCommon.jsm",
   FileSource: "resource://gre/modules/L10nRegistry.jsm",
   L10nRegistry: "resource://gre/modules/L10nRegistry.jsm",
@@ -147,7 +148,6 @@ const LOGGER_ID_BASE = "addons.webextension.";
 const UUID_MAP_PREF = "extensions.webextensions.uuids";
 const LEAVE_STORAGE_PREF = "extensions.webextensions.keepStorageOnUninstall";
 const LEAVE_UUID_PREF = "extensions.webextensions.keepUuidOnUninstall";
-const IDB_MIGRATED_PREF_BRANCH = "extensions.webextensions.ExtensionStorageIDB.migrated";
 
 const COMMENT_REGEXP = new RegExp(String.raw`
     ^
@@ -252,8 +252,7 @@ var UninstallObserver = {
       });
       Services.qms.clearStoragesForPrincipal(storagePrincipal);
 
-      // Clear the preference set for the extensions migrated to the IDBBackend.
+      ExtensionStorageIDB.clearMigratedExtensionPref(addon.id);
-      Services.prefs.clearUserPref(`${IDB_MIGRATED_PREF_BRANCH}.${addon.id}`);
 
       // Clear localStorage created by the extension
       let storage = Services.domStorageManager.getStorage(null, principal);
@@ -1773,6 +1772,23 @@ class Extension extends ExtensionData {
 
       this.updatePermissions(this.startupReason);
 
+      // Select the storage.local backend if it is already known,
+      // and start the data migration if needed.
+      if (this.hasPermission("storage")) {
+        if (!ExtensionStorageIDB.isBackendEnabled) {
+          this.setSharedData("storageIDBBackend", false);
+        } else if (ExtensionStorageIDB.isMigratedExtension(this)) {
+          this.setSharedData("storageIDBBackend", true);
+          this.setSharedData("storageIDBPrincipal", ExtensionStorageIDB.getStoragePrincipal(this));
+        } else {
+          // If the extension has to migrate backend, ensure that the data migration
+          // starts once Firefox is idle after the extension has been started.
+          this.once("ready", () => ChromeUtils.idleDispatch(() => {
+            ExtensionStorageIDB.selectBackend({extension: this});
+          }));
+        }
+      }
+
       // The "startup" Management event sent on the extension instance itself
       // is emitted just before the Management "startup" event,
       // and it is used to run code that needs to be executed before

toolkit/components/extensions/ExtensionStorageIDB.jsm

@@ -373,8 +373,7 @@ async function migrateJSONFileData(extension, storagePrincipal) {
   let dataMigrateCompleted = false;
   let hasOldData = false;
 
-  const isMigratedExtension = Services.prefs.getBoolPref(`${IDB_MIGRATED_PREF_BRANCH}.${extension.id}`, false);
+  if (ExtensionStorageIDB.isMigratedExtension(extension)) {
-  if (isMigratedExtension) {
     return;
   }
 
@@ -387,7 +386,7 @@ async function migrateJSONFileData(extension, storagePrincipal) {
       // there is no "going back": any data that has not been migrated will be still on disk
       // but it is not going to be migrated anymore, it could be eventually used to allow
       // a user to manually retrieve the old data file).
-      Services.prefs.setBoolPref(`${IDB_MIGRATED_PREF_BRANCH}.${extension.id}`, true);
+      ExtensionStorageIDB.setMigratedExtensionPref(extension, true);
       return;
     }
   } catch (err) {
@@ -485,7 +484,7 @@ async function migrateJSONFileData(extension, storagePrincipal) {
     }
   }
 
-  Services.prefs.setBoolPref(`${IDB_MIGRATED_PREF_BRANCH}.${extension.id}`, true);
+  ExtensionStorageIDB.setMigratedExtensionPref(extension, true);
 
   DataMigrationTelemetry.recordResult({
     backend: "IndexedDB",
@@ -521,6 +520,18 @@ this.ExtensionStorageIDB = {
     XPCOMUtils.defineLazyPreferenceGetter(this, "isBackendEnabled", BACKEND_ENABLED_PREF, false);
   },
 
+  isMigratedExtension(extension) {
+    return Services.prefs.getBoolPref(`${IDB_MIGRATED_PREF_BRANCH}.${extension.id}`, false);
+  },
+
+  setMigratedExtensionPref(extension, val) {
+    Services.prefs.setBoolPref(`${IDB_MIGRATED_PREF_BRANCH}.${extension.id}`, !!val);
+  },
+
+  clearMigratedExtensionPref(extensionId) {
+    Services.prefs.clearUserPref(`${IDB_MIGRATED_PREF_BRANCH}.${extensionId}`);
+  },
+
   getStoragePrincipal(extension) {
     return extension.createPrincipal(extension.baseURI, {
       userContextId: WEBEXT_STORAGE_USER_CONTEXT_ID,

toolkit/components/extensions/child/ext-storage.js

@@ -4,6 +4,8 @@ ChromeUtils.defineModuleGetter(this, "ExtensionStorage",
                                "resource://gre/modules/ExtensionStorage.jsm");
 ChromeUtils.defineModuleGetter(this, "ExtensionStorageIDB",
                                "resource://gre/modules/ExtensionStorageIDB.jsm");
+ChromeUtils.defineModuleGetter(this, "Services",
+                               "resource://gre/modules/Services.jsm");
 ChromeUtils.defineModuleGetter(this, "TelemetryStopwatch",
                                "resource://gre/modules/TelemetryStopwatch.jsm");
 
@@ -55,7 +57,7 @@ this.storage = class extends ExtensionAPI {
     };
   }
 
-  getLocalIDBBackend(context, {hasParentListeners, serialize, storagePrincipal}) {
+  getLocalIDBBackend(context, {fireOnChanged, serialize, storagePrincipal}) {
     let dbPromise;
     async function getDB() {
       if (dbPromise) {
@@ -86,14 +88,8 @@ this.storage = class extends ExtensionAPI {
             serialize: ExtensionStorage.serialize,
           });
 
-          if (!changes) {
+          if (changes) {
-            return;
+            fireOnChanged(changes);
-          }
-
-          const hasListeners = await hasParentListeners();
-          if (hasListeners) {
-            await context.childManager.callParentAsyncFunction(
-              "storage.local.IDBBackend.fireOnChanged", [changes]);
           }
         });
       },
@@ -101,34 +97,23 @@ this.storage = class extends ExtensionAPI {
         const db = await getDB();
         const changes = await db.remove(keys);
 
-        if (!changes) {
+        if (changes) {
-          return;
+          fireOnChanged(changes);
-        }
-
-        const hasListeners = await hasParentListeners();
-        if (hasListeners) {
-          await context.childManager.callParentAsyncFunction(
-            "storage.local.IDBBackend.fireOnChanged", [changes]);
         }
       },
       async clear() {
         const db = await getDB();
         const changes = await db.clear(context.extension);
 
-        if (!changes) {
+        if (changes) {
-          return;
+          fireOnChanged(changes);
-        }
-
-        const hasListeners = await hasParentListeners();
-        if (hasListeners) {
-          await context.childManager.callParentAsyncFunction(
-            "storage.local.IDBBackend.fireOnChanged", [changes]);
         }
       },
     };
   }
 
   getAPI(context) {
+    const {extension} = context;
     const serialize = ExtensionStorage.serializeForContext.bind(null, context);
     const deserialize = ExtensionStorage.deserializeForContext.bind(null, context);
 
@@ -152,9 +137,17 @@ this.storage = class extends ExtensionAPI {
       return sanitized;
     }
 
-    // Detect the actual storage.local enabled backend for the extension (as soon as the
+    function fireOnChanged(changes) {
-    // storage.local API has been accessed for the first time).
+      // This call is used (by the storage.local API methods for the IndexedDB backend) to fire a storage.onChanged event,
-    let promiseStorageLocalBackend;
+      // it uses the underlying message manager since the child context (or its ProxyContentParent counterpart
+      // running in the main process) may be gone by the time we call this, and so we can't use the childManager
+      // abstractions (e.g. callParentAsyncFunction or callParentFunctionNoReturn).
+      Services.cpmm.sendAsyncMessage(`Extension:StorageLocalOnChanged:${extension.uuid}`, changes);
+    }
+
+    // If the selected backend for the extension is not known yet, we have to lazily detect it
+    // by asking to the main process (as soon as the storage.local API has been accessed for
+    // the first time).
     const getStorageLocalBackend = async () => {
       const {
         backendEnabled,
@@ -167,33 +160,58 @@ this.storage = class extends ExtensionAPI {
 
       return this.getLocalIDBBackend(context, {
         storagePrincipal,
-        hasParentListeners() {
+        fireOnChanged,
-          // We spare a good amount of memory if there are no listeners around
-          // (e.g. because they have never been subscribed or they have been removed
-          // in the meantime).
-          return context.childManager.callParentAsyncFunction(
-            "storage.local.IDBBackend.hasListeners", []);
-        },
         serialize,
       });
     };
 
+    // Synchronously select the backend if it is already known.
+    let selectedBackend;
+
+    const useStorageIDBBackend = extension.getSharedData("storageIDBBackend");
+    if (useStorageIDBBackend === false) {
+      selectedBackend = this.getLocalFileBackend(context, {deserialize, serialize});
+    } else if (useStorageIDBBackend === true) {
+      selectedBackend = this.getLocalIDBBackend(context, {
+        storagePrincipal: extension.getSharedData("storageIDBPrincipal"),
+        fireOnChanged,
+        serialize,
+      });
+    }
+
+    let promiseStorageLocalBackend;
+
     // Generate the backend-agnostic local API wrapped methods.
     const local = {};
     for (let method of ["get", "set", "remove", "clear"]) {
       local[method] = async function(...args) {
         try {
-          if (!promiseStorageLocalBackend) {
+          // Discover the selected backend if it is not known yet.
-            promiseStorageLocalBackend = getStorageLocalBackend();
+          if (!selectedBackend) {
+            if (!promiseStorageLocalBackend) {
+              promiseStorageLocalBackend = getStorageLocalBackend().catch(err => {
+                // Clear the cached promise if it has been rejected.
+                promiseStorageLocalBackend = null;
+                throw err;
+              });
+            }
+
+            // If the storage.local method is not 'get' (which doesn't change any of the stored data),
+            // fall back to call the method in the parent process, so that it can be completed even
+            // if this context has been destroyed in the meantime.
+            if (method !== "get") {
+              // Let the outer try to catch rejections returned by the backend methods.
+              const result = await context.childManager.callParentAsyncFunction(
+                "storage.local.callMethodInParentProcess", [method, args]);
+              return result;
+            }
+
+            // Get the selected backend and cache it for the next API calls from this context.
+            selectedBackend = await promiseStorageLocalBackend;
           }
-          const backend = await promiseStorageLocalBackend.catch(err => {
-            // Clear the cached promise if it has been rejected.
-            promiseStorageLocalBackend = null;
-            throw err;
-          });
 
           // Let the outer try to catch rejections returned by the backend methods.
-          const result = await backend[method](...args);
+          const result = await selectedBackend[method](...args);
           return result;
         } catch (err) {
           // Ensure that the error we throw is converted into an ExtensionError

toolkit/components/extensions/parent/ext-storage.js

@@ -34,12 +34,52 @@ const lookupManagedStorage = async (extensionId, context) => {
 };
 
 this.storage = class extends ExtensionAPI {
+  constructor(extension) {
+    super(extension);
+
+    const messageName = `Extension:StorageLocalOnChanged:${extension.uuid}`;
+    Services.ppmm.addMessageListener(messageName, this);
+    this.clearStorageChangedListener = () => {
+      Services.ppmm.removeMessageListener(messageName, this);
+    };
+  }
+
+  onShutdown() {
+    const {clearStorageChangedListener} = this;
+    this.clearStorageChangedListener = null;
+
+    if (clearStorageChangedListener) {
+      clearStorageChangedListener();
+    }
+  }
+
+  receiveMessage({name, data}) {
+    if (name !== `Extension:StorageLocalOnChanged:${this.extension.uuid}`) {
+      return;
+    }
+
+    ExtensionStorageIDB.notifyListeners(this.extension.id, data);
+  }
+
   getAPI(context) {
     let {extension} = context;
 
     return {
       storage: {
         local: {
+          async callMethodInParentProcess(method, args) {
+            const res = await ExtensionStorageIDB.selectBackend({extension});
+            if (!res.backendEnabled) {
+              return ExtensionStorage[method](extension.id, ...args);
+            }
+
+            const db = await ExtensionStorageIDB.open(res.storagePrincipal.deserialize(this));
+            const changes = await db[method](...args);
+            if (changes) {
+              ExtensionStorageIDB.notifyListeners(extension.id, changes);
+            }
+            return changes;
+          },
           // Private storage.local JSONFile backend methods (used internally by the child
           // ext-storage.js module).
           JSONFileBackend: {
@@ -62,15 +102,6 @@ this.storage = class extends ExtensionAPI {
             selectBackend() {
               return ExtensionStorageIDB.selectBackend(context);
             },
-            hasListeners() {
-              return ExtensionStorageIDB.hasListeners(extension.id);
-            },
-            fireOnChanged(changes) {
-              ExtensionStorageIDB.notifyListeners(extension.id, changes);
-            },
-            onceDataMigrated() {
-              return ExtensionStorageIDB.onceDataMigrated(context);
-            },
           },
         },
 

toolkit/components/extensions/test/xpcshell/test_ext_storage_tab.js

@@ -104,6 +104,15 @@ add_task(async function test_storage_local_idb_backend_from_tab() {
 async function test_storage_local_call_from_destroying_context() {
   let extension = ExtensionTestUtils.loadExtension({
     async background() {
+      let numberOfChanges = 0;
+      browser.storage.onChanged.addListener((changes, areaName) => {
+        if (areaName !== "local") {
+          browser.test.fail(`Received unexpected storage changes for "${areaName}"`);
+        }
+
+        numberOfChanges++;
+      });
+
       browser.test.onMessage.addListener(async ({msg, values}) => {
         switch (msg) {
           case "storage-set": {
@@ -116,6 +125,10 @@ async function test_storage_local_call_from_destroying_context() {
             browser.test.sendMessage("storage-get:done", res);
             break;
           }
+          case "storage-changes": {
+            browser.test.sendMessage("storage-changes-count", numberOfChanges);
+            break;
+          }
           default:
             browser.test.fail(`Received unexpected message: ${msg}`);
         }
@@ -135,10 +148,8 @@ async function test_storage_local_call_from_destroying_context() {
       "tab.js"() {
         browser.test.log("Extension tab - calling storage.local API method");
         // Call the storage.local API from a tab that is going to be quickly closed.
-        browser.storage.local.get({}).then(() => {
+        browser.storage.local.set({
-          // This call should never be reached (because the tab should have been
+          "test-key-from-destroying-context": "testvalue2",
-          // destroyed in the meantime).
-          browser.test.fail("Extension tab - Unexpected storage.local promise resolved");
         });
         // Navigate away from the extension page, so that the storage.local API call will be unable
         // to send the call to the caller context (because it has been destroyed in the meantime).
@@ -154,17 +165,25 @@ async function test_storage_local_call_from_destroying_context() {
   const url = await extension.awaitMessage("ext-page-url");
 
   let contentPage = await ExtensionTestUtils.loadContentPage(url, {extension});
-  let expectedData = {"test-key": "test-value"};
+  let expectedBackgroundPageData = {"test-key-from-background-page": "test-value"};
+  let expectedTabData = {"test-key-from-destroying-context": "testvalue2"};
 
   info("Call storage.local.set from the background page and wait it to be completed");
-  extension.sendMessage({msg: "storage-set", values: expectedData});
+  extension.sendMessage({msg: "storage-set", values: expectedBackgroundPageData});
   await extension.awaitMessage("storage-set:done");
 
   info("Call storage.local.get from the background page and wait it to be completed");
   extension.sendMessage({msg: "storage-get"});
   let res = await extension.awaitMessage("storage-get:done");
 
-  Assert.deepEqual(res, expectedData, "Got the expected data set in the storage.local backend");
+  Assert.deepEqual(res, {
+    ...expectedBackgroundPageData,
+    ...expectedTabData,
+  }, "Got the expected data set in the storage.local backend");
+
+  extension.sendMessage({msg: "storage-changes"});
+  equal(await extension.awaitMessage("storage-changes-count"), 2,
+        "Got the expected number of storage.onChanged event received");
 
   contentPage.close();
 

toolkit/components/extensions/test/xpcshell/test_ext_storage_telemetry.js

@@ -14,6 +14,9 @@ const HISTOGRAM_IDB_IDS = [
 
 const HISTOGRAM_IDS = [].concat(HISTOGRAM_JSON_IDS, HISTOGRAM_IDB_IDS);
 
+const EXTENSION_ID1 = "@test-extension1";
+const EXTENSION_ID2 = "@test-extension2";
+
 async function test_telemetry_background() {
   const expectedEmptyHistograms = ExtensionStorageIDB.isBackendEnabled ?
           HISTOGRAM_JSON_IDS : HISTOGRAM_IDB_IDS;
@@ -32,16 +35,17 @@ async function test_telemetry_background() {
     browser.runtime.sendMessage("contentDone");
   }
 
-  let extInfo = {
+  let baseManifest = {
-    manifest: {
+    permissions: ["storage"],
-      permissions: ["storage"],
+    content_scripts: [
-      content_scripts: [
+      {
-        {
+        "matches": ["http://*/*/file_sample.html"],
-          "matches": ["http://*/*/file_sample.html"],
+        "js": ["content_script.js"],
-          "js": ["content_script.js"],
+      },
-        },
+    ],
-      ],
+  };
-    },
+
+  let baseExtInfo = {
     async background() {
       browser.runtime.onMessage.addListener(msg => {
         browser.test.sendMessage(msg);
@@ -56,8 +60,27 @@ async function test_telemetry_background() {
     },
   };
 
-  let extension1 = ExtensionTestUtils.loadExtension(extInfo);
+  let extInfo1 = {
-  let extension2 = ExtensionTestUtils.loadExtension(extInfo);
+    ...baseExtInfo,
+    manifest: {
+      ...baseManifest,
+      applications: {
+        gecko: {id: EXTENSION_ID1},
+      },
+    },
+  };
+  let extInfo2 = {
+    ...baseExtInfo,
+    manifest: {
+      ...baseManifest,
+      applications: {
+        gecko: {id: EXTENSION_ID2},
+      },
+    },
+  };
+
+  let extension1 = ExtensionTestUtils.loadExtension(extInfo1);
+  let extension2 = ExtensionTestUtils.loadExtension(extInfo2);
 
   clearHistograms();
 
@@ -129,6 +152,13 @@ add_task(function test_telemetry_background_file_backend() {
 });
 
 add_task(function test_telemetry_background_idb_backend() {
-  return runWithPrefs([[ExtensionStorageIDB.BACKEND_ENABLED_PREF, true]],
+  return runWithPrefs([
-                      test_telemetry_background);
+    [ExtensionStorageIDB.BACKEND_ENABLED_PREF, true],
+    // Set the migrated preference for the two test extension, because the
+    // first storage.local call fallbacks to run in the parent process when we
+    // don't know which is the selected backend during the extension startup
+    // and so we can't choose the telemetry histogram to use.
+    [`${ExtensionStorageIDB.IDB_MIGRATED_PREF_BRANCH}.${EXTENSION_ID1}`, true],
+    [`${ExtensionStorageIDB.IDB_MIGRATED_PREF_BRANCH}.${EXTENSION_ID2}`, true],
+  ], test_telemetry_background);
 });