author | Ed Lee <edilee@mozilla.com> |
Tue, 14 Nov 2017 02:17:57 -0800 | |
changeset 393075 | 257de1de1304548c1dcb2c7d8c66a20d26c021e6 |
parent 393074 | 2541054da0cb9f23cfa5fdf3ef08379a4696baad |
child 393076 | 366c4c091bb3dba617776f06a03ac63875fa6081 |
push id | 55833 |
push user | edilee@gmail.com |
push date | Wed, 22 Nov 2017 13:55:24 +0000 |
treeherder | autoland@257de1de1304 [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | standard8 |
bugs | 1417017 |
milestone | 59.0a1 |
first release with | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
last release without | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
--- 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") ] });