Bug 1434563 - Do not execute helper if the function is defined in content; r=Honza.
authorNicolas Chevobbe <nchevobbe@mozilla.com>
Wed, 18 Jul 2018 11:51:17 +0000
changeset 427108 9b6c65bb4719bbf551fbe72e84ce465981ef694c
parent 427107 de3e3149c345b02c4967caf771698f9b166623c2
child 427109 f32ce7c754f0180db94244ada289915c8f9c5b2e
push id66507
push usernchevobbe@mozilla.com
push dateWed, 18 Jul 2018 11:54:44 +0000
treeherderautoland@9b6c65bb4719 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersHonza
bugs1434563
milestone63.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
Bug 1434563 - Do not execute helper if the function is defined in content; r=Honza. If the content page sets a global keys, values, , … function, calling them in the jsterm won't work because the helpers we provide will override them. This is very disturbing when debugging code, and this patch fixes this issue by looking at all the registered helpers and clearing helpers bindings if a function with the same name as an helper exists in the content page. This is not done for the print helper though, since the print function on window always exists. We also take this as an opportunity to only return unique item in the autocomplete server function so we don't show a function twice in the popup if it is defined in content and as an helper. The test that was asserting this behaviour for $ and $$ is renamed and modified to test all the helpers. MozReview-Commit-ID: 1bTliGUb39U Differential Revision: https://phabricator.services.mozilla.com/D2156
devtools/client/webconsole/test/mochitest/browser.ini
devtools/client/webconsole/test/mochitest/browser_jsterm_content_defined_helpers.js
devtools/client/webconsole/test/mochitest/browser_jsterm_dollar.js
devtools/client/webconsole/test/mochitest/test-jsterm-dollar.html
devtools/server/actors/webconsole.js
--- a/devtools/client/webconsole/test/mochitest/browser.ini
+++ b/devtools/client/webconsole/test/mochitest/browser.ini
@@ -97,17 +97,16 @@ support-files =
   test-ineffective-iframe-sandbox-warning3.html
   test-ineffective-iframe-sandbox-warning4.html
   test-ineffective-iframe-sandbox-warning5.html
   test-insecure-frame.html
   test-insecure-passwords-about-blank-web-console-warning.html
   test-insecure-passwords-web-console-warning.html
   test-inspect-cross-domain-objects-frame.html
   test-inspect-cross-domain-objects-top.html
-  test-jsterm-dollar.html
   test_jsterm_screenshot_command.html
   test-local-session-storage.html
   test-location-debugger-link-console-log.js
   test-location-debugger-link-errors.js
   test-location-debugger-link.html
   test-location-styleeditor-link-1.css
   test-location-styleeditor-link-2.css
   test-location-styleeditor-link.html
@@ -195,22 +194,22 @@ skip-if = verify
 [browser_jsterm_autocomplete_inside_text.js]
 [browser_jsterm_autocomplete_native_getters.js]
 [browser_jsterm_autocomplete_nav_and_tab_key.js]
 [browser_jsterm_autocomplete_paste_undo.js]
 [browser_jsterm_autocomplete_return_key_no_selection.js]
 [browser_jsterm_autocomplete_return_key.js]
 [browser_jsterm_autocomplete-properties-with-non-alphanumeric-names.js]
 [browser_jsterm_completion.js]
+[browser_jsterm_content_defined_helpers.js]
 [browser_jsterm_copy_command.js]
 [browser_jsterm_ctrl_a_select_all.js]
 [browser_jsterm_ctrl_key_nav.js]
 skip-if = os != 'mac' # The tested ctrl+key shortcuts are OSX only
 [browser_jsterm_document_no_xray.js]
-[browser_jsterm_dollar.js]
 [browser_jsterm_error_docs.js]
 [browser_jsterm_error_outside_valid_range.js]
 [browser_jsterm_helper_clear.js]
 [browser_jsterm_helper_dollar_dollar.js]
 [browser_jsterm_helper_dollar_x.js]
 [browser_jsterm_helper_dollar.js]
 [browser_jsterm_helper_help.js]
 [browser_jsterm_helper_keys_values.js]
rename from devtools/client/webconsole/test/mochitest/browser_jsterm_dollar.js
rename to devtools/client/webconsole/test/mochitest/browser_jsterm_content_defined_helpers.js
--- a/devtools/client/webconsole/test/mochitest/browser_jsterm_dollar.js
+++ b/devtools/client/webconsole/test/mochitest/browser_jsterm_content_defined_helpers.js
@@ -1,39 +1,63 @@
 /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
 /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
-// Test that using `$` and `$$` in jsterm call the global content functions
-// if they are defined. See Bug 621644.
+// Test that using helper functions in jsterm call the global content functions
+// if they are defined.
 
-const TEST_URI = "http://example.com/browser/devtools/client/webconsole/" +
-                 "test/mochitest/test-jsterm-dollar.html";
+const PREFIX = "content-";
+const HELPERS = [
+  "$_",
+  "$",
+  "$$",
+  "$0",
+  "$x",
+  "cd",
+  "clear",
+  "clearHistory",
+  "copy",
+  "help",
+  "inspect",
+  "keys",
+  "pprint",
+  "screenshot",
+  "values",
+];
+
+// The page script sets a global function for each known helper (except print).
+const TEST_URI = `data:text/html,<meta charset=utf8>
+  <script>
+    const helpers = ${JSON.stringify(HELPERS)};
+    for (const helper of helpers) {
+      window[helper] = () => "${PREFIX}" + helper;
+    }
+  </script>`;
 
 add_task(async function() {
   // Run test with legacy JsTerm
   await performTests();
   // And then run it with the CodeMirror-powered one.
   await pushPref("devtools.webconsole.jsterm.codeMirror", true);
   await performTests();
 });
 
 async function performTests() {
-  const hud = await openNewTabAndConsole(TEST_URI);
-  await test$(hud);
-  await test$$(hud);
+  const {jsterm} = await openNewTabAndConsole(TEST_URI);
+  const {autocompletePopup} = jsterm;
+
+  for (const helper of HELPERS) {
+    await jstermSetValueAndComplete(jsterm, helper);
+    const autocompleteItems = getPopupLabels(autocompletePopup).filter(l => l === helper);
+    is(autocompleteItems.length, 1,
+      `There's no duplicated "${helper}" item in the autocomplete popup`);
+    const msg = await jsterm.execute(`${helper}()`);
+    ok(msg.textContent.includes(PREFIX + helper), `output is correct for ${helper}()`);
+  }
 }
 
-async function test$(hud) {
-  hud.ui.clearOutput();
-  const msg = await hud.jsterm.execute("$(document.body)");
-  ok(msg.textContent.includes("<p>"), "jsterm output is correct for $()");
+function getPopupLabels(popup) {
+  return popup.getItems().map(item => item.label);
 }
-
-async function test$$(hud) {
-  hud.ui.clearOutput();
-  hud.jsterm.setInputValue();
-  const msg = await hud.jsterm.execute("$$(document)");
-  ok(msg.textContent.includes("621644"), "jsterm output is correct for $$()");
-}
deleted file mode 100644
--- a/devtools/client/webconsole/test/mochitest/test-jsterm-dollar.html
+++ /dev/null
@@ -1,24 +0,0 @@
-<!DOCTYPE html>
-<html dir="ltr" xml:lang="en-US" lang="en-US">
-  <head>
-    <meta charset="utf-8">
-    <title>Web Console test for bug 621644</title>
-    <script>
-      /* eslint-disable */
-      function $(elem) {
-        return elem.innerHTML;
-      }
-      function $$(doc) {
-        return doc.title;
-      }
-    </script>
-    <!--
-       - Any copyright is dedicated to the Public Domain.
-       - http://creativecommons.org/publicdomain/zero/1.0/
-       -->
-  </head>
-  <body>
-    <h1>Web Console test for bug 621644</h1>
-    <p>hello world!</p>
-  </body>
-</html>
--- a/devtools/server/actors/webconsole.js
+++ b/devtools/server/actors/webconsole.js
@@ -22,16 +22,17 @@ const ErrorDocs = require("devtools/serv
 loader.lazyRequireGetter(this, "NetworkMonitor", "devtools/shared/webconsole/network-monitor", true);
 loader.lazyRequireGetter(this, "NetworkMonitorChild", "devtools/shared/webconsole/network-monitor", true);
 loader.lazyRequireGetter(this, "NetworkEventActor", "devtools/server/actors/network-event", true);
 loader.lazyRequireGetter(this, "ConsoleProgressListener", "devtools/shared/webconsole/network-monitor", true);
 loader.lazyRequireGetter(this, "StackTraceCollector", "devtools/shared/webconsole/network-monitor", true);
 loader.lazyRequireGetter(this, "JSPropertyProvider", "devtools/shared/webconsole/js-property-provider", true);
 loader.lazyRequireGetter(this, "Parser", "resource://devtools/shared/Parser.jsm", true);
 loader.lazyRequireGetter(this, "NetUtil", "resource://gre/modules/NetUtil.jsm", true);
+loader.lazyRequireGetter(this, "WebConsoleCommands", "devtools/server/actors/webconsole/utils", true);
 loader.lazyRequireGetter(this, "addWebConsoleCommands", "devtools/server/actors/webconsole/utils", true);
 loader.lazyRequireGetter(this, "formatCommand", "devtools/server/actors/webconsole/commands", true);
 loader.lazyRequireGetter(this, "isCommand", "devtools/server/actors/webconsole/commands", true);
 loader.lazyRequireGetter(this, "CONSOLE_WORKER_IDS", "devtools/server/actors/webconsole/utils", true);
 loader.lazyRequireGetter(this, "WebConsoleUtils", "devtools/server/actors/webconsole/utils", true);
 loader.lazyRequireGetter(this, "EnvironmentActor", "devtools/server/actors/environment", true);
 loader.lazyRequireGetter(this, "EventEmitter", "devtools/shared/event-emitter");
 
@@ -1129,19 +1130,24 @@ WebConsoleActor.prototype =
       matches = matches.concat(this._webConsoleCommandsCache
           .filter(n =>
             // filter out `screenshot` command as it is inaccessible without
             // the `:` prefix
             n !== "screenshot" && n.startsWith(result.matchProp)
           ));
     }
 
+    // Make sure we return an array with unique items, since `matches` can hold twice
+    // the same function name if it was defined in the content page and match an helper
+    // function (e.g. $, keys, …).
+    matches = [...new Set(matches)].sort();
+
     return {
       from: this.actorID,
-      matches: matches.sort(),
+      matches,
       matchProp: result.matchProp,
     };
   },
 
   /**
    * The "clearMessagesCache" request handler.
    */
   clearMessagesCache: function() {
@@ -1413,49 +1419,45 @@ WebConsoleActor.prototype =
 
     if (options.selectedNodeActor) {
       const actor = this.conn.getActor(options.selectedNodeActor);
       if (actor) {
         helpers.selectedNode = actor.rawNode;
       }
     }
 
-    // Check if the Debugger.Frame or Debugger.Object for the global include
-    // $ or $$. We will not overwrite these functions with the Web Console
-    // commands.
-    let found$ = false, found$$ = false, disableScreenshot = false;
+    // Check if the Debugger.Frame or Debugger.Object for the global include any of the
+    // helper function we set. We will not overwrite these functions with the Web Console
+    // commands. The exception being "print" which should exist everywhere as
+    // `window.print`, and that we don't want to trigger from the console.
+    const availableHelpers = [...WebConsoleCommands._originalCommands.keys()]
+      .filter(h => h !== "print");
+
+    let helpersToDisable = [];
+    const helperCache = {};
+
     // do not override command functions if we are using the command key `:`
     // before the command string
     if (!isCmd) {
-      // if we do not have the command key as a prefix, screenshot is disabled by default
-      disableScreenshot = true;
       if (frame) {
         const env = frame.environment;
         if (env) {
-          found$ = !!env.find("$");
-          found$$ = !!env.find("$$");
+          helpersToDisable = availableHelpers.filter(name => !!env.find(name));
         }
       } else {
-        found$ = !!dbgWindow.getOwnPropertyDescriptor("$");
-        found$$ = !!dbgWindow.getOwnPropertyDescriptor("$$");
+        helpersToDisable = availableHelpers.filter(name =>
+          !!dbgWindow.getOwnPropertyDescriptor(name));
       }
+      // if we do not have the command key as a prefix, screenshot is disabled by default
+      helpersToDisable.push("screenshot");
     }
 
-    let $ = null, $$ = null, screenshot = null;
-    if (found$) {
-      $ = bindings.$;
-      delete bindings.$;
-    }
-    if (found$$) {
-      $$ = bindings.$$;
-      delete bindings.$$;
-    }
-    if (disableScreenshot) {
-      screenshot = bindings.screenshot;
-      delete bindings.screenshot;
+    for (const helper of helpersToDisable) {
+      helperCache[helper] = bindings[helper];
+      delete bindings[helper];
     }
 
     // Ready to evaluate the string.
     helpers.evalInput = string;
 
     let evalOptions;
     if (typeof options.url == "string") {
       evalOptions = { url: options.url };
@@ -1542,24 +1544,18 @@ WebConsoleActor.prototype =
       }
     }
 
     const helperResult = helpers.helperResult;
     delete helpers.evalInput;
     delete helpers.helperResult;
     delete helpers.selectedNode;
 
-    if ($) {
-      bindings.$ = $;
-    }
-    if ($$) {
-      bindings.$$ = $$;
-    }
-    if (screenshot) {
-      bindings.screenshot = screenshot;
+    for (const [helperName, helper] of Object.entries(helperCache)) {
+      bindings[helperName] = helper;
     }
 
     if (bindings._self) {
       delete bindings._self;
     }
 
     return {
       result: result,