Bug 1474557 - Prevent ExtensionStorageIDB and child/ext-storage from caching a stale or rejected selectBackend promise. r=mixedpuppy a=lizzard
authorLuca Greco <lgreco@mozilla.com>
Mon, 09 Jul 2018 22:35:12 +0200
changeset 478022 7fa538f4814b2e18309f4a1c0c092fbef2d2fcf2
parent 478021 6fffc0b497a3b306d929f7ed8221ac648748f5c0
child 478023 52e3d099d0632f231688d9b3142cb50a72d1aa2a
push id9500
push userarchaeopteryx@coole-files.de
push dateThu, 19 Jul 2018 07:13:35 +0000
treeherdermozilla-beta@9573e5a35f1c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmixedpuppy, lizzard
bugs1474557
milestone62.0
Bug 1474557 - Prevent ExtensionStorageIDB and child/ext-storage from caching a stale or rejected selectBackend promise. r=mixedpuppy a=lizzard 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);
+});