Bug 1328565 - Prevent cases of Cu.import importing into variables and global scope at the same time. r=mossop
authorMark Banner <standard8@mozilla.com>
Wed, 04 Jan 2017 19:37:43 +0000
changeset 456678 b6d798426ca0d98b1ae703783bf89aa9bf56407b
parent 456677 f01970b5e82b7c108e70788f501f667fe74cdfe9
child 456679 8b56ca2a166270dc1b1b1ae01f538d00c06270af
push id40575
push userjwwang@mozilla.com
push dateFri, 06 Jan 2017 02:27:46 +0000
reviewersmossop
bugs1328565
milestone53.0a1
Bug 1328565 - Prevent cases of Cu.import importing into variables and global scope at the same time. r=mossop MozReview-Commit-ID: CXly2RhNpRP
.eslintrc.js
browser/modules/test/browser_UnsubmittedCrashHandler.js
toolkit/components/crashes/tests/xpcshell/test_crash_manager.js
toolkit/components/crashes/tests/xpcshell/test_crash_service.js
toolkit/components/crashes/tests/xpcshell/test_crash_store.js
toolkit/modules/tests/browser/browser_Battery.js
tools/lint/docs/linters/eslint-plugin-mozilla.rst
tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-import-into-var-and-global.js
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -2,16 +2,17 @@
 
 module.exports = {
   // When adding items to this file please check for effects on sub-directories.
   "plugins": [
     "mozilla"
   ],
   "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",
   },
   "env": {
     "es6": true
   },
   "parserOptions": {
--- a/browser/modules/test/browser_UnsubmittedCrashHandler.js
+++ b/browser/modules/test/browser_UnsubmittedCrashHandler.js
@@ -1,23 +1,23 @@
 "use strict";
 
 /**
  * This suite tests the "unsubmitted crash report" notification
  * that is seen when we detect pending crash reports on startup.
  */
 
 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";
 
 /**
  * Returns the directly where the browsing is storing the
  * pending crash reports.
  *
--- a/toolkit/components/crashes/tests/xpcshell/test_crash_manager.js
+++ b/toolkit/components/crashes/tests/xpcshell/test_crash_manager.js
@@ -1,16 +1,16 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 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);
 Cu.import("resource://gre/modules/TelemetryEnvironment.jsm", this);
 
 Cu.import("resource://testing-common/CrashManagerTest.jsm", this);
 Cu.import("resource://testing-common/TelemetryArchiveTesting.jsm", this);
 
@@ -357,17 +357,17 @@ add_task(function* test_high_water_mark(
 
   let store = yield m._getStore();
 
   for (let i = 0; i < store.HIGH_WATER_DAILY_THRESHOLD + 1; i++) {
     yield m.createEventsFile("m" + i, "crash.main.2", DUMMY_DATE, "m" + i);
   }
 
   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();
 
   Assert.equal(store.crashesCount, store.HIGH_WATER_DAILY_THRESHOLD + 1);
 });
 
 add_task(function* test_addCrash() {
--- a/toolkit/components/crashes/tests/xpcshell/test_crash_service.js
+++ b/toolkit/components/crashes/tests/xpcshell/test_crash_service.js
@@ -2,17 +2,17 @@
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 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();
 }
 
 add_task(function* test_instantiation() {
   Assert.ok(!bsp.gCrashManager, "CrashManager global instance not initially defined.");
 
--- a/toolkit/components/crashes/tests/xpcshell/test_crash_store.js
+++ b/toolkit/components/crashes/tests/xpcshell/test_crash_store.js
@@ -4,17 +4,17 @@
 /*
  * This file tests the CrashStore type in CrashManager.jsm.
  */
 
 "use strict";
 
 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);
 
 const DUMMY_DATE = new Date(Date.now() - 10 * 24 * 60 * 60 * 1000);
 DUMMY_DATE.setMilliseconds(0);
 
 const DUMMY_DATE_2 = new Date(Date.now() - 5 * 24 * 60 * 60 * 1000);
 DUMMY_DATE_2.setMilliseconds(0);
