Bug 1474557 - Prevent ExtensionStorageIDB and child/ext-storage from caching a stale or rejected selectBackend promise. r=mixedpuppy
authorLuca Greco <lgreco@mozilla.com>
Mon, 09 Jul 2018 22:35:12 +0200
changeset 426586 18c5ad6ddb9e
parent 426585 75ee38b6550f
child 426587 d2b1558a677c
push id34276
push userncsoregi@mozilla.com
push date2018-07-14 09:41 +0000
treeherdermozilla-central@04dd259d71db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmixedpuppy
bugs1474557
milestone63.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 1474557 - Prevent ExtensionStorageIDB and child/ext-storage from caching a stale or rejected selectBackend promise. r=mixedpuppy MozReview-Commit-ID: Kgwtm7QXW9o
toolkit/components/extensions/ExtensionStorageIDB.jsm
toolkit/components/extensions/child/ext-storage.js
toolkit/components/extensions/test/xpcshell/test_ext_storage_tab.js
--- a/toolkit/components/extensions/ExtensionStorageIDB.jsm
+++ b/toolkit/components/extensions/ExtensionStorageIDB.jsm
@@ -367,45 +367,47 @@ this.ExtensionStorageIDB = {
    */
   selectBackend(context) {
     const {extension} = context;
 
     if (!this.selectedBackendPromises.has(extension)) {
       let promise;
 
       if (context.childManager) {
-        // Create a promise object that is not tied to the current extension context, because
-        // we are caching it for the entire life of the extension in the current process (and
-        // the promise returned by context.childManager.callParentAsyncFunction would become
-        // a dead object when the context.cloneScope has been destroyed).
-        promise = (async () => {
-          // Ask the parent process if the new backend is enabled for the
-          // running extension.
-          let result = await context.childManager.callParentAsyncFunction(
-            "storage.local.IDBBackend.selectBackend", []
-          );
+        return context.childManager.callParentAsyncFunction(
+          "storage.local.IDBBackend.selectBackend", []
+        ).then(parentResult => {
+          let result;
 
-          if (!result.backendEnabled) {
-            return {backendEnabled: false};
+          if (!parentResult.backendEnabled) {
+            result = {backendEnabled: false};
+          } else {
+            result = {
+              ...parentResult,
+              // In the child process, we need to deserialize the storagePrincipal
+              // from the StructuredCloneHolder used to send it across the processes.
+              storagePrincipal: parentResult.storagePrincipal.deserialize(this),
+            };
           }
 
-          return {
-            ...result,
-            // In the child process, we need to deserialize the storagePrincipal
-            // from the StructuredCloneHolder used to send it across the processes.
-            storagePrincipal: result.storagePrincipal.deserialize(this),
-          };
-        })();
+          // Cache the result once we know that it has been resolved. The promise returned by
+          // context.childManager.callParentAsyncFunction will be dead when context.cloneScope
+          // is destroyed. To keep a promise alive in the cache, we wrap the result in an
+          // independent promise.
+          this.selectedBackendPromises.set(extension, Promise.resolve(result));
+
+          return result;
+        });
+      }
+
+      // If migrating to the IDB backend is not enabled by the preference, then we
+      // don't need to migrate any data and the new backend is not enabled.
+      if (!this.isBackendEnabled) {
+        promise = Promise.resolve({backendEnabled: false});
       } else {
-        // If migrating to the IDB backend is not enabled by the preference, then we
-        // don't need to migrate any data and the new backend is not enabled.
-        if (!this.isBackendEnabled) {
-          return Promise.resolve({backendEnabled: false});
-        }
-
         // In the main process, lazily create a storagePrincipal isolated in a
         // reserved user context id (its purpose is ensuring that the IndexedDB storage used
         // by the browser.storage.local API is not directly accessible from the extension code).
         const storagePrincipal = this.getStoragePrincipal(extension);
 
         // Serialize the nsIPrincipal object into a StructuredCloneHolder related to the privileged
         // js global, ready to be sent to the child processes.
         const serializedPrincipal = new StructuredCloneHolder(storagePrincipal, this);
--- a/toolkit/components/extensions/child/ext-storage.js
+++ b/toolkit/components/extensions/child/ext-storage.js
@@ -167,17 +167,21 @@ this.storage = class extends ExtensionAP
 
     // Generate the backend-agnostic local API wrapped methods.
     const local = {};
     for (let method of ["get", "set", "remove", "clear"]) {
       local[method] = async function(...args) {
         if (!promiseStorageLocalBackend) {
           promiseStorageLocalBackend = getStorageLocalBackend();
         }
-        const backend = await promiseStorageLocalBackend;
+        const backend = await promiseStorageLocalBackend.catch(err => {
+          // Clear the cached promise if it has been rejected.
+          promiseStorageLocalBackend = null;
+          throw err;
+        });
         return backend[method](...args);
       };
     }
 
     return {
       storage: {
         local,
 
--- a/toolkit/components/extensions/test/xpcshell/test_ext_storage_tab.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_storage_tab.js
@@ -95,8 +95,88 @@ add_task(async function test_storage_loc
   return runWithPrefs([[ExtensionStorageIDB.BACKEND_ENABLED_PREF, false]],
                       test_multiple_pages);
 });
 
 add_task(async function test_storage_local_idb_backend_from_tab() {
   return runWithPrefs([[ExtensionStorageIDB.BACKEND_ENABLED_PREF, true]],
                       test_multiple_pages);
 });
+
+async function test_storage_local_call_from_destroying_context() {
+  let extension = ExtensionTestUtils.loadExtension({
+    async background() {
+      browser.test.onMessage.addListener(async ({msg, values}) => {
+        switch (msg) {
+          case "storage-set": {
+            await browser.storage.local.set(values);
+            browser.test.sendMessage("storage-set:done");
+            break;
+          }
+          case "storage-get": {
+            const res = await browser.storage.local.get();
+            browser.test.sendMessage("storage-get:done", res);
+            break;
+          }
+          default:
+            browser.test.fail(`Received unexpected message: ${msg}`);
+        }
+      });
+
+      browser.test.sendMessage("ext-page-url", browser.runtime.getURL("tab.html"));
+    },
+    files: {
+      "tab.html": `<!DOCTYPE html>
+        <html>
+          <head>
+            <meta charset="utf-8">
+            <script src="tab.js"></script>
+          </head>
+        </html>`,
+
+      "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(() => {
+          // This call should never be reached (because the tab should have been
+          // 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).
+        window.location = "about:blank";
+      },
+    },
+    manifest: {
+      permissions: ["storage"],
+    },
+  });
+
+  await extension.startup();
+  const url = await extension.awaitMessage("ext-page-url");
+
+  let contentPage = await ExtensionTestUtils.loadContentPage(url, {extension});
+  let expectedData = {"test-key": "test-value"};
+
+  info("Call storage.local.set from the background page and wait it to be completed");
+  extension.sendMessage({msg: "storage-set", values: expectedData});
+  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");
+
+  contentPage.close();
+
+  await extension.unload();
+}
+
+add_task(async function test_storage_local_file_backend_destroyed_context_promise() {
+  return runWithPrefs([[ExtensionStorageIDB.BACKEND_ENABLED_PREF, false]],
+                      test_storage_local_call_from_destroying_context);
+});
+
+add_task(async function test_storage_local_idb_backend_destroyed_context_promise() {
+  return runWithPrefs([[ExtensionStorageIDB.BACKEND_ENABLED_PREF, true]],
+                      test_storage_local_call_from_destroying_context);
+});