From c7b8502fb8f769d104f070e64db2cd6136bad7ae Mon Sep 17 00:00:00 2001 From: chujun Date: Wed, 12 Jun 2019 12:10:35 +0000 Subject: [PATCH] Bug 1536854 - Log errors from logpoint as actual errors in the console r=davidwalsh,nchevobbe If logpoint throws, set `level` to "error". Add tests to ensure the correct level is set. Demo function: https://luxuriant-system.glitch.me/ Add some logpoints. The first two statements are invalid. The third one is valid. {F1311386} In console: {F1311387} Differential Revision: https://phabricator.services.mozilla.com/D31157 --HG-- extra : moz-landing-system : lando --- .../test/mochitest/browser_dbg-log-points.js | 2 +- devtools/client/shared/components/Frame.js | 16 +++++ .../shared/components/test/mochitest/head.js | 9 ++- .../test/mochitest/test_frame_01.html | 18 +++++ devtools/client/webconsole/utils/messages.js | 5 ++ devtools/server/actors/breakpoint.js | 7 +- .../server/tests/unit/test_logpoint-01.js | 1 + .../server/tests/unit/test_logpoint-03.js | 69 +++++++++++++++++++ devtools/server/tests/unit/xpcshell.ini | 1 + 9 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 devtools/server/tests/unit/test_logpoint-03.js diff --git a/devtools/client/debugger/test/mochitest/browser_dbg-log-points.js b/devtools/client/debugger/test/mochitest/browser_dbg-log-points.js index 2af2b28621a1..289484f0ee6a 100644 --- a/devtools/client/debugger/test/mochitest/browser_dbg-log-points.js +++ b/devtools/client/debugger/test/mochitest/browser_dbg-log-points.js @@ -31,6 +31,6 @@ add_task(async function() { await hasConsoleMessage(dbg, "firstCall"); const { link, value } = await findConsoleMessage(dbg, "a b c"); - is(link, "script-switching-01.js:8:2", "logs should have the relevant link"); + is(link, "Logpoint @ script-switching-01.js:8:2", "logs should have the relevant link"); is(value, "a b c", "logs should have multiple values"); }); diff --git a/devtools/client/shared/components/Frame.js b/devtools/client/shared/components/Frame.js index b3445878a7e0..1c876ef0aedd 100644 --- a/devtools/client/shared/components/Frame.js +++ b/devtools/client/shared/components/Frame.js @@ -180,6 +180,22 @@ class Frame extends Component { } } + // If the message comes from a logPoint or conditional breakpoint, + // prefix the source location accordingly + if (frame.origin) { + let locationPrefix; + if (frame.origin === "logPoint") { + locationPrefix = "Logpoint @ "; + } + + if (locationPrefix) { + sourceElements.push(dom.span({ + key: "locationPrefix", + className: "frame-link-prefix", + }, locationPrefix)); + } + } + let displaySource = showFullSourceUrl ? unicodeLong : unicodeShort; if (isSourceMapped) { displaySource = getSourceMappedFile(displaySource); diff --git a/devtools/client/shared/components/test/mochitest/head.js b/devtools/client/shared/components/test/mochitest/head.js index 65d60e4661fc..eaa17121a97d 100644 --- a/devtools/client/shared/components/test/mochitest/head.js +++ b/devtools/client/shared/components/test/mochitest/head.js @@ -213,13 +213,14 @@ var TEST_TREE = { * Frame */ function checkFrameString({ - el, file, line, column, source, functionName, shouldLink, tooltip, + el, file, line, column, source, functionName, shouldLink, tooltip, locationPrefix, }) { const $ = selector => el.querySelector(selector); const $func = $(".frame-link-function-display-name"); const $source = $(".frame-link-source"); const $sourceInner = $(".frame-link-source-inner"); + const $locationPrefix = $(".frame-link-prefix"); const $filename = $(".frame-link-filename"); const $line = $(".frame-link-line"); @@ -249,6 +250,12 @@ function checkFrameString({ } else { ok(!$func, "Should not have an element for `functionName`"); } + + if (locationPrefix != null) { + is($locationPrefix.textContent, locationPrefix, "Correct prefix"); + } else { + ok(!$locationPrefix, "Should not have an element for `locationPrefix`"); + } } function checkSmartFrameString({ el, location, functionName, tooltip }) { diff --git a/devtools/client/shared/components/test/mochitest/test_frame_01.html b/devtools/client/shared/components/test/mochitest/test_frame_01.html index b32d2f7a8592..9deb2af1e589 100644 --- a/devtools/client/shared/components/test/mochitest/test_frame_01.html +++ b/devtools/client/shared/components/test/mochitest/test_frame_01.html @@ -314,6 +314,24 @@ window.onload = async function () { source: "https://bugzilla.mozilla.org/original.js", }); + // Check when a message comes from a logPoint, + // a prefix should render before source + await checkFrameComponent({ + frame: { + origin: "logPoint", + source: "http://myfile.com/mahscripts.js", + line: 55, + column: 10, + } + }, { + locationPrefix: "Logpoint @ ", + file: "mahscripts.js", + line: 55, + column: 10, + shouldLink: true, + tooltip: "View source in Debugger → http://myfile.com/mahscripts.js:55:10", + }); + function checkFrameComponent(input, expected) { let props = Object.assign({ onClick: () => {} }, input); let frame = ReactDOM.render(Frame(props), window.document.body); diff --git a/devtools/client/webconsole/utils/messages.js b/devtools/client/webconsole/utils/messages.js index 504f2c90ef06..f92c6a943081 100644 --- a/devtools/client/webconsole/utils/messages.js +++ b/devtools/client/webconsole/utils/messages.js @@ -238,6 +238,10 @@ function transformConsoleAPICallPacket(packet) { column: message.columnNumber, } : null; + if (type === "logPointError" || type === "logPoint") { + frame.origin = "logPoint"; + } + return new ConsoleMessage({ source: MESSAGE_SOURCE.CONSOLE_API, type, @@ -457,6 +461,7 @@ function getLevelFromType(type) { error: levels.LEVEL_ERROR, exception: levels.LEVEL_ERROR, assert: levels.LEVEL_ERROR, + logPointError: levels.LEVEL_ERROR, warn: levels.LEVEL_WARNING, info: levels.LEVEL_INFO, log: levels.LEVEL_LOG, diff --git a/devtools/server/actors/breakpoint.js b/devtools/server/actors/breakpoint.js index 4869a708b8aa..a3e2fc83f871 100644 --- a/devtools/server/actors/breakpoint.js +++ b/devtools/server/actors/breakpoint.js @@ -228,13 +228,16 @@ BreakpointActor.prototype = { const displayName = formatDisplayName(frame); const completion = frame.evalWithBindings(`[${logValue}]`, { displayName }); let value; + let level = "logPoint"; + if (!completion) { // The evaluation was killed (possibly by the slow script dialog). value = ["Log value evaluation incomplete"]; } else if ("return" in completion) { value = completion.return; } else { - value = [this.getThrownMessage(completion)]; + value = ["[Logpoint threw]: " + this.getThrownMessage(completion)]; + level = "logPointError"; } if (value && typeof value.unsafeDereference === "function") { @@ -245,8 +248,8 @@ BreakpointActor.prototype = { filename: sourceActor.url, lineNumber: line, columnNumber: column, - level: "logPoint", arguments: value, + level, }; this.threadActor._parent._consoleActor.onConsoleAPICall(message); diff --git a/devtools/server/tests/unit/test_logpoint-01.js b/devtools/server/tests/unit/test_logpoint-01.js index a30c3466980f..f181029651f8 100644 --- a/devtools/server/tests/unit/test_logpoint-01.js +++ b/devtools/server/tests/unit/test_logpoint-01.js @@ -63,6 +63,7 @@ function test_simple_breakpoint() { 1); /* eslint-enable */ + Assert.equal(lastMessage.level, "logPoint"); Assert.equal(lastMessage.arguments[0], "three"); finishClient(gClient); } diff --git a/devtools/server/tests/unit/test_logpoint-03.js b/devtools/server/tests/unit/test_logpoint-03.js new file mode 100644 index 000000000000..92215bb8c61e --- /dev/null +++ b/devtools/server/tests/unit/test_logpoint-03.js @@ -0,0 +1,69 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ +/* eslint-disable no-shadow, max-nested-callbacks */ + +"use strict"; + +/** + * Check that logpoints generate console errors if the logpoint statement is invalid. + */ + +var gDebuggee; +var gClient; +var gThreadClient; + +function run_test() { + initTestDebuggerServer(); + gDebuggee = addTestGlobal("test-logpoint"); + gClient = new DebuggerClient(DebuggerServer.connectPipe()); + gClient.connect().then(function() { + attachTestTabAndResume(gClient, "test-logpoint", + function(response, targetFront, threadClient) { + gThreadClient = threadClient; + test_simple_breakpoint(); + }); + }); + do_test_pending(); +} + +function test_simple_breakpoint() { + const rootActor = gClient.transport._serverConnection.rootActor; + const threadActor = rootActor._parameters.tabList._targetActors[0].threadActor; + + let lastMessage; + threadActor._parent._consoleActor = { + onConsoleAPICall(message) { + lastMessage = message; + }, + }; + + gThreadClient.once("paused", async function(packet) { + const source = await getSourceById( + gThreadClient, + packet.frame.where.actor + ); + + // Set a logpoint which should throw an error message. + gThreadClient.setBreakpoint({ + sourceUrl: source.url, + line: 3, + }, { logValue: "c" }); + + // Execute the rest of the code. + gThreadClient.resume(); + }); + + /* eslint-disable */ + Cu.evalInSandbox("debugger;\n" + // 1 + "var a = 'three';\n" + // 2 + "var b = 2;\n", // 3 + gDebuggee, + "1.8", + "test.js", + 1); + /* eslint-enable */ + + Assert.equal(lastMessage.level, "logPointError"); + Assert.equal(lastMessage.arguments[0], "[Logpoint threw]: c is not defined"); + finishClient(gClient); +} diff --git a/devtools/server/tests/unit/xpcshell.ini b/devtools/server/tests/unit/xpcshell.ini index 711015c3c976..ec4d2d0de9b6 100644 --- a/devtools/server/tests/unit/xpcshell.ini +++ b/devtools/server/tests/unit/xpcshell.ini @@ -139,6 +139,7 @@ skip-if = true # breakpoint sliding is not supported bug 1525685 [test_conditional_breakpoint-03.js] [test_logpoint-01.js] [test_logpoint-02.js] +[test_logpoint-03.js] [test_listsources-01.js] [test_listsources-02.js] [test_listsources-03.js]