From 87bb1e27016c344fa2e968b6c0035119d809b8ca Mon Sep 17 00:00:00 2001 From: Alexandre Poirot Date: Tue, 18 Dec 2018 13:35:42 +0000 Subject: [PATCH] Bug 1513028 - Throw exception with stack and message when a module doesn't exists. r=jdescottes loadSubScript throws a string, without any stack/location. Consider all string exceptions as coming from loadSubScript and re-throw a real error object, coming with the stack of the require call. Also handle wrong require path with a more explicit error message. Differential Revision: https://phabricator.services.mozilla.com/D14139 --HG-- extra : moz-landing-system : lando --- devtools/server/actors/utils/call-watcher.js | 39 +++++++- devtools/shared/base-loader.js | 88 +++---------------- devtools/shared/tests/unit/test_require.js | 61 +++++++++++++ .../shared/tests/unit/throwing-module-1.js | 7 ++ .../shared/tests/unit/throwing-module-2.js | 8 ++ devtools/shared/tests/unit/xpcshell.ini | 2 + 6 files changed, 125 insertions(+), 80 deletions(-) create mode 100644 devtools/shared/tests/unit/throwing-module-1.js create mode 100644 devtools/shared/tests/unit/throwing-module-2.js diff --git a/devtools/server/actors/utils/call-watcher.js b/devtools/server/actors/utils/call-watcher.js index 92eef8feed09..0c990872a418 100644 --- a/devtools/server/actors/utils/call-watcher.js +++ b/devtools/server/actors/utils/call-watcher.js @@ -7,9 +7,6 @@ const {Cu} = require("chrome"); const ChromeUtils = require("ChromeUtils"); -// base-loader.js is a JSM without .jsm file extension, so it has to be loaded -// via ChromeUtils.import and not require() which would consider it as a CommonJS module -const {serializeStack, parseStack} = ChromeUtils.import("resource://devtools/shared/base-loader.js", {}); const { METHOD_FUNCTION, @@ -341,6 +338,42 @@ CallWatcher.prototype = { }, }; +function parseURI(uri) { + return String(uri).split(" -> ").pop(); +} + +function parseStack(stack) { + const lines = String(stack).split("\n"); + return lines.reduce(function(frames, line) { + if (line) { + const atIndex = line.indexOf("@"); + const columnIndex = line.lastIndexOf(":"); + const lineIndex = line.lastIndexOf(":", columnIndex - 1); + const fileName = parseURI(line.slice(atIndex + 1, lineIndex)); + const lineNumber = parseInt(line.slice(lineIndex + 1, columnIndex), 10); + const columnNumber = parseInt(line.slice(columnIndex + 1), 10); + const name = line.slice(0, atIndex).split("(").shift(); + frames.unshift({ + fileName: fileName, + name: name, + lineNumber: lineNumber, + columnNumber: columnNumber, + }); + } + return frames; + }, []); +} + +function serializeStack(frames) { + return frames.reduce(function(stack, frame) { + return frame.name + "@" + + frame.fileName + ":" + + frame.lineNumber + ":" + + frame.columnNumber + "\n" + + stack; + }, ""); +} + /** * Creates a new error from an error that originated from content but was called * from a wrapped overridden method. This is so we can make our own error diff --git a/devtools/shared/base-loader.js b/devtools/shared/base-loader.js index 4fde5d7d7fa1..e6e40b6706c1 100644 --- a/devtools/shared/base-loader.js +++ b/devtools/shared/base-loader.js @@ -48,50 +48,6 @@ function isRelative(id) { return id.startsWith("."); } -function sourceURI(uri) { - return String(uri).split(" -> ").pop(); -} - -function isntLoaderFrame(frame) { - return frame.fileName !== __URI__ && !frame.fileName.endsWith("/browser-loader.js"); -} - -function parseURI(uri) { - return String(uri).split(" -> ").pop(); -} - -function parseStack(stack) { - const lines = String(stack).split("\n"); - return lines.reduce(function(frames, line) { - if (line) { - const atIndex = line.indexOf("@"); - const columnIndex = line.lastIndexOf(":"); - const lineIndex = line.lastIndexOf(":", columnIndex - 1); - const fileName = parseURI(line.slice(atIndex + 1, lineIndex)); - const lineNumber = parseInt(line.slice(lineIndex + 1, columnIndex), 10); - const columnNumber = parseInt(line.slice(columnIndex + 1), 10); - const name = line.slice(0, atIndex).split("(").shift(); - frames.unshift({ - fileName: fileName, - name: name, - lineNumber: lineNumber, - columnNumber: columnNumber, - }); - } - return frames; - }, []); -} - -function serializeStack(frames) { - return frames.reduce(function(stack, frame) { - return frame.name + "@" + - frame.fileName + ":" + - frame.lineNumber + ":" + - frame.columnNumber + "\n" + - stack; - }, ""); -} - function readURI(uri) { const nsURI = NetUtil.newURI(uri); if (nsURI.scheme == "resource") { @@ -228,40 +184,18 @@ function load(loader, module) { try { Services.scriptloader.loadSubScript(module.uri, sandbox, "UTF-8"); } catch (error) { - let { message, fileName, lineNumber } = error; - const stack = error.stack || Error().stack; - const frames = parseStack(stack).filter(isntLoaderFrame); - let toString = String(error); - const file = sourceURI(fileName); - - // Note that `String(error)` where error is from subscript loader does - // not puts `:` after `"Error"` unlike regular errors thrown by JS code. - // If there is a JS stack then this error has already been handled by an - // inner module load. - if (/^Error opening input stream/.test(String(error))) { - const caller = frames.slice(0).pop(); - fileName = caller.fileName; - lineNumber = caller.lineNumber; - message = "Module `" + module.id + "` is not found at " + module.uri; - toString = message; - } else if (frames[frames.length - 1].fileName !== file) { - // Workaround for a Bug 910653. Errors thrown by subscript loader - // do not include `stack` field and above created error won't have - // fileName or lineNumber of the module being loaded, so we ensure - // it does. - frames.push({ fileName: file, lineNumber: lineNumber, name: "" }); + // loadSubScript sometime throws string errors, which includes no stack. + // At least provide the current stack by re-throwing a real Error object. + if (typeof error == "string") { + if (error.startsWith("Error creating URI") || + error.startsWith("Error opening input stream (invalid filename?)")) { + throw new Error(`Module \`${module.id}\` is not found at ${module.uri}`); + } + throw new Error(`Error while loading module \`${module.id}\` at ${module.uri}:` + + "\n" + error); } - - const prototype = typeof (error) === "object" ? error.constructor.prototype : - Error.prototype; - - throw Object.create(prototype, { - message: { value: message, writable: true, configurable: true }, - fileName: { value: fileName, writable: true, configurable: true }, - lineNumber: { value: lineNumber, writable: true, configurable: true }, - stack: { value: serializeStack(frames), writable: true, configurable: true }, - toString: { value: () => toString, writable: true, configurable: true }, - }); + // Otherwise just re-throw everything else which should have a stack + throw error; } // Only freeze the exports object if we created it ourselves. Modules diff --git a/devtools/shared/tests/unit/test_require.js b/devtools/shared/tests/unit/test_require.js index b375dee7a58d..5b1b0e48003a 100644 --- a/devtools/shared/tests/unit/test_require.js +++ b/devtools/shared/tests/unit/test_require.js @@ -17,6 +17,67 @@ function testBug1091706() { Assert.ok(indent1 === indent2); } +function testInvalidModule() { + const loader = new DevToolsLoader(); + const require = loader.require; + + try { + // This will result in an invalid URL with no scheme and mae loadSubScript + // throws "Error creating URI" error + require("foo"); + Assert.ok(false, "require should throw"); + } catch (error) { + Assert.equal(error.message, "Module `foo` is not found at foo.js"); + Assert.ok(error.stack.includes("testInvalidModule"), + "Exception's stack includes the test function"); + } + + try { + // But when using devtools prefix, the URL is going to be correct but the file + // doesn't exists, leading to "Error opening input stream (invalid filename?)" error + require("devtools/foo"); + Assert.ok(false, "require should throw"); + } catch (error) { + Assert.equal(error.message, "Module `devtools/foo` is not found at resource://devtools/foo.js"); + Assert.ok(error.stack.includes("testInvalidModule"), + "Exception's stack includes the test function"); + } +} + +function testThrowingModule() { + const loader = new DevToolsLoader(); + const require = loader.require; + + try { + // Require a test module that is throwing an Error object + require("xpcshell-test/throwing-module-1.js"); + Assert.ok(false, "require should throw"); + } catch (error) { + Assert.equal(error.message, "my-exception"); + Assert.ok(error.stack.includes("testThrowingModule"), + "Exception's stack includes the test function"); + Assert.ok(error.stack.includes("throwingMethod"), + "Exception's stack also includes the module function that throws"); + } + try { + // Require a test module that is throwing a string + require("xpcshell-test/throwing-module-2.js"); + Assert.ok(false, "require should throw"); + } catch (error) { + Assert.equal(error.message, + "Error while loading module `xpcshell-test/throwing-module-2.js` at " + + "resource://test/throwing-module-2.js:\nmy-exception"); + Assert.ok(error.stack.includes("testThrowingModule"), + "Exception's stack includes the test function"); + Assert.ok(!error.stack.includes("throwingMethod"), + "Exception's stack also includes the module function that throws"); + } +} + function run_test() { testBug1091706(); + + testInvalidModule(); + + testThrowingModule(); } diff --git a/devtools/shared/tests/unit/throwing-module-1.js b/devtools/shared/tests/unit/throwing-module-1.js new file mode 100644 index 000000000000..cc7e159a761b --- /dev/null +++ b/devtools/shared/tests/unit/throwing-module-1.js @@ -0,0 +1,7 @@ +"use strict"; + +function throwingMethod() { + throw new Error("my-exception"); +} + +throwingMethod(); diff --git a/devtools/shared/tests/unit/throwing-module-2.js b/devtools/shared/tests/unit/throwing-module-2.js new file mode 100644 index 000000000000..3e723844ecdb --- /dev/null +++ b/devtools/shared/tests/unit/throwing-module-2.js @@ -0,0 +1,8 @@ +"use strict"; + +function throwingMethod() { + // eslint-disable-next-line no-throw-literal + throw "my-exception"; +} + +throwingMethod(); diff --git a/devtools/shared/tests/unit/xpcshell.ini b/devtools/shared/tests/unit/xpcshell.ini index 26724df755a5..f3cfbda0ce44 100644 --- a/devtools/shared/tests/unit/xpcshell.ini +++ b/devtools/shared/tests/unit/xpcshell.ini @@ -5,6 +5,8 @@ firefox-appdir = browser skip-if = toolkit == 'android' support-files = exposeLoader.js + throwing-module-1.js + throwing-module-2.js [test_assert.js] [test_csslexer.js]