Bug 1328565 - Prevent cases of Cu.import importing into variables and global scope at the same time. r=mossop

MozReview-Commit-ID: CXly2RhNpRP

--HG--
extra : rebase_source : 9b9a8542bbd437aa0160c122b1b826e08de03009

.eslintrc.js

@@ -7,6 +7,7 @@ module.exports = {
   ],
   "rules": {
     "mozilla/import-globals": "warn",
+    "mozilla/no-import-into-var-and-global": "error",
 
     // No (!foo in bar) or (!object instanceof Class)
     "no-unsafe-negation": "error",

browser/modules/test/browser_UnsubmittedCrashHandler.js

@@ -6,13 +6,13 @@
  */
 
 const { UnsubmittedCrashHandler } =
-  Cu.import("resource:///modules/ContentCrashHandlers.jsm", this);
+  Cu.import("resource:///modules/ContentCrashHandlers.jsm", {});
 const { FileUtils } =
-  Cu.import("resource://gre/modules/FileUtils.jsm", this);
+  Cu.import("resource://gre/modules/FileUtils.jsm", {});
 const { makeFakeAppDir }  =
-  Cu.import("resource://testing-common/AppData.jsm", this);
+  Cu.import("resource://testing-common/AppData.jsm", {});
 const { OS } =
-  Cu.import("resource://gre/modules/osfile.jsm", this);
+  Cu.import("resource://gre/modules/osfile.jsm", {});
 
 const DAY = 24 * 60 * 60 * 1000; // milliseconds
 const SERVER_URL = "http://example.com/browser/toolkit/crashreporter/test/browser/crashreport.sjs";

toolkit/components/crashes/tests/xpcshell/test_crash_manager.js

@@ -5,7 +5,7 @@
 
 var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
-var bsp = Cu.import("resource://gre/modules/CrashManager.jsm", this);
+var {CrashStore, CrashManager} = Cu.import("resource://gre/modules/CrashManager.jsm", {});
 Cu.import("resource://gre/modules/Promise.jsm", this);
 Cu.import("resource://gre/modules/Task.jsm", this);
 Cu.import("resource://gre/modules/osfile.jsm", this);
@@ -362,7 +362,7 @@ add_task(function* test_high_water_mark() {
   }
 
   let count = yield m.aggregateEventsFiles();
-  Assert.equal(count, bsp.CrashStore.prototype.HIGH_WATER_DAILY_THRESHOLD + 1);
+  Assert.equal(count, CrashStore.prototype.HIGH_WATER_DAILY_THRESHOLD + 1);
 
   // Need to fetch again in case the first one was garbage collected.
   store = yield m._getStore();

toolkit/components/crashes/tests/xpcshell/test_crash_service.js

@@ -7,7 +7,7 @@ var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
 Cu.import("resource://gre/modules/Services.jsm", this);
 Cu.import("resource://testing-common/AppData.jsm", this);
-var bsp = Cu.import("resource://gre/modules/CrashManager.jsm", this);
+var bsp = Cu.import("resource://gre/modules/CrashManager.jsm", {});
 
 function run_test() {
   run_next_test();

toolkit/components/crashes/tests/xpcshell/test_crash_store.js

@@ -9,7 +9,7 @@
 
 var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
-var bsp = Cu.import("resource://gre/modules/CrashManager.jsm", this);
+var {CrashManager, CrashStore, dateToDays} = Cu.import("resource://gre/modules/CrashManager.jsm", {});
 Cu.import("resource://gre/modules/osfile.jsm", this);
 Cu.import("resource://gre/modules/Task.jsm", this);
 
@@ -31,8 +31,6 @@ const {
   SUBMISSION_RESULT_FAILED,
 } = CrashManager.prototype;
 
-const CrashStore = bsp.CrashStore;
-
 var STORE_DIR_COUNT = 0;
 
 function getStore() {
@@ -469,8 +467,8 @@ add_task(function* test_high_water() {
   Assert.equal(crashes.length, 2 * s.HIGH_WATER_DAILY_THRESHOLD);
 
   // But raw counts should be preserved.
-  let day1 = bsp.dateToDays(d1);
-  let day2 = bsp.dateToDays(d2);
+  let day1 = dateToDays(d1);
+  let day2 = dateToDays(d2);
   Assert.ok(s._countsByDay.has(day1));
   Assert.ok(s._countsByDay.has(day2));
 

toolkit/modules/tests/browser/browser_Battery.js

@@ -2,13 +2,13 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
-var imported = Components.utils.import("resource://gre/modules/Battery.jsm", this);
+var {GetBattery, Debugging} = Components.utils.import("resource://gre/modules/Battery.jsm", {});
 Cu.import("resource://gre/modules/Services.jsm", this);
 
 function test() {
   waitForExplicitFinish();
 
-  is(imported.Debugging.fake, false, "Battery spoofing is initially false")
+  is(Debugging.fake, false, "Battery spoofing is initially false")
 
   GetBattery().then(function(battery) {
     for (let k of ["charging", "chargingTime", "dischargingTime", "level"]) {
@@ -24,7 +24,7 @@ function test() {
       is(battery[k], backup, "Setting battery " + k + " preference without spoofing enabled should fail");
     }
 
-    imported.Debugging.fake = true;
+    Debugging.fake = true;
 
     // reload again to get the fake one
     GetBattery().then(function(battery) {
@@ -44,7 +44,7 @@ function test() {
 
       // Resetting the value to make the test run successful
       // for multiple runs in same browser session.
-      imported.Debugging.fake = false;
+      Debugging.fake = false;
       finish();
     });
   });

tools/lint/docs/linters/eslint-plugin-mozilla.rst

@@ -113,6 +113,17 @@ Rejects calls to "Cu.import" that do not supply a second argument (meaning they
 add the exported properties into global scope).
 
 
+no-import-into-var-and-global
+-----------------------------
+
+Reject use of ``Cu.import`` (or ``Components.utils.import``) where it attempts to
+import into a var and into the global scope at the same time, e.g.
+
+``var foo = Cu.import("path.jsm", this);``
+
+This is considered bad practice as it is confusing as to what is actually being
+imported.
+
 reject-importGlobalProperties
 -----------------------------
 

tools/lint/eslint/eslint-plugin-mozilla/lib/index.js

@@ -25,6 +25,7 @@ module.exports = {
     "no-aArgs": require("../lib/rules/no-aArgs"),
     "no-cpows-in-tests": require("../lib/rules/no-cpows-in-tests"),
     "no-single-arg-cu-import": require("../lib/rules/no-single-arg-cu-import"),
+    "no-import-into-var-and-global": require("../lib/rules/no-import-into-var-and-global.js"),
     "reject-importGlobalProperties": require("../lib/rules/reject-importGlobalProperties"),
     "reject-some-requires": require("../lib/rules/reject-some-requires"),
     "var-only-at-top-level": require("../lib/rules/var-only-at-top-level")
@@ -38,6 +39,7 @@ module.exports = {
     "no-aArgs": 0,
     "no-cpows-in-tests": 0,
     "no-single-arg-cu-import": 0,
+    "no-import-into-var-and-global": 0,
     "reject-importGlobalProperties": 0,
     "reject-some-requires": 0,
     "var-only-at-top-level": 0

tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-import-into-var-and-global.js

@@ -0,0 +1,49 @@
+/**
+ * @fileoverview Reject use of Cu.import where it attempts to import into
+ *               a var and into the global scope, e.g.
+ *               var foo = Cu.import("path.jsm", this);
+ *
+ *               This is considered bad practice as it is confusing as to what
+ *               is actually being imported.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+"use strict";
+
+// -----------------------------------------------------------------------------
+// Rule Definition
+// -----------------------------------------------------------------------------
+
+var helpers = require("../helpers");
+
+module.exports = function(context) {
+
+  // ---------------------------------------------------------------------------
+  // Public
+  //  --------------------------------------------------------------------------
+
+  return {
+    "CallExpression": function(node) {
+      if (node.callee.type === "MemberExpression" &&
+          node.parent.type === "VariableDeclarator" &&
+          node.arguments.length === 2) {
+        let memexp = node.callee;
+        if (((memexp.object.type === "Identifier" &&
+              memexp.object.name === "Cu") ||
+             (memexp.object.type === "MemberExpression" &&
+              memexp.object.object && memexp.object.property &&
+              memexp.object.object.name === "Components" &&
+              memexp.object.property.name === "utils")) &&
+            memexp.property.type === "Identifier" &&
+            memexp.property.name === "import" &&
+            node.arguments[1].type === "ThisExpression") {
+          context.report(node, "Cu.import imports into variables and into " +
+                         "global scope.");
+        }
+      }
+    }
+  };
+};