@@ -26,18 +26,16 @@ const {
   PROCESS_TYPE_GMPLUGIN,
   PROCESS_TYPE_GPU,
   CRASH_TYPE_CRASH,
   CRASH_TYPE_HANG,
   SUBMISSION_RESULT_OK,
   SUBMISSION_RESULT_FAILED,
 } = CrashManager.prototype;
 
-const CrashStore = bsp.CrashStore;
-
 var STORE_DIR_COUNT = 0;
 
 function getStore() {
   return Task.spawn(function* () {
     let storeDir = do_get_tempdir().path;
     storeDir = OS.Path.join(storeDir, "store-" + STORE_DIR_COUNT++);
 
     yield OS.File.makeDir(storeDir, {unixMode: OS.Constants.libc.S_IRWXU});
@@ -464,18 +462,18 @@ add_task(function* test_high_water() {
   Assert.equal(crashes.length, 2 * s.HIGH_WATER_DAILY_THRESHOLD);
 
   crashes = s.getCrashesOfType(PROCESS_TYPE_PLUGIN, CRASH_TYPE_CRASH);
   Assert.equal(crashes.length, 2 * s.HIGH_WATER_DAILY_THRESHOLD);
   crashes = s.getCrashesOfType(PROCESS_TYPE_PLUGIN, CRASH_TYPE_HANG);
   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));
 
   Assert.equal(s._countsByDay.get(day1).
                  get(PROCESS_TYPE_MAIN + "-" + CRASH_TYPE_CRASH),
                s.HIGH_WATER_DAILY_THRESHOLD + 1);
   Assert.equal(s._countsByDay.get(day1).
                  get(PROCESS_TYPE_MAIN + "-" + CRASH_TYPE_HANG),
--- a/toolkit/modules/tests/browser/browser_Battery.js
+++ b/toolkit/modules/tests/browser/browser_Battery.js
@@ -1,35 +1,35 @@
 /* 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";
-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"]) {
       let backup = battery[k];
       try {
         battery[k] = "__magic__";
       } catch (e) {
         // We are testing that we cannot set battery to new values
         // when "use strict" is enabled, this throws a TypeError
         if (e.name != "TypeError")
           throw e;
       }
       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) {
       battery.charging = true;
       battery.chargingTime = 100;
       battery.level = 0.5;
       ok(battery.charging, "Test for charging setter");
       is(battery.chargingTime, 100, "Test for chargingTime setter");
@@ -39,13 +39,13 @@ function test() {
       battery.dischargingTime = 50;
       battery.level = 0.7;
       ok(!battery.charging, "Test for charging setter");
       is(battery.dischargingTime, 50, "Test for dischargingTime setter");
       is(battery.level, 0.7, "Test for level setter");
 
       // Resetting the value to make the test run successful
       // for multiple runs in same browser session.
-      imported.Debugging.fake = false;
+      Debugging.fake = false;
       finish();
     });
   });
 }
--- a/tools/lint/docs/linters/eslint-plugin-mozilla.rst
+++ b/tools/lint/docs/linters/eslint-plugin-mozilla.rst
@@ -108,16 +108,27 @@ object is assigned to another variable e
 
 no-single-arg-cu-import
 -----------------------
 
 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
 -----------------------------
 
 Rejects calls to ``Cu.importGlobalProperties``.  Use of this function is
 undesirable in some parts of the tree.
 
 
 reject-some-requires
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
@@ -20,26 +20,28 @@ module.exports = {
     "balanced-listeners": require("../lib/rules/balanced-listeners"),
     "import-globals": require("../lib/rules/import-globals"),
     "import-headjs-globals": require("../lib/rules/import-headjs-globals"),
     "import-browserjs-globals": require("../lib/rules/import-browserjs-globals"),
     "mark-test-function-used": require("../lib/rules/mark-test-function-used"),
     "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")
   },
   rulesConfig: {
     "balanced-listeners": 0,
     "import-globals": 0,
     "import-headjs-globals": 0,
     "import-browserjs-globals": 0,
     "mark-test-function-used": 0,
     "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
   }
 };
new file mode 100644
--- /dev/null
+++ b/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.");
+        }
+      }
+    }
+  };
+};