Broken message test

[targos]

CI run: https://ci.nodejs.org/job/node-test-commit-node-v8/32/

Error:

length differs.
expect=1
actual=0
patterns:
pattern = ^Hello\,\ World\!$
outlines:
not ok 1432 message/console_low_stack_space
  ---
  duration_ms: 0.140
  severity: fail
  stack: |-

Commit: v8/v8@69aa868

[targos]

This is still happening and I don't know if we have to change this test or if it's a V8 issue.

Repro:

function a(n) {
  try {
    a(n + 1);
  } catch (e) {
    console.log('overflow ' + n);
  }
}

a(0);

With Node 7, it prints "overflow 15288".
With Node 8, it prints "overflow 5088".
With canary, it prints nothing.

I put some process._rawDebug() in the code and it shows that the log fails here (Maximum call stack size exceeded): https://github.com/nodejs/node/blob/ca8a29c3606421d71ce3120cd34febacd7874293/lib/console.js#L92

/cc @nodejs/v8 @addaleax

[targos]

Reverting v8/v8@69aa868 fixes this issue.

[addaleax]

Ugh … this is tricky. I think the most semantically correct way to fix that would be to re-throw certain errors like a stack overflow in the catch block two lines below the line you linked

[bmeurer]

cc @isheludko

[addaleax]

@bmeurer To be clear, I don’t think V8 is doing anything it shouldn’t, and you don’t need to investigate.

This is my fault, I introduced that test in dab0987 and later accidentally broke it by changing error handling in f18e08d, it’s pure luck that the test hasn’t been failing so far.

[bnoordhuis]

I wouldn't expect console.log() or any other function call to work in a stack overflow situation. I'd just change or remove the test.

[addaleax]

@bnoordhuis I probably should have commented that in the test, but the test isn’t checking that console.log() works in a stack overflow situation; it’s checking that, once a stack overflow occurs while trying to put theconsole object together, console.log() works after the stack has unwound sufficiently.

[bnoordhuis]

I was going by @targos's test case, I see that the message test does something slightly different. I don't know if exception landing pads are expected to work like that when the stack is full.

[targos]

@addaleax do you have an idea on how to fix this without removing the test?

[addaleax]

@targos can you try something like

diff --git a/lib/console.js b/lib/console.js
index 7ec9c846329c..49e6633902b0 100644
--- a/lib/console.js
+++ b/lib/console.js
@@ -90,6 +90,9 @@ function write(ignoreErrors, stream, string, errorhandler) {
 
     stream.write(string, errorhandler);
   } catch (e) {
+    if (String(e) === 'RangeError: Maximum call stack size exceeded') {
+      throw e;
+    }
     // Sorry, there’s no proper way to pass along the error here.
   } finally {
     stream.removeListener('error', noop);

I didn’t check myself that this works together with canary, I might have the time a bit later this week to do it myself.

(I know this seems odd – maybe f18e08d should have been a bit less strict about catching all errors…)

[targos]

@addaleax I tried that and it doesn't work with canary.
If I add process._rawDebug('test') just before the throw, I can see "test" being logged 4 times and then nothing.

[addaleax]

Ugh, okay. I’ll take a look as soon as I get the chance (which is likely not before Friday, sorry).

[targos]

No problem. We have plenty of time before V8 6.1 goes stable.

[targos]

Ping @addaleax. Would you still like to find something better than removing the test?

[targos]

This is going to be fixed in nodejs/node#14580. Closing.