Bug 1246896 - Improve detection for mozilla/no-cpows-in-tests.js 'content' - only notify when content is used as an identifier. r?florian draft
authorMark Banner <standard8@mozilla.com>
Fri, 13 Oct 2017 17:04:48 +0100
changeset 687688 6c536a0f67a5b1393d117ec6f595d233e8aa7ad0
parent 687687 5c5b2eb6e2c7209b4a2ece939198e599fb6d1249
child 687689 9cc74f17dfb0183e242de8579e924a75ce073f3f
push id86565
push userbmo:standard8@mozilla.com
push dateFri, 27 Oct 2017 15:48:54 +0000
reviewersflorian
bugs1246896
milestone58.0a1
Bug 1246896 - Improve detection for mozilla/no-cpows-in-tests.js 'content' - only notify when content is used as an identifier. r?florian MozReview-Commit-ID: 1X38Wq4hUmZ
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/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,22 +9,24 @@
 "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$/
 ];
 
 var isInContentTask = false;
 
 module.exports = function(context) {
   // ---------------------------------------------------------------------------
   // Helpers
   // ---------------------------------------------------------------------------
@@ -81,32 +83,60 @@ module.exports = function(context) {
           showError(node, expression);
           return true;
         }
         return false;
       });
       if (!someCpowFound && helpers.getIsGlobalScope(context.getAncestors())) {
         if (/^content\./.test(expression)) {
           showError(node, expression);
-
         }
       }
     },
 
     Identifier(node) {
       if (helpers.getTestType(context) != "browser") {
         return;
       }
 
+      if (node.name !== "content" ||
+          // Don't complain if this is part of a member expression - the
+          // MemberExpression() function will handle those.
+          node.parent.type === "MemberExpression" ||
+          // If this is a declared variable in a function, then don't complain.
+          node.parent.type === "FunctionDeclaration") {
+        return;
+      }
+
+      // 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);
+
       var expression = context.getSource(node);
-      if (expression == "content" || /^content\./.test(expression)) {
-        if (node.parent.type === "MemberExpression" &&
-            node.parent.object &&
-            node.parent.object.type === "Identifier" &&
-            node.parent.object.name != "content") {
-          return;
-        }
-        showError(node, expression);
-
-      }
+      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
@@ -15,28 +15,34 @@ const ruleTester = new RuleTester({ pars
 // ------------------------------------------------------------------------------
 // Tests
 // ------------------------------------------------------------------------------
 
 function wrapCode(code, filename = "browser_fake.js") {
   return {code, filename};
 }
 
-function invalidCode(code, item) {
+function invalidCode(code, item, type = "MemberExpression") {
   let message = `${item} is a possible Cross Process Object Wrapper (CPOW).`;
   let obj = wrapCode(code);
-  obj.errors = [{message, type: "MemberExpression"}];
+  obj.errors = [{message, type}];
   return obj;
 }
 
 ruleTester.run("no-cpows-in-tests", rule, {
   valid: [
     "window.document",
-    wrapCode("ContentTask.spawn(browser, null, () => { content.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);')
   ],
   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("let x = window.content;", "window.content"),
+    invalidCode("content.document;", "content.document"),
+    invalidCode("let x = content;", "content", "Identifier"),
+    invalidCode("let x = gBrowser.contentWindow.wrappedJSObject", "gBrowser.contentWindow")
   ]
 });