Bug 1474557 - Prevent ExtensionStorageIDB and child/ext-storage from caching a stale or rejected selectBackend promise. r=mixedpuppy

MozReview-Commit-ID: Kgwtm7QXW9o

--HG--
extra : rebase_source : dc28d2f622876a359592d8e44a6cf449237acc30

toolkit/components/extensions/ExtensionStorageIDB.jsm

@@ -372,35 +372,37 @@ this.ExtensionStorageIDB = {
       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),
-          };
-        })();
-      } 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});
-        }
+          // 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 {
         // 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).

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

@@ -172,7 +172,11 @@ this.storage = class extends ExtensionAPI {
         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);
       };
     }

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

@@ -100,3 +100,83 @@ 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);
+});