Bug 1399348 - Explicitly wait and check for error in test browser_ext_tabs_executeScript_bad r=rpl
authorRob Wu <rob@robwu.nl>
Mon, 23 Apr 2018 15:28:41 +0200
changeset 471968 dd9ba54d497c7493eb926b984572eebffe6138b9
parent 471967 8ac380929a557b50ef89ff03698429e02d7aa3b8
child 471969 3a10be9bb6bd3b90594b4afe2896f9279f339e9a
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrpl
bugs1399348, 1435100
milestone61.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 1399348 - Explicitly wait and check for error in test browser_ext_tabs_executeScript_bad r=rpl I think that the intermittent error in the bug may be caused by a pending executeScript call that is somehow handled around the shutdown of the extension. To verify this hypothesis, the test now explicitly waits for the result of the first executeScript call before executing the last script that is responsible for test completion. The test should explicitly be checking for the error anyway. And clean up comments and add reference to bug 1435100 in an existing comment. MozReview-Commit-ID: 6gV30Z6zQc4
browser/components/extensions/test/browser/browser_ext_tabs_executeScript_bad.js
mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_bad.html
--- a/browser/components/extensions/test/browser/browser_ext_tabs_executeScript_bad.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_executeScript_bad.js
@@ -1,46 +1,34 @@
 "use strict";
 
-// Various "Missing host permission" rejections are left uncaught. This may be
-// caused by issues in the test, in the testing framework, or in production
-// code. This bug should be fixed, but for the moment this file is whitelisted.
-//
-// NOTE: Whitelisting a class of rejections should be limited. Normally you
-//       should use "expectUncaughtRejection" to flag individual failures.
-ChromeUtils.import("resource://testing-common/PromiseTestUtils.jsm", this);
-PromiseTestUtils.whitelistRejectionsGlobally(/Missing host permission/);
-
-// This is a pretty terrible hack, but it's the best we can do until we
-// support |executeScript| callbacks and |lastError|.
 async function testHasNoPermission(params) {
   let contentSetup = params.contentSetup || (() => Promise.resolve());
 
   async function background(contentSetup) {
     browser.runtime.onMessage.addListener((msg, sender) => {
       browser.test.assertEq(msg, "second script ran", "second script ran");
       browser.test.notifyPass("executeScript");
     });
 
-    browser.test.onMessage.addListener(msg => {
+    browser.test.onMessage.addListener(async msg => {
       browser.test.assertEq(msg, "execute-script");
 
-      browser.tabs.query({currentWindow: true}, tabs => {
-        browser.tabs.executeScript({
-          file: "script.js",
-        });
+      let tabs = await browser.tabs.query({currentWindow: true});
+      await browser.test.assertRejects(browser.tabs.executeScript({
+        file: "script.js",
+      }), /Missing host permission for the tab/);
 
-        // Execute a script we know we have permissions for in the
-        // second tab, in the hopes that it will execute after the
-        // first one. This has intermittent failure written all over
-        // it, but it's just about the best we can do until we
-        // support callbacks for executeScript.
-        browser.tabs.executeScript(tabs[1].id, {
-          file: "second-script.js",
-        });
+      // TODO bug 1457115: Remove the executeScript call below.
+
+      // Execute a script we know we have permissions for in the
+      // second tab, in the hopes that it will execute after the
+      // first one.
+      browser.tabs.executeScript(tabs[1].id, {
+        file: "second-script.js",
       });
     });
 
     await contentSetup();
 
     browser.test.sendMessage("ready");
   }
 
@@ -346,14 +334,13 @@ add_task(async function testBadURL() {
 
   await extension.startup();
 
   await extension.awaitFinish("executeScript-lastError");
 
   await extension.unload();
 });
 
-// TODO: Test that |executeScript| fails if the tab has navigated to a
-// new page, and no longer matches our expected state. This involves
-// intentionally trying to trigger a race condition, and is probably not
-// even worth attempting until we have proper |executeScript| callbacks.
+// TODO bug 1435100: Test that |executeScript| fails if the tab has navigated
+// to a new page, and no longer matches our expected state. This involves
+// intentionally trying to trigger a race condition.
 
 add_task(forceGC);
--- a/mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_bad.html
+++ b/mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_bad.html
@@ -8,43 +8,40 @@
   <script type="text/javascript" src="head.js"></script>
   <link rel="stylesheet" href="/tests/SimpleTest/test.css"/>
 </head>
 <body>
 
 <script type="text/javascript">
 "use strict";
 
-// This is a pretty terrible hack, but it's the best we can do until we
-// support |executeScript| callbacks and |lastError|.
 function* testHasNoPermission(params) {
   let contentSetup = params.contentSetup || (() => Promise.resolve());
 
   async function background(contentSetup) {
     browser.runtime.onMessage.addListener((msg, sender) => {
       browser.test.assertEq(msg, "second script ran", "second script ran");
       browser.test.notifyPass("executeScript");
     });
 
-    browser.test.onMessage.addListener(msg => {
+    browser.test.onMessage.addListener(async msg => {
       browser.test.assertEq(msg, "execute-script");
 
-      browser.tabs.query({currentWindow: true}, tabs => {
-        browser.tabs.executeScript({
-          file: "script.js",
-        });
+      let tabs = await browser.tabs.query({currentWindow: true});
+      await browser.test.assertRejects(browser.tabs.executeScript({
+        file: "script.js",
+      }), /Missing host permission for the tab/);
 
-        // Execute a script we know we have permissions for in the
-        // second tab, in the hopes that it will execute after the
-        // first one. This has intermittent failure written all over
-        // it, but it's just about the best we can do until we
-        // support callbacks for executeScript.
-        browser.tabs.executeScript(tabs[1].id, {
-          file: "second-script.js",
-        });
+      // TODO bug 1457115: Remove the executeScript call below.
+
+      // Execute a script we know we have permissions for in the
+      // second tab, in the hopes that it will execute after the
+      // first one.
+      browser.tabs.executeScript(tabs[1].id, {
+        file: "second-script.js",
       });
     });
 
     await contentSetup();
 
     browser.test.sendMessage("ready");
   }
 
@@ -159,16 +156,15 @@ add_task(async function testBadURL() {
 
   await extension.startup();
 
   await extension.awaitFinish("executeScript-lastError");
 
   await extension.unload();
 });
 
-// TODO: Test that |executeScript| fails if the tab has navigated to a
-// new page, and no longer matches our expected state. This involves
-// intentionally trying to trigger a race condition, and is probably not
-// even worth attempting until we have proper |executeScript| callbacks.
+// TODO bug 1435100: Test that |executeScript| fails if the tab has navigated
+// to a new page, and no longer matches our expected state. This involves
+// intentionally trying to trigger a race condition.
 </script>
 
 </body>
 </html>