Bug 1417017 - Check for content. usage at global and within add_task for no-cpows-in-tests. r?Standard8 draft
authorEd Lee <edilee@mozilla.com>
Tue, 14 Nov 2017 02:17:57 -0800
changeset 701622 44ac66bbf75577ebdf95591e606ae3a4d403aaca
parent 701610 b96f009478987d44a68a8d7cad40c6a3d6626235
child 741223 17b368f5488e59f4adfcae229080e1545bbfe546
push id90223
push userbmo:edilee@mozilla.com
push dateWed, 22 Nov 2017 01:48:48 +0000
reviewersStandard8
bugs1417017
milestone59.0a1
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")
   ]
 });