Bug 1417017 - Check for content. usage at global and within add_task for no-cpows-in-tests. r=standard8
authorEd Lee <edilee@mozilla.com>
Tue, 14 Nov 2017 02:17:57 -0800
changeset 393075 257de1de1304548c1dcb2c7d8c66a20d26c021e6
parent 393074 2541054da0cb9f23cfa5fdf3ef08379a4696baad
child 393076 366c4c091bb3dba617776f06a03ac63875fa6081
push id55833
push useredilee@gmail.com
push dateWed, 22 Nov 2017 13:55:24 +0000
treeherderautoland@257de1de1304 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersstandard8
bugs1417017
milestone59.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 1417017 - Check for content. usage at global and within add_task for no-cpows-in-tests. r=standard8 MozReview-Commit-ID: Asz6dM29uRJ
browser/base/content/test/general/browser_bug581253.js
browser/components/preferences/in-content/tests/browser_advanced_update.js
browser/components/preferences/in-content/tests/browser_cookies_dialog.js
browser/components/preferences/in-content/tests/browser_password_management.js
browser/components/preferences/in-content/tests/browser_siteData.js
browser/components/preferences/in-content/tests/browser_siteData3.js
browser/components/preferences/in-content/tests/browser_site_login_exceptions.js
browser/components/preferences/in-content/tests/browser_subdialogs.js
browser/components/search/test/browser_abouthome_behavior.js
browser/extensions/onboarding/test/browser/browser_onboarding_skip_tour.js
toolkit/content/tests/browser/browser_datetime_datepicker.js
tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-cpows-in-tests.js
tools/lint/eslint/eslint-plugin-mozilla/tests/no-cpows-in-tests.js
--- a/browser/base/content/test/general/browser_bug581253.js
+++ b/browser/base/content/test/general/browser_bug581253.js
@@ -19,16 +19,17 @@ add_task(async function test_remove_book
   await PlacesUtils.bookmarks.insert({
     parentGuid: PlacesUtils.bookmarks.unfiledGuid,
     title: "",
     url: testURL,
   });
 
   Assert.ok(await PlacesUtils.bookmarks.fetch({url: testURL}), "the test url is bookmarked");
 
+  // eslint-disable-next-line mozilla/no-cpows-in-tests
   content.location = testURL;
 
   await BrowserTestUtils.waitForCondition(
     () => BookmarkingUI.status == BookmarkingUI.STATUS_STARRED,
     "star button indicates that the page is bookmarked");
 
   PlacesUtils.tagging.tagURI(makeURI(testURL), [testTag]);
 
--- a/browser/components/preferences/in-content/tests/browser_advanced_update.js
+++ b/browser/components/preferences/in-content/tests/browser_advanced_update.js
@@ -109,16 +109,17 @@ add_task(async function() {
   gBrowser.removeCurrentTab();
 });
 
 add_task(async function() {
   await openPreferencesViaOpenPreferencesAPI("general", { leaveOpen: true });
   let doc = gBrowser.selectedBrowser.contentDocument;
 
   let showBtn = doc.getElementById("showUpdateHistory");
+  // eslint-disable-next-line mozilla/no-cpows-in-tests
   let dialogOverlay = content.gSubDialog._preloadDialog._overlay;
 
   // XXX: For unknown reasons, this mock cannot be loaded by
   // XPCOMUtils.defineLazyServiceGetter() called in aboutDialog-appUpdater.js.
   // It is registered here so that we could assert update history subdialog
   // without stopping the preferences advanced pane from loading.
   // See bug 1361929.
   mockUpdateManager.register();
--- a/browser/components/preferences/in-content/tests/browser_cookies_dialog.js
+++ b/browser/components/preferences/in-content/tests/browser_cookies_dialog.js
@@ -35,16 +35,17 @@ add_task(async function testDeleteCookie
   if (AppConstants.platform == "macosx") {
     EventUtils.synthesizeKey("VK_BACK_SPACE", {});
   } else {
     EventUtils.synthesizeKey("VK_DELETE", {});
   }
 
   await waitForCondition(() => tree.view.rowCount == 0);
 
+  // eslint-disable-next-line mozilla/no-cpows-in-tests
   is_element_visible(content.gSubDialog._dialogs[0]._box,
     "Subdialog is visible after deleting an element");
 
 });
 
 add_task(async function removeTab() {
   gBrowser.removeCurrentTab();
 });
--- a/browser/components/preferences/in-content/tests/browser_password_management.js
+++ b/browser/components/preferences/in-content/tests/browser_password_management.js
@@ -61,11 +61,12 @@ add_task(async function test_deletePassw
   if (AppConstants.platform == "macosx") {
     EventUtils.synthesizeKey("VK_BACK_SPACE", {});
   } else {
     EventUtils.synthesizeKey("VK_DELETE", {});
   }
 
   await waitForCondition(() => tree.view.rowCount == 0);
 
+  // eslint-disable-next-line mozilla/no-cpows-in-tests
   is_element_visible(content.gSubDialog._dialogs[0]._box,
     "Subdialog is visible after deleting an element");
 });
--- a/browser/components/preferences/in-content/tests/browser_siteData.js
+++ b/browser/components/preferences/in-content/tests/browser_siteData.js
@@ -142,16 +142,17 @@ add_task(async function() {
   // eslint-disable-next-line mozilla/no-cpows-in-tests
   await waitForEvent(gBrowser.selectedBrowser.contentWindow, "test-indexedDB-done");
   await BrowserTestUtils.removeTab(gBrowser.selectedTab);
 
   let updatedPromise = promiseSiteDataManagerSitesUpdated();
   await openPreferencesViaOpenPreferencesAPI("privacy", { leaveOpen: true });
   await updatedPromise;
   await openSiteDataSettingsDialog();
+  // eslint-disable-next-line mozilla/no-cpows-in-tests
   let dialog = content.gSubDialog._topDialog;
   let dialogFrame = dialog._frame;
   let frameDoc = dialogFrame.contentDocument;
 
   let siteItems = frameDoc.getElementsByTagName("richlistitem");
   is(siteItems.length, 2, "Should list sites using quota usage or appcache");
 
   let appcacheSite = frameDoc.querySelector(`richlistitem[host="${TEST_OFFLINE_HOST}"]`);
--- a/browser/components/preferences/in-content/tests/browser_siteData3.js
+++ b/browser/components/preferences/in-content/tests/browser_siteData3.js
@@ -132,16 +132,17 @@ add_task(async function() {
     },
   ];
 
   let updatePromise = promiseSiteDataManagerSitesUpdated();
   await openPreferencesViaOpenPreferencesAPI("privacy", { leaveOpen: true });
   await updatePromise;
   await openSiteDataSettingsDialog();
 
+  // eslint-disable-next-line mozilla/no-cpows-in-tests
   let dialog = content.gSubDialog._topDialog;
   let dialogFrame = dialog._frame;
   let frameDoc = dialogFrame.contentDocument;
   let hostCol = frameDoc.getElementById("hostCol");
   let usageCol = frameDoc.getElementById("usageCol");
   let statusCol = frameDoc.getElementById("statusCol");
   let sitesList = frameDoc.getElementById("sitesList");
 
--- a/browser/components/preferences/in-content/tests/browser_site_login_exceptions.js
+++ b/browser/components/preferences/in-content/tests/browser_site_login_exceptions.js
@@ -66,11 +66,12 @@ add_task(async function deleteALoginExce
   if (AppConstants.platform == "macosx") {
     EventUtils.synthesizeKey("VK_BACK_SPACE", {});
   } else {
     EventUtils.synthesizeKey("VK_DELETE", {});
   }
 
   await waitForCondition(() => tree.view.rowCount == 0);
 
+  // eslint-disable-next-line mozilla/no-cpows-in-tests
   is_element_visible(content.gSubDialog._dialogs[0]._box,
     "Subdialog is visible after deleting an element");
 });
--- a/browser/components/preferences/in-content/tests/browser_subdialogs.js
+++ b/browser/components/preferences/in-content/tests/browser_subdialogs.js
@@ -174,16 +174,17 @@ add_task(async function check_reopening_
   await close_subdialog_and_test_generic_end_state(tab.linkedBrowser,
     function() { content.window.gSubDialog._topDialog._frame.contentDocument.documentElement.acceptDialog(); },
     "accept", 1);
 });
 
 add_task(async function check_opening_while_closing() {
   await open_subdialog_and_test_generic_start_state(tab.linkedBrowser);
   info("closing");
+  // eslint-disable-next-line mozilla/no-cpows-in-tests
   content.window.gSubDialog._topDialog.close();
   info("reopening immediately after calling .close()");
   await open_subdialog_and_test_generic_start_state(tab.linkedBrowser);
   await close_subdialog_and_test_generic_end_state(tab.linkedBrowser,
     function() { content.window.gSubDialog._topDialog._frame.contentDocument.documentElement.acceptDialog(); },
     "accept", 1);
 
 });
--- a/browser/components/search/test/browser_abouthome_behavior.js
+++ b/browser/components/search/test/browser_abouthome_behavior.js
@@ -34,16 +34,17 @@ function test() {
       if (event.originalTarget != tab.linkedBrowser.contentDocumentAsCPOW ||
           event.target.location.href == "about:blank") {
         info("skipping spurious load event");
         return;
       }
       tab.linkedBrowser.removeEventListener("load", load, true);
 
       // Observe page setup
+      // eslint-disable-next-line mozilla/no-cpows-in-tests
       let doc = gBrowser.contentDocumentAsCPOW;
       gMutationObserver = new MutationObserver(function(mutations) {
         for (let mutation of mutations) {
           if (mutation.attributeName == "searchEngineName") {
             // Re-add the listener, and perform a search
             gBrowser.addProgressListener(listener);
             gMutationObserver.disconnect();
             gMutationObserver = null;
--- a/browser/extensions/onboarding/test/browser/browser_onboarding_skip_tour.js
+++ b/browser/extensions/onboarding/test/browser/browser_onboarding_skip_tour.js
@@ -31,12 +31,13 @@ add_task(async function test_hide_skip_b
   resetOnboardingDefaultState();
   Preferences.set("browser.onboarding.skip-tour-button.hide", true);
 
   let tab = await openTab(ABOUT_NEWTAB_URL);
   await promiseOnboardingOverlayLoaded(tab.linkedBrowser);
   await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-overlay-button", {}, tab.linkedBrowser);
   await promiseOnboardingOverlayOpened(tab.linkedBrowser);
 
+  // eslint-disable-next-line mozilla/no-cpows-in-tests
   ok(!content.document.querySelector("#onboarding-skip-tour-button"), "should not render the skip button");
 
   await BrowserTestUtils.removeTab(tab);
 });
--- a/toolkit/content/tests/browser/browser_datetime_datepicker.js
+++ b/toolkit/content/tests/browser/browser_datetime_datepicker.js
@@ -162,16 +162,17 @@ add_task(async function test_datepicker_
   await helper.openPicker(`data:text/html, <input type="date" value="${inputValue}">`);
   // Click the first item (top-left corner) of the calendar
   helper.click(helper.getElement(DAYS_VIEW).children[0]);
   await ContentTask.spawn(helper.tab.linkedBrowser, {}, async function() {
     let inputEl = content.document.querySelector("input");
     await ContentTaskUtils.waitForEvent(inputEl, "input");
   });
 
+  // eslint-disable-next-line mozilla/no-cpows-in-tests
   Assert.equal(content.document.querySelector("input").value, firstDayOnCalendar);
 
   await helper.tearDown();
 });
 
 /**
  * Make sure picker is in correct state when it is reopened.
  */
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
@@ -372,29 +372,46 @@ module.exports = {
       ecmaFeatures: {
         experimentalObjectRestSpread: true,
         globalReturn: true
       }
     };
   },
 
   /**
+   * Check whether a node is a function.
+   *
+   * @param {Object} node
+   *        The AST node to check
+   *
+   * @return {Boolean}
+   *         True or false
+   */
+  getIsFunctionNode(node) {
+    switch (node.type) {
+      case "ArrowFunctionExpression":
+      case "FunctionDeclaration":
+      case "FunctionExpression":
+        return true;
+    }
+    return false;
+  },
+
+  /**
    * Check whether the context is the global scope.
    *
    * @param {Array} ancestors
    *        The parents of the current node.
    *
    * @return {Boolean}
    *         True or false
    */
   getIsGlobalScope(ancestors) {
     for (let parent of ancestors) {
-      if (parent.type == "FunctionExpression" ||
-          parent.type == "FunctionDeclaration" ||
-          parent.type == "ArrowFunctionExpression") {
+      if (this.getIsFunctionNode(parent)) {
         return false;
       }
     }
     return true;
   },
 
   /**
    * Check whether we might be in a test head file.
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-cpows-in-tests.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-cpows-in-tests.js
@@ -9,45 +9,81 @@
 "use strict";
 
 // -----------------------------------------------------------------------------
 // Rule Definition
 // -----------------------------------------------------------------------------
 
 var helpers = require("../helpers");
 
-// Note: we match to the end of the string as well as the beginning, to avoid
-// multiple reports from MemberExpression statements.
 var cpows = [
-  /^gBrowser\.contentWindow$/,
-  /^gBrowser\.contentDocument$/,
-  /^gBrowser\.selectedBrowser\.contentWindow$/,
-  /^browser\.contentDocument$/,
-  /^window\.content$/
+  /^gBrowser\.contentWindow/,
+  /^gBrowser\.contentDocument/,
+  /^gBrowser\.selectedBrowser\.contentWindow/,
+  /^browser\.contentDocument/,
+  /^window\.content/
 ];
 
+// Keep track of where the last error is reported so to avoid reporting the same
+// expression (e.g., window.content.X vs window.content). Resets for each file.
+var lastErrorStart;
+
+// Keep track of whether we've entered a ContentTask.spawn call where accesses
+// to "content" are *not* CPOW. Resets when exiting the call.
 var isInContentTask = false;
 
 module.exports = function(context) {
   // ---------------------------------------------------------------------------
   // Helpers
   // ---------------------------------------------------------------------------
 
   function showError(node, identifier) {
     if (isInContentTask) {
       return;
     }
 
+    // Avoid showing partial expressions errors when one already errored.
+    if (node.start === lastErrorStart) {
+      return;
+    }
+    lastErrorStart = node.start;
+
     context.report({
       node,
       message: identifier +
                " is a possible Cross Process Object Wrapper (CPOW)."
     });
   }
 
+  function hasLocalContentVariable(node) {
+    // Walk up the parents, see if we can find if "content" is a local variable.
+    let parent = node;
+    do {
+      parent = parent.parent;
+
+      // Don't error if 'content' is one of the function parameters.
+      if (helpers.getIsFunctionNode(parent) &&
+          context.getDeclaredVariables(parent).some(variable => variable.name === "content")) {
+        return true;
+      } else if (parent.type === "BlockStatement" || parent.type === "Program") {
+        // Don't error if the block or program includes their own definition of content.
+        for (let item of parent.body) {
+          if (item.type === "VariableDeclaration" && item.declarations.length) {
+            for (let declaration of item.declarations) {
+              if (declaration.id && declaration.id.name === "content") {
+                return true;
+              }
+            }
+          }
+        }
+      }
+    } while (parent.parent);
+    return false;
+  }
+
   function isContentTask(node) {
     return node &&
            node.type === "MemberExpression" &&
            node.property.type === "Identifier" &&
            node.property.name === "spawn" &&
            node.object.type === "Identifier" &&
            node.object.name === "ContentTask";
   }
@@ -64,36 +100,57 @@ module.exports = function(context) {
     },
 
     "CallExpression:exit": function(node) {
       if (isContentTask(node.callee)) {
         isInContentTask = false;
       }
     },
 
+    Program() {
+      lastErrorStart = undefined;
+    },
+
     MemberExpression(node) {
       if (helpers.getTestType(context) != "browser") {
         return;
       }
 
       var expression = context.getSource(node);
 
       // Only report a single CPOW error per node -- so if checking
       // |cpows| reports one, don't report another below.
       var someCpowFound = cpows.some(function(cpow) {
         if (cpow.test(expression)) {
           showError(node, expression);
           return true;
         }
         return false;
       });
-      if (!someCpowFound && helpers.getIsGlobalScope(context.getAncestors())) {
-        if (/^content\./.test(expression)) {
-          showError(node, expression);
+
+      // Specially scope checks for "context." to avoid false positives.
+      if (!someCpowFound && /^content\./.test(expression)) {
+        // Don't error if we're multiple scopes deep. For now we only care about
+        // 2 scopes: global (length = 0) or immediately called by add_task.
+        // Ideally, we would care about any scope, but figuring out whether
+        // "context" is CPOW-or-not depends on how functions are defined and
+        // called -- both aspects are especially complex with head.js helpers.
+        // https://bugzilla.mozilla.org/show_bug.cgi?id=1417017#c9
+        const scopes = context.getAncestors().filter(helpers.getIsFunctionNode);
+        if (scopes.length > 1 || scopes.length === 1 &&
+            (!scopes[0].parent.callee || scopes[0].parent.callee.name !== "add_task")) {
+          return;
         }
+
+        // Don't error if there's a locally scoped "content"
+        if (hasLocalContentVariable(node)) {
+          return;
+        }
+
+        showError(node, expression);
       }
     },
 
     Identifier(node) {
       if (helpers.getTestType(context) != "browser") {
         return;
       }
 
@@ -107,36 +164,18 @@ module.exports = function(context) {
       }
 
       // Don't error in the case of `let content = foo`.
       if (node.parent.type === "VariableDeclarator" &&
           node.parent.id && node.parent.id.name === "content") {
         return;
       }
 
-      // Walk up the parents, see if we can find if this is a local variable.
-      let parent = node;
-      do {
-        parent = parent.parent;
-
-        // Don't error if 'content' is one of the function parameters.
-        if (parent.type === "FunctionDeclaration" &&
-            context.getDeclaredVariables(parent).some(variable => variable.name === "content")) {
-          return;
-        } else if (parent.type === "BlockStatement" || parent.type === "Program") {
-          // Don't error if the block or program includes their own definition of content.
-          for (let item of parent.body) {
-            if (item.type === "VariableDeclaration" && item.declarations.length) {
-              for (let declaration of item.declarations) {
-                if (declaration.id && declaration.id.name === "content") {
-                  return;
-                }
-              }
-            }
-          }
-        }
-      } while (parent.parent);
+      // Don't error if there's a locally scoped "content"
+      if (hasLocalContentVariable(node)) {
+        return;
+      }
 
       var expression = context.getSource(node);
       showError(node, expression);
     }
   };
 };
--- a/tools/lint/eslint/eslint-plugin-mozilla/tests/no-cpows-in-tests.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/tests/no-cpows-in-tests.js
@@ -28,21 +28,28 @@ function invalidCode(code, item, type = 
 }
 
 ruleTester.run("no-cpows-in-tests", rule, {
   valid: [
     "window.document",
     wrapCode("ContentTask.spawn(browser, null, () => { content.document; });"),
     wrapCode("let x = cssDocs.tooltip.content.contentDocument;"),
     wrapCode("function test(content) { let x = content; }"),
-    wrapCode('let content = "content"; foo(content);')
+    wrapCode('let content = "content"; foo(content);'),
+    wrapCode("helper(() => content.document);"),
+    wrapCode("add_task(() => helper(() => content.document));"),
+    wrapCode("add_task(() => { let content = {}; content.document; })"),
+    wrapCode("add_task(content => content.document)"),
+    wrapCode("add_task(function(content) { content.document; })"),
+    wrapCode("add_task(() => ContentTask.spawn(browser, null, () => content.document));")
   ],
   invalid: [
     invalidCode("let x = gBrowser.contentWindow;", "gBrowser.contentWindow"),
     invalidCode("let x = gBrowser.contentDocument;", "gBrowser.contentDocument"),
     invalidCode("let x = gBrowser.selectedBrowser.contentWindow;", "gBrowser.selectedBrowser.contentWindow"),
     invalidCode("let x = browser.contentDocument;", "browser.contentDocument"),
     invalidCode("let x = window.content;", "window.content"),
     invalidCode("content.document;", "content.document"),
+    invalidCode("add_task(() => content.document.querySelectorAll())", "content.document.querySelectorAll"),
     invalidCode("let x = content;", "content", "Identifier"),
-    invalidCode("let x = gBrowser.contentWindow.wrappedJSObject", "gBrowser.contentWindow")
+    invalidCode("let x = gBrowser.contentWindow.wrappedJSObject", "gBrowser.contentWindow.wrappedJSObject")
   ]
 });