From 49516e37552829802e129eab6a852802aebd3f79 Mon Sep 17 00:00:00 2001 From: Mark Banner Date: Wed, 4 Jan 2017 19:37:43 +0000 Subject: [PATCH] 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 | 1 + .../test/browser_UnsubmittedCrashHandler.js | 8 +-- .../tests/xpcshell/test_crash_manager.js | 4 +- .../tests/xpcshell/test_crash_service.js | 2 +- .../tests/xpcshell/test_crash_store.js | 8 ++- .../modules/tests/browser/browser_Battery.js | 8 +-- .../docs/linters/eslint-plugin-mozilla.rst | 11 +++++ .../eslint/eslint-plugin-mozilla/lib/index.js | 2 + .../rules/no-import-into-var-and-global.js | 49 +++++++++++++++++++ 9 files changed, 77 insertions(+), 16 deletions(-) create mode 100644 tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-import-into-var-and-global.js diff --git a/.eslintrc.js b/.eslintrc.js index 52982e1d5270..226546cf2aef 100644 --- a/.eslintrc.js +++ b/.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", diff --git a/browser/modules/test/browser_UnsubmittedCrashHandler.js b/browser/modules/test/browser_UnsubmittedCrashHandler.js index 79d1689026a6..441e20930f43 100644 --- a/browser/modules/test/browser_UnsubmittedCrashHandler.js +++ b/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"; diff --git a/toolkit/components/crashes/tests/xpcshell/test_crash_manager.js b/toolkit/components/crashes/tests/xpcshell/test_crash_manager.js index 85c00a31e01e..e2683f123e6d 100644 --- a/toolkit/components/crashes/tests/xpcshell/test_crash_manager.js +++ b/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(); diff --git a/toolkit/components/crashes/tests/xpcshell/test_crash_service.js b/toolkit/components/crashes/tests/xpcshell/test_crash_service.js index c207057e0147..aadf1b86bb11 100644 --- a/toolkit/components/crashes/tests/xpcshell/test_crash_service.js +++ b/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(); diff --git a/toolkit/components/crashes/tests/xpcshell/test_crash_store.js b/toolkit/components/crashes/tests/xpcshell/test_crash_store.js index e18e595a2d05..15ed54652e38 100644 --- a/toolkit/components/crashes/tests/xpcshell/test_crash_store.js +++ b/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)); diff --git a/toolkit/modules/tests/browser/browser_Battery.js b/toolkit/modules/tests/browser/browser_Battery.js index 93a93f44da43..e5139bfabe3a 100644 --- a/toolkit/modules/tests/browser/browser_Battery.js +++ b/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(); }); }); diff --git a/tools/lint/docs/linters/eslint-plugin-mozilla.rst b/tools/lint/docs/linters/eslint-plugin-mozilla.rst index e75864238582..8df2305a38a3 100644 --- a/tools/lint/docs/linters/eslint-plugin-mozilla.rst +++ b/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 ----------------------------- diff --git a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js b/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js index e1f694c3602e..8920c8065574 100644 --- a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js +++ b/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 diff --git a/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-import-into-var-and-global.js b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-import-into-var-and-global.js new file mode 100644 index 000000000000..c1c226188637 --- /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."); + } + } + } + }; +};