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

--HG--
rename : devtools/client/webconsole/test/mochitest/browser_jsterm_dollar.js => devtools/client/webconsole/test/mochitest/browser_jsterm_content_defined_helpers.js
extra : moz-landing-system : lando

devtools/client/webconsole/test/mochitest/browser.ini

@@ -102,7 +102,6 @@ support-files =
   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
@@ -200,12 +199,12 @@ skip-if = verify
 [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]

devtools/client/webconsole/test/mochitest/browser_jsterm_content_defined_helpers.js

@@ -0,0 +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 helper functions in jsterm call the global content functions
+// if they are defined.
+
+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 {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}()`);
+  }
+}
+
+function getPopupLabels(popup) {
+  return popup.getItems().map(item => item.label);
+}

devtools/client/webconsole/test/mochitest/browser_jsterm_dollar.js

@@ -1,39 +0,0 @@
-/* -*- 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.
-
-const TEST_URI = "http://example.com/browser/devtools/client/webconsole/" +
-                 "test/mochitest/test-jsterm-dollar.html";
-
-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);
-}
-
-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 $()");
-}
-
-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 $$()");
-}

devtools/client/webconsole/test/mochitest/test-jsterm-dollar.html

@@ -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>

devtools/server/actors/webconsole.js

@@ -27,6 +27,7 @@ loader.lazyRequireGetter(this, "StackTraceCollector", "devtools/shared/webconsol
 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);
@@ -1134,9 +1135,14 @@ WebConsoleActor.prototype =
           ));
     }
 
+    // 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,
     };
   },
@@ -1418,39 +1424,35 @@ WebConsoleActor.prototype =
       }
     }
 
-    // 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.
@@ -1547,14 +1549,8 @@ WebConsoleActor.prototype =
     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) {