From fb25c910dfb359adb2225117505fd52a4f5b2c93 Mon Sep 17 00:00:00 2001
From: Matheus Marchini <mmarchini@netflix.com>
Date: Mon, 16 Sep 2019 15:54:20 -0700
Subject: [PATCH] src: fix JSError inspection with stringified stack

When the `stack` property of an Error object is accessed in V8, a
stringified version of the stack is generated, and then the "raw stack"
(with FP and arguments) is removed from memory. Our current inspection
code couldn't handle this because it was not checking if the raw stack
was a valid object. This commit makes that code more robust and thus
fixes inspection of error objects with stringified stacks.

PR-URL: https://github.com/nodejs/llnode/pull/291
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 src/llv8.cc                       |  7 ++++++
 src/printer.cc                    | 36 +++++++++++++++++--------------
 test/fixtures/inspect-scenario.js |  3 +++
 test/plugin/inspect-test.js       | 22 +++++++++++++++++++
 4 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/src/llv8.cc b/src/llv8.cc
index 9ae66f6c..4c453cfd 100644
--- a/src/llv8.cc
+++ b/src/llv8.cc
@@ -1271,6 +1271,13 @@ StackTrace::Iterator StackTrace::end() {
 
 StackTrace::StackTrace(JSArray frame_array, Error& err)
     : frame_array_(frame_array) {
+  if (!frame_array.Check()) {
+    PRINT_DEBUG("JS Array is not a valid object");
+    len_ = -1;
+    multiplier_ = -1;
+    return;
+  }
+
   v8::Value maybe_stack_len = frame_array.GetArrayElement(0, err);
 
   if (err.Fail()) {
diff --git a/src/printer.cc b/src/printer.cc
index bdc970a9..507e5c3b 100644
--- a/src/printer.cc
+++ b/src/printer.cc
@@ -526,27 +526,31 @@ std::string Printer::Stringify(v8::JSError js_error, Error& err) {
   if (options_.detailed) {
     output << StringifyJSObjectFields(js_error, err);
 
-    v8::StackTrace stack_trace = js_error.GetStackTrace(err);
+    if (js_error.HasStackTrace(err)) {
+      v8::StackTrace stack_trace = js_error.GetStackTrace(err);
 
-    std::stringstream error_stack;
-    error_stack << std::endl
-                << rang::fg::red << "  error stack" << rang::fg::reset << " {"
-                << std::endl;
+      std::stringstream error_stack;
+      error_stack << std::endl
+                  << rang::fg::red << "  error stack" << rang::fg::reset << " {"
+                  << std::endl;
 
-    Printer printer(llv8_);
-    for (v8::StackFrame frame : stack_trace) {
-      v8::JSFunction js_function = frame.GetFunction(err);
-      if (err.Fail()) {
-        error_stack << rang::fg::gray << "    <unknown>" << std::endl;
-        continue;
+
+      Printer printer(llv8_);
+      for (v8::StackFrame frame : stack_trace) {
+        v8::JSFunction js_function = frame.GetFunction(err);
+        if (err.Fail()) {
+          error_stack << rang::fg::gray << "    <unknown>" << std::endl;
+          continue;
+        }
+
+        error_stack << "    "
+                    << printer.Stringify<v8::HeapObject>(js_function, err)
+                    << std::endl;
       }
 
-      error_stack << "    "
-                  << printer.Stringify<v8::HeapObject>(js_function, err)
-                  << std::endl;
+      error_stack << "  }";
+      output << error_stack.str();
     }
-    error_stack << "  }";
-    output << error_stack.str();
   }
 
   output << rang::fg::yellow << ">" << rang::fg::reset;
diff --git a/test/fixtures/inspect-scenario.js b/test/fixtures/inspect-scenario.js
index fb6883e3..1864a8b7 100644
--- a/test/fixtures/inspect-scenario.js
+++ b/test/fixtures/inspect-scenario.js
@@ -64,6 +64,9 @@ function closure() {
   c.hashmap['error'].code = 'ERR_TEST';
   c.hashmap['error'].errno = 1;
 
+  c.hashmap['stringifiedError'] = new Error('test');
+  c.hashmap['stringifiedErrorStack'] = c.hashmap['stringifiedError'].stack;
+
   c.hashmap[0] = null;
   c.hashmap[4] = undefined;
   c.hashmap[23] = /regexp/;
diff --git a/test/plugin/inspect-test.js b/test/plugin/inspect-test.js
index ebeb545f..35792f53 100644
--- a/test/plugin/inspect-test.js
+++ b/test/plugin/inspect-test.js
@@ -161,6 +161,28 @@ const hashMapTests = {
       });
     }
   },
+  // .stringifiedError=0x0000392d5d661119:<Object: Error>
+  'error': {
+    re: /.stringifiedError=(0x[0-9a-f]+):<Object: Error>/,
+    desc: '.stringifiedError Error property with stringified stack',
+    validator(t, sess, addresses, name, cb) {
+      const address = addresses[name];
+      sess.send(`v8 inspect ${address}`);
+
+      sess.linesUntil(/}>/, (err, lines) => {
+        if (err) return cb(err);
+        lines = lines.join('\n');
+
+        let strStackMatch = lines.match(/stack=(0x[0-9a-f]+):<String: /i);
+        t.ok(strStackMatch, 'hashmap.stringifiedError should have stringified stack');
+
+        let stackMatch  = lines.match(/error stack {/i);
+        t.notOk(stackMatch, 'Error object with stringified stack should not have an error stack');
+
+        cb(null);
+      });
+    }
+  },
   // .array=0x000003df9cbe7919:<Array: length=6>,
   'array': {
     re: /.array=(0x[0-9a-f]+):<Array: length=6>/,