inspector: log exceptions in message handlers

Fixes: nodejs#14965 PR-URL: nodejs#14980 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
addaleax · Sep 13, 2017 · d19377c · d19377c
1 parent c6f3374
commit d19377c
Show file tree

Hide file tree

Showing 2 changed files with 57 additions and 19 deletions.
diff --git a/lib/inspector.js b/lib/inspector.js
@@ -29,14 +29,18 @@ class Session extends EventEmitter {
 
   [onMessageSymbol](message) {
     const parsed = JSON.parse(message);
-    if (parsed.id) {
-      const callback = this[messageCallbacksSymbol].get(parsed.id);
-      this[messageCallbacksSymbol].delete(parsed.id);
-      if (callback)
-        callback(parsed.error || null, parsed.result || null);
-    } else {
-      this.emit(parsed.method, parsed);
-      this.emit('inspectorNotification', parsed);
+    try {
+      if (parsed.id) {
+        const callback = this[messageCallbacksSymbol].get(parsed.id);
+        this[messageCallbacksSymbol].delete(parsed.id);
+        if (callback)
+          callback(parsed.error || null, parsed.result || null);
+      } else {
+        this.emit(parsed.method, parsed);
+        this.emit('inspectorNotification', parsed);
+      }
+    } catch (error) {
+      process.emitWarning(error);
     }
   }
 

diff --git a/test/inspector/test-bindings.js b/test/inspector/test-bindings.js
@@ -33,14 +33,25 @@ function debuggerPausedCallback(session, notification) {
   checkScope(session, scopeId);
 }
 
-function testNoCrashWithExceptionInCallback() {
+function waitForWarningSkipAsyncStackTraces(resolve) {
+  process.once('warning', function(warning) {
+    if (warning.code === 'INSPECTOR_ASYNC_STACK_TRACES_NOT_AVAILABLE') {
+      waitForWarningSkipAsyncStackTraces(resolve);
+    } else {
+      resolve(warning);
+    }
+  });
+}
+
+async function testNoCrashWithExceptionInCallback() {
   // There is a deliberate exception in the callback
   const session = new inspector.Session();
   session.connect();
   const error = new Error('We expect this');
-  assert.throws(() => {
-    session.post('Console.enable', () => { throw error; });
-  }, (e) => e === error);
+  console.log('Expecting warning to be emitted');
+  const promise = new Promise(waitForWarningSkipAsyncStackTraces);
+  session.post('Console.enable', () => { throw error; });
+  assert.strictEqual(await promise, error);
   session.disconnect();
 }
 
@@ -97,10 +108,33 @@ function testSampleDebugSession() {
   assert.throws(() => session.post('Debugger.enable'), (e) => !!e);
 }
 
-testNoCrashWithExceptionInCallback();
-testSampleDebugSession();
-let breakpointHit = false;
-scopeCallback = () => (breakpointHit = true);
-debuggedFunction();
-assert.strictEqual(breakpointHit, false);
-testSampleDebugSession();
+async function testNoCrashConsoleLogBeforeThrow() {
+  const session = new inspector.Session();
+  session.connect();
+  let attempt = 1;
+  process.on('warning', common.mustCall(3));
+  session.on('inspectorNotification', () => {
+    if (attempt++ > 3)
+      return;
+    console.log('console.log in handler');
+    throw new Error('Exception in handler');
+  });
+  session.post('Runtime.enable');
+  console.log('Did not crash');
+  session.disconnect();
+}
+
+common.crashOnUnhandledRejection();
+
+async function doTests() {
+  await testNoCrashWithExceptionInCallback();
+  testSampleDebugSession();
+  let breakpointHit = false;
+  scopeCallback = () => (breakpointHit = true);
+  debuggedFunction();
+  assert.strictEqual(breakpointHit, false);
+  testSampleDebugSession();
+  await testNoCrashConsoleLogBeforeThrow();
+}
+
+doTests();