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

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

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

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();
 }

devtools/shared/tests/unit/throwing-module-1.js

@@ -0,0 +1,7 @@
+"use strict";
+
+function throwingMethod() {
+  throw new Error("my-exception");
+}
+
+throwingMethod();

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();

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]