Bug 1618059 - Extract the storage extension live reload test r=ochameau,davidwalsh

The failure only occurs locally when I use an attached target for the test_panel_live_reload mochitest.
And it only happens if we perform the `extension.upgrade` call during the test.
Most of the time it seems linked to a "frameUpdated" event fired when the webextension is being updated.
But even after commenting out the event, the test remains intermittent (albeit with a much lower frequency)

A first option would be to expose a new API on the webextension descriptor front in order to create
detached targets.

But it seems that isolating the live_reload test in a dedicated file also fixes the intermittent.
It makes the fix a bit obscure, and it probably means we won't look into the issue much furhter but
I would prefer to avoid test-only APIs in the codebase.

Differential Revision: https://phabricator.services.mozilla.com/D81322

devtools/server/tests/xpcshell/helper_test_extension_storage_actor.js

@@ -0,0 +1,214 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/* globals browser */
+
+"use strict";
+
+/**
+ * Test helpers shared by the test_extension_storage_actor* tests.
+ */
+
+const { FileUtils } = require("resource://gre/modules/FileUtils.jsm");
+const { Ci } = require("chrome");
+const {
+  ExtensionTestUtils,
+} = require("resource://testing-common/ExtensionXPCShellUtils.jsm");
+
+const Services = require("Services");
+const { DevToolsServer } = require("devtools/server/devtools-server");
+const { DevToolsClient } = require("devtools/client/devtools-client");
+
+/**
+ * Starts up DevTools server and connects a new DevTools client.
+ *
+ * @return {Promise} Resolves with a client object when the debugger has started up.
+ */
+async function startDebugger() {
+  DevToolsServer.init();
+  DevToolsServer.registerAllActors();
+  const transport = DevToolsServer.connectPipe();
+  const client = new DevToolsClient(transport);
+  await client.connect();
+  return client;
+}
+
+/**
+ * Set up the equivalent of an `about:debugging` toolbox for a given extension, minus
+ * the toolbox.
+ *
+ * @param {String} id - The id for the extension to be targeted by the toolbox.
+ * @return {Object} Resolves with the web extension actor front and target objects when
+ * the debugger has been connected to the extension.
+ */
+async function setupExtensionDebugging(id) {
+  const client = await startDebugger();
+  const front = await client.mainRoot.getAddon({ id });
+  // Starts a DevTools server in the extension child process.
+  const target = await front.getTarget();
+  return { front, target };
+}
+exports.setupExtensionDebugging = setupExtensionDebugging;
+
+/**
+ * Loads and starts up a test extension given the provided extension configuration.
+ *
+ * @param {Object} extConfig - The extension configuration object
+ * @return {ExtensionWrapper} extension - Resolves with an extension object once the
+ * extension has started up.
+ */
+async function startupExtension(extConfig) {
+  const extension = ExtensionTestUtils.loadExtension(extConfig);
+
+  await extension.startup();
+
+  return extension;
+}
+exports.startupExtension = startupExtension;
+
+/**
+ * Initializes the extensionStorage actor for a target extension. This is effectively
+ * what happens when the addon storage panel is opened in the browser.
+ *
+ * @param {String} - id, The addon id
+ * @return {Object} - Resolves with the web extension actor target and extensionStorage
+ * store objects when the panel has been opened.
+ */
+async function openAddonStoragePanel(id) {
+  const { target } = await setupExtensionDebugging(id);
+
+  const storageFront = await target.getFront("storage");
+  const stores = await storageFront.listStores();
+  const extensionStorage = stores.extensionStorage || null;
+
+  return { target, extensionStorage, storageFront };
+}
+exports.openAddonStoragePanel = openAddonStoragePanel;
+
+/**
+ * Builds the extension configuration object passed into ExtensionTestUtils.loadExtension
+ *
+ * @param {Object} options - Options, if any, to add to the configuration
+ * @param {Function} options.background - A function comprising the test extension's
+ * background script if provided
+ * @param {Object} options.files - An object whose keys correspond to file names and
+ * values map to the file contents
+ * @param {Object} options.manifest - An object representing the extension's manifest
+ * @return {Object} - The extension configuration object
+ */
+function getExtensionConfig(options = {}) {
+  const { manifest, ...otherOptions } = options;
+  const baseConfig = {
+    manifest: {
+      ...manifest,
+      permissions: ["storage"],
+    },
+    useAddonManager: "temporary",
+  };
+  return {
+    ...baseConfig,
+    ...otherOptions,
+  };
+}
+exports.getExtensionConfig = getExtensionConfig;
+
+/**
+ * Shared files for a test extension that has no background page but adds storage
+ * items via a transient extension page in a tab
+ */
+const ext_no_bg = {
+  files: {
+    "extension_page_in_tab.html": `<!DOCTYPE html>
+      <html>
+        <head>
+          <meta charset="utf-8">
+        </head>
+        <body>
+          <h1>Extension Page in a Tab</h1>
+          <script src="extension_page_in_tab.js"></script>
+        </body>
+      </html>`,
+    "extension_page_in_tab.js": extensionScriptWithMessageListener,
+  },
+};
+exports.ext_no_bg = ext_no_bg;
+
+/**
+ * An extension script that can be used in any extension context (e.g. as a background
+ * script or as an extension page script loaded in a tab).
+ */
+async function extensionScriptWithMessageListener() {
+  let fireOnChanged = false;
+  browser.storage.onChanged.addListener(() => {
+    if (fireOnChanged) {
+      // Do not fire it again until explicitly requested again using the "storage-local-fireOnChanged" test message.
+      fireOnChanged = false;
+      browser.test.sendMessage("storage-local-onChanged");
+    }
+  });
+
+  browser.test.onMessage.addListener(async (msg, ...args) => {
+    let item = null;
+    switch (msg) {
+      case "storage-local-set":
+        await browser.storage.local.set(args[0]);
+        break;
+      case "storage-local-get":
+        item = await browser.storage.local.get(args[0]);
+        break;
+      case "storage-local-remove":
+        await browser.storage.local.remove(args[0]);
+        break;
+      case "storage-local-clear":
+        await browser.storage.local.clear();
+        break;
+      case "storage-local-fireOnChanged": {
+        // Allow the storage.onChanged listener to send a test event
+        // message when onChanged is being fired.
+        fireOnChanged = true;
+        // Do not fire fireOnChanged:done.
+        return;
+      }
+      default:
+        browser.test.fail(`Unexpected test message: ${msg}`);
+    }
+
+    browser.test.sendMessage(`${msg}:done`, item);
+  });
+  browser.test.sendMessage("extension-origin", window.location.origin);
+}
+exports.extensionScriptWithMessageListener = extensionScriptWithMessageListener;
+
+/**
+ * Shutdown procedure common to all tasks.
+ *
+ * @param {Object} extension - The test extension
+ * @param {Object} target - The web extension actor targeted by the DevTools client
+ */
+async function shutdown(extension, target) {
+  if (target) {
+    await target.destroy();
+  }
+  await extension.unload();
+}
+exports.shutdown = shutdown;
+
+/**
+ * Mocks the missing 'storage/permanent' directory needed by the "indexedDB"
+ * storage actor's 'preListStores' method (called when 'listStores' is called). This
+ * directory exists in a full browser i.e. mochitest.
+ */
+function createMissingIndexedDBDirs() {
+  const dir = Services.dirsvc.get("ProfD", Ci.nsIFile).clone();
+  dir.append("storage");
+  if (!dir.exists()) {
+    dir.create(dir.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
+  }
+  dir.append("permanent");
+  if (!dir.exists()) {
+    dir.create(dir.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
+  }
+
+  return dir;
+}
+exports.createMissingIndexedDBDirs = createMissingIndexedDBDirs;

devtools/server/tests/xpcshell/test_extension_storage_actor.js

@@ -5,14 +5,20 @@
 
 "use strict";
 
-const { FileUtils } = ChromeUtils.import(
-  "resource://gre/modules/FileUtils.jsm"
-);
-
 const { ExtensionTestUtils } = ChromeUtils.import(
   "resource://testing-common/ExtensionXPCShellUtils.jsm"
 );
 
+const {
+  createMissingIndexedDBDirs,
+  extensionScriptWithMessageListener,
+  ext_no_bg,
+  getExtensionConfig,
+  openAddonStoragePanel,
+  shutdown,
+  startupExtension,
+} = require("resource://test/helper_test_extension_storage_actor.js");
+
 // Ignore rejection related to the storage.onChanged listener being removed while the extension context is being closed.
 const { PromiseTestUtils } = ChromeUtils.import(
   "resource://testing-common/PromiseTestUtils.jsm"
@@ -37,197 +43,14 @@ registerCleanupFunction(() => {
   Services.prefs.clearUserPref(EXTENSION_STORAGE_ENABLED_PREF);
 });
 
-/**
+add_task(async function setup() {
- * Starts up DevTools server and connects a new DevTools client.
+  await promiseStartupManager();
- *
+  const dir = createMissingIndexedDBDirs();
- * @return {Promise} Resolves with a client object when the debugger has started up.
- */
-async function startDebugger() {
-  DevToolsServer.init();
-  DevToolsServer.registerAllActors();
-  const transport = DevToolsServer.connectPipe();
-  const client = new DevToolsClient(transport);
-  await client.connect();
-  return client;
-}
 
-/**
- * Set up the equivalent of an `about:debugging` toolbox for a given extension, minus
- * the toolbox.
- *
- * @param {String} id - The id for the extension to be targeted by the toolbox.
- * @return {Object} Resolves with the web extension actor front and target objects when
- * the debugger has been connected to the extension.
- */
-async function setupExtensionDebugging(id) {
-  const client = await startDebugger();
-  const front = await client.mainRoot.getAddon({ id });
-  // Starts a DevTools server in the extension child process.
-  const target = await front.getTarget();
-  return { front, target };
-}
-
-/**
- * Loads and starts up a test extension given the provided extension configuration.
- *
- * @param {Object} extConfig - The extension configuration object
- * @return {ExtensionWrapper} extension - Resolves with an extension object once the
- * extension has started up.
- */
-async function startupExtension(extConfig) {
-  const extension = ExtensionTestUtils.loadExtension(extConfig);
-
-  await extension.startup();
-
-  return extension;
-}
-
-/**
- * Initializes the extensionStorage actor for a target extension. This is effectively
- * what happens when the addon storage panel is opened in the browser.
- *
- * @param {String} - id, The addon id
- * @return {Object} - Resolves with the web extension actor target and extensionStorage
- * store objects when the panel has been opened.
- */
-async function openAddonStoragePanel(id) {
-  const { target } = await setupExtensionDebugging(id);
-
-  const storageFront = await target.getFront("storage");
-  const stores = await storageFront.listStores();
-  const extensionStorage = stores.extensionStorage || null;
-
-  return { target, extensionStorage, storageFront };
-}
-
-/**
- * Builds the extension configuration object passed into ExtensionTestUtils.loadExtension
- *
- * @param {Object} options - Options, if any, to add to the configuration
- * @param {Function} options.background - A function comprising the test extension's
- * background script if provided
- * @param {Object} options.files - An object whose keys correspond to file names and
- * values map to the file contents
- * @param {Object} options.manifest - An object representing the extension's manifest
- * @return {Object} - The extension configuration object
- */
-function getExtensionConfig(options = {}) {
-  const { manifest, ...otherOptions } = options;
-  const baseConfig = {
-    manifest: {
-      ...manifest,
-      permissions: ["storage"],
-    },
-    useAddonManager: "temporary",
-  };
-  return {
-    ...baseConfig,
-    ...otherOptions,
-  };
-}
-
-/**
- * An extension script that can be used in any extension context (e.g. as a background
- * script or as an extension page script loaded in a tab).
- */
-async function extensionScriptWithMessageListener() {
-  let fireOnChanged = false;
-  browser.storage.onChanged.addListener(() => {
-    if (fireOnChanged) {
-      // Do not fire it again until explicitly requested again using the "storage-local-fireOnChanged" test message.
-      fireOnChanged = false;
-      browser.test.sendMessage("storage-local-onChanged");
-    }
-  });
-
-  browser.test.onMessage.addListener(async (msg, ...args) => {
-    let item = null;
-    switch (msg) {
-      case "storage-local-set":
-        await browser.storage.local.set(args[0]);
-        break;
-      case "storage-local-get":
-        item = await browser.storage.local.get(args[0]);
-        break;
-      case "storage-local-remove":
-        await browser.storage.local.remove(args[0]);
-        break;
-      case "storage-local-clear":
-        await browser.storage.local.clear();
-        break;
-      case "storage-local-fireOnChanged": {
-        // Allow the storage.onChanged listener to send a test event
-        // message when onChanged is being fired.
-        fireOnChanged = true;
-        // Do not fire fireOnChanged:done.
-        return;
-      }
-      default:
-        browser.test.fail(`Unexpected test message: ${msg}`);
-    }
-
-    browser.test.sendMessage(`${msg}:done`, item);
-  });
-  browser.test.sendMessage("extension-origin", window.location.origin);
-}
-
-/**
- * Shared files for a test extension that has no background page but adds storage
- * items via a transient extension page in a tab
- */
-const ext_no_bg = {
-  files: {
-    "extension_page_in_tab.html": `<!DOCTYPE html>
-      <html>
-        <head>
-          <meta charset="utf-8">
-        </head>
-        <body>
-          <h1>Extension Page in a Tab</h1>
-          <script src="extension_page_in_tab.js"></script>
-        </body>
-      </html>`,
-    "extension_page_in_tab.js": extensionScriptWithMessageListener,
-  },
-};
-
-/**
- * Shutdown procedure common to all tasks.
- *
- * @param {Object} extension - The test extension
- * @param {Object} target - The web extension actor targeted by the DevTools client
- */
-async function shutdown(extension, target) {
-  if (target) {
-    await target.destroy();
-  }
-  await extension.unload();
-}
-
-/**
- * Mocks the missing 'storage/permanent' directory needed by the "indexedDB"
- * storage actor's 'preListStores' method (called when 'listStores' is called). This
- * directory exists in a full browser i.e. mochitest.
- */
-function createMissingIndexedDBDirs() {
-  const dir = Services.dirsvc.get("ProfD", Ci.nsIFile).clone();
-  dir.append("storage");
-  if (!dir.exists()) {
-    dir.create(dir.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
-  }
-  dir.append("permanent");
-  if (!dir.exists()) {
-    dir.create(dir.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
-  }
   Assert.ok(
     dir.exists(),
     "Should have a 'storage/permanent' dir in the profile dir"
   );
-}
-
-add_task(async function setup() {
-  await promiseStartupManager();
-  createMissingIndexedDBDirs();
 });
 
 add_task(async function test_extension_store_exists() {
@@ -1006,75 +829,6 @@ add_task(
   }
 );
 
-/**
- * Test case: Bg page adds an item to storage. With storage panel open, reload extension.
- * - Load extension with background page that adds a storage item on message.
- * - Open the add-on storage panel.
- * - With the storage panel still open, reload the extension.
- * - The data in the storage panel should match the item added prior to reloading.
- */
-add_task(async function test_panel_live_reload() {
-  const EXTENSION_ID = "test_panel_live_reload@xpcshell.mozilla.org";
-  let manifest = {
-    version: "1.0",
-    applications: {
-      gecko: {
-        id: EXTENSION_ID,
-      },
-    },
-  };
-
-  info("Loading extension version 1.0");
-  const extension = await startupExtension(
-    getExtensionConfig({
-      manifest,
-      background: extensionScriptWithMessageListener,
-    })
-  );
-
-  info("Waiting for message from test extension");
-  const host = await extension.awaitMessage("extension-origin");
-
-  info("Adding storage item");
-  extension.sendMessage("storage-local-set", { a: 123 });
-  await extension.awaitMessage("storage-local-set:done");
-
-  const { target, extensionStorage } = await openAddonStoragePanel(
-    extension.id
-  );
-
-  manifest = {
-    ...manifest,
-    version: "2.0",
-  };
-  // "Reload" is most similar to an upgrade, as e.g. storage data is preserved
-  info("Update to version 2.0");
-  await extension.upgrade(
-    getExtensionConfig({
-      manifest,
-      background: extensionScriptWithMessageListener,
-    })
-  );
-
-  await extension.awaitMessage("extension-origin");
-
-  const { data } = await extensionStorage.getStoreObjects(host);
-  Assert.deepEqual(
-    data,
-    [
-      {
-        area: "local",
-        name: "a",
-        value: { str: "123" },
-        isValueEditable: true,
-      },
-    ],
-    "Got the expected results on populated storage.local"
-  );
-
-  await shutdown(extension, target);
-});
-
 /**
  * Test case: Transient page adds an item to storage. With storage panel open,
  * reload extension.

devtools/server/tests/xpcshell/test_extension_storage_actor_upgrade.js

@@ -0,0 +1,124 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/**
+ * Note: this test used to be in test_extension_storage_actor.js, but seems to
+ * fail frequently as soon as we start auto-attaching targets.
+ * See Bug 1618059.
+ */
+
+const { ExtensionTestUtils } = ChromeUtils.import(
+  "resource://testing-common/ExtensionXPCShellUtils.jsm"
+);
+
+const {
+  createMissingIndexedDBDirs,
+  extensionScriptWithMessageListener,
+  getExtensionConfig,
+  openAddonStoragePanel,
+  shutdown,
+  startupExtension,
+} = require("resource://test/helper_test_extension_storage_actor.js");
+
+// Ignore rejection related to the storage.onChanged listener being removed while the extension context is being closed.
+const { PromiseTestUtils } = ChromeUtils.import(
+  "resource://testing-common/PromiseTestUtils.jsm"
+);
+PromiseTestUtils.whitelistRejectionsGlobally(/Message manager disconnected/);
+
+const { createAppInfo, promiseStartupManager } = AddonTestUtils;
+
+const EXTENSION_STORAGE_ENABLED_PREF =
+  "devtools.storage.extensionStorage.enabled";
+
+AddonTestUtils.init(this);
+createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "42");
+
+ExtensionTestUtils.init(this);
+
+// This storage actor is gated behind a pref, so make sure it is enabled first
+Services.prefs.setBoolPref(EXTENSION_STORAGE_ENABLED_PREF, true);
+registerCleanupFunction(() => {
+  Services.prefs.clearUserPref(EXTENSION_STORAGE_ENABLED_PREF);
+});
+
+add_task(async function setup() {
+  await promiseStartupManager();
+  const dir = createMissingIndexedDBDirs();
+
+  Assert.ok(
+    dir.exists(),
+    "Should have a 'storage/permanent' dir in the profile dir"
+  );
+});
+
+/**
+ * Test case: Bg page adds an item to storage. With storage panel open, reload extension.
+ * - Load extension with background page that adds a storage item on message.
+ * - Open the add-on storage panel.
+ * - With the storage panel still open, reload the extension.
+ * - The data in the storage panel should match the item added prior to reloading.
+ */
+add_task(async function test_panel_live_reload() {
+  const EXTENSION_ID = "test_panel_live_reload@xpcshell.mozilla.org";
+  let manifest = {
+    version: "1.0",
+    applications: {
+      gecko: {
+        id: EXTENSION_ID,
+      },
+    },
+  };
+
+  info("Loading extension version 1.0");
+  const extension = await startupExtension(
+    getExtensionConfig({
+      manifest,
+      background: extensionScriptWithMessageListener,
+    })
+  );
+
+  info("Waiting for message from test extension");
+  const host = await extension.awaitMessage("extension-origin");
+
+  info("Adding storage item");
+  extension.sendMessage("storage-local-set", { a: 123 });
+  await extension.awaitMessage("storage-local-set:done");
+
+  const { target, extensionStorage } = await openAddonStoragePanel(
+    extension.id
+  );
+
+  manifest = {
+    ...manifest,
+    version: "2.0",
+  };
+  // "Reload" is most similar to an upgrade, as e.g. storage data is preserved
+  info("Update to version 2.0");
+  await extension.upgrade(
+    getExtensionConfig({
+      manifest,
+      background: extensionScriptWithMessageListener,
+    })
+  );
+
+  await extension.awaitMessage("extension-origin");
+
+  const { data } = await extensionStorage.getStoreObjects(host);
+  Assert.deepEqual(
+    data,
+    [
+      {
+        area: "local",
+        name: "a",
+        value: { str: "123" },
+        isValueEditable: true,
+      },
+    ],
+    "Got the expected results on populated storage.local"
+  );
+
+  await shutdown(extension, target);
+});

devtools/server/tests/xpcshell/xpcshell.ini

@@ -6,6 +6,7 @@ skip-if = toolkit == 'android'
 
 support-files =
   completions.js
+  helper_test_extension_storage_actor.js
   source-map-data/sourcemapped.coffee
   source-map-data/sourcemapped.map
   post_init_global_actors.js
@@ -59,7 +60,8 @@ support-files =
 [test_blackboxing-05.js]
 [test_blackboxing-08.js]
 [test_extension_storage_actor.js]
-skip-if = tsan || (os == 'win') # Unreasonably slow, bug 1612707, Bug 1618059
+skip-if = tsan # Unreasonably slow, bug 1612707
+[test_extension_storage_actor_upgrade.js]
 [test_frameactor-01.js]
 [test_frameactor-02.js]
 [test_frameactor-03.js]