lib: fix issue `throw null` and `throw undefined` and also `throw false` crash in repl #16574

priyank-p · 2017-10-28T16:35:00Z

The issue was that when it tries to check if err.message == 'Script execution interrupted.'
line 269 repl.js it throws error as e would be null or undefined. Now before it checks
the err.message it checks if err was null|undefined and id so returns err before checking
and exits,

# sample output repl
> throw null
Thrown: null 
> throw undefined 
Thrown: undefined

Fixes: #16545
Fixes: #16607

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
For make -j4 test
make error 1 Flaky test-timers-block-eventloop Flaky test-timers-block-eventloop #16310
commit message follows commit guidelines

Affected core subsystem(s)

repl

priyank-p · 2017-10-28T16:39:46Z

make -j4 test
resulted in everything ok but this failure which i think is a current issue #16545

# output
make[1]: Leaving directory `/home/ubuntu/workspace/node'
/usr/bin/python2.7 tools/test.py --mode=release -J \
                async-hooks \
                default \
                addons addons-napi \
                doctool known_issues
=== release test-stringbytes-external-exceed-max ===                           
Path: addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max
Command: out/Release/node /home/ubuntu/workspace/node/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js
--- CRASHED (Signal: 9) ---
=== release test-stringbytes-external-exceed-max-by-1-binary ===               
Path: addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary
Command: out/Release/node /home/ubuntu/workspace/node/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary.js
--- CRASHED (Signal: 9) ---
=== release test-stringbytes-external-exceed-max-by-1-hex ===                  
Path: addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex
Command: out/Release/node /home/ubuntu/workspace/node/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex.js
--- CRASHED (Signal: 9) ---
=== release test-timers-block-eventloop ===                                    
Path: sequential/test-timers-block-eventloop
assert.js:45
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: eventloop blocked!
    at Timeout.mustNotCall [as _onTimeout] (/home/ubuntu/workspace/node/test/common/index.js:582:12)
    at ontimeout (timers.js:478:11)
    at tryOnTimeout (timers.js:302:5)
    at Timer.listOnTimeout (timers.js:262:5)
Command: out/Release/node /home/ubuntu/workspace/node/test/sequential/test-timers-block-eventloop.js
[12:02|% 100|+ 2038|-   4]: Done                                       
make: *** [test] Error 1

mscdex · 2017-10-28T16:43:35Z

The target subsystem should be 'repl' instead of 'lib' in the commit message.

bnoordhuis · 2017-10-28T16:57:40Z

lib/repl.js

+          process.domain.emit('error', err);
+          process.domain.exit();
+          return;
+        }


You can merge this with line 266-269 if you change the condition on line 260 to err && err.message === ... and line 275 to just if (process.domain) {.

Can you also add a regression test? Thanks.

test/parallel/test-repl-domain.js is probably the right place for it, although if you can fit it in one of the other test/parallel/test-repl-* tests, that's probably perfectly alright too.

@bnoordhuis added regression test and fixed changes requested.

The issue was that when it tries to check if `err.message == 'Script execution interrupted.'` line 269 repl.js it throws error as e would be `null` or `undefined`. Now before it checks the err.message it checks if err was `null|undefined` and id so returns err before checking and exits, ` // sample output repl > throw null Thrown null > throw undefined Thrown undefined ` Fixes: #16545

joyeecheung · 2017-10-29T04:54:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11064/

gireeshpunathil · 2017-10-29T06:52:34Z

test/parallel/test-repl-throw-null-or-undefined.js

+require('../common');
+
+// This test makes sures that the repl does not 
+// crash or emit error when throwing `null|undefined`


makes sures -> makes sure

gireeshpunathil · 2017-10-29T06:53:00Z

test/parallel/test-repl-throw-null-or-undefined.js

+r.write('.exit\n');
+
+let i = 0;
+r.on('line', function replOutput(output) {


IMO new tests can accommodate arrow functions whenever possible.

gireeshpunathil · 2017-10-29T06:53:59Z

test/parallel/test-repl-throw-null-or-undefined.js

+
+let i = 0;
+r.on('line', function replOutput(output) {
+  const testStatement = i === 0 ? 'null' : 'undefined';


While this mostly works, the eventemitter manifests pure asynchronous behavior and does not guarentee any order of listener invocation with respect to the order of the associated event, at least by specification. Is it possible to see if replOutput function can handle both the possibilities?

gireeshpunathil · 2017-10-29T06:54:22Z

test/parallel/test-repl-throw-null-or-undefined.js

+let i = 0;
+r.on('line', function replOutput(output) {
+  const testStatement = i === 0 ? 'null' : 'undefined';
+  const expectedOutput = 'Thrown: ' + testStatement;


How about replacing this with template strings?

gireeshpunathil · 2017-10-29T06:54:35Z

test/parallel/test-repl-throw-null-or-undefined.js

+r.on('line', function replOutput(output) {
+  const testStatement = i === 0 ? 'null' : 'undefined';
+  const expectedOutput = 'Thrown: ' + testStatement;
+  const msg = 'repl did not throw ' + testStatement;


same as above

priyank-p · 2017-10-29T15:51:00Z

@gireeshpunathil fixed all the changes requested. Also fixed issue that not all the time the asserstion would be made as r.on('line', func) is async so when the line r.write('exit\n') is executed the process exits with code 0.

All changes are amended to previous nit commit.

gireeshpunathil

great, thank you!

lpinca · 2017-10-29T20:37:33Z

test/parallel/test-repl-throw-null-or-undefined.js

+let i = 0;
+r.on('line', (statement) => {
+  const isThrowNull = statement.includes('null');
+  const assertFail = 'repl did not throw ';


Nit: can you please remove the trailing space? There is already one in the template string below.

lpinca · 2017-10-29T20:40:57Z

test/parallel/test-repl-throw-null-or-undefined.js

+  const assertFail = 'repl did not throw ';
+  if (isThrowNull) {
+    // expecting output Thrown null
+    assert.deepStrictEqual(statement, 'Thrown null',


Nit: can you please use assert.strictEqual()?

lpinca · 2017-10-29T20:41:49Z

test/parallel/test-repl-throw-null-or-undefined.js

+    return;
+  }
+
+  assert.deepStrictEqual(statement, 'Thrown undefined',


priyank-p · 2017-10-30T11:38:14Z

@lpinca i added replaced the existing test as it does not throw error on previous node version 8.5.0. reason being that repl does not does not emit output to writer if there is and error. so no error checking but this should work.

priyank-p · 2017-10-30T12:42:55Z

There is no way to test throw false through regression test as it will not throw error and it will not be passed through repl.writer.

lance

CI looks good except for flakey raspberry pi builds

Fishrock123

CI again: https://ci.nodejs.org/job/node-test-pull-request/11097/

lance · 2017-10-31T18:43:14Z

Landed in bb59d2b

When `throw undefined` or `throw null` is executed, the REPL crashes. This change does a check for `null|undefined` before accessing an error's properties to prevent crashing. Fixes: #16545 Fixes: #16607 PR-URL: #16574 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Lance Ball <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

When `throw undefined` or `throw null` is executed, the REPL crashes. This change does a check for `null|undefined` before accessing an error's properties to prevent crashing. Fixes: nodejs/node#16545 Fixes: nodejs/node#16607 PR-URL: nodejs/node#16574 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Lance Ball <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

When `throw undefined` or `throw null` is executed, the REPL crashes. This change does a check for `null|undefined` before accessing an error's properties to prevent crashing. Fixes: nodejs#16545 Fixes: nodejs#16607 PR-URL: nodejs#16574 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Lance Ball <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

MylesBorins · 2017-11-14T16:51:57Z

This fix does not appear to fix the problem on v6.x

Would someone be willing to do a manual backport?

When `throw undefined` or `throw null` is executed, the REPL crashes. This change does a check for `null|undefined` before accessing an error's properties to prevent crashing. Fixes: #16545 Fixes: #16607 PR-URL: #16574 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Lance Ball <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

When `throw undefined` or `throw null` is executed, the REPL crashes. This change does a check for `null|undefined` before accessing an error's properties to prevent crashing. Fixes: nodejs/node#16545 Fixes: nodejs/node#16607 PR-URL: nodejs/node#16574 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Lance Ball <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Oct 28, 2017

bnoordhuis reviewed Oct 28, 2017

View reviewed changes

priyank-p added 2 commits October 28, 2017 21:32

test: add a regression test for testing throw null or throw undefined

ca62b2a

addaleax approved these changes Oct 29, 2017

View reviewed changes

gireeshpunathil reviewed Oct 29, 2017

View reviewed changes

jasnell approved these changes Oct 29, 2017

View reviewed changes

gireeshpunathil approved these changes Oct 29, 2017

View reviewed changes

lpinca approved these changes Oct 29, 2017

View reviewed changes

lpinca reviewed Oct 29, 2017

View reviewed changes

test/parallel/test-repl-throw-null-or-undefined.js Outdated

return;

}

assert.deepStrictEqual(statement, 'Thrown undefined',

Copy link

Member

lpinca Oct 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

nit: fixed code as requested, fixed tests

bd17694

priyank-p mentioned this pull request Oct 30, 2017

throw false ignored in REPL #16607

Closed

priyank-p changed the title ~~lib: fix issue throw null and throw undefined crash in repl~~ lib: fix issue throw null and throw undefined and also throw false crash in repl Oct 30, 2017

lance approved these changes Oct 30, 2017

View reviewed changes

cjihrig approved these changes Oct 30, 2017

View reviewed changes

Fishrock123 approved these changes Oct 30, 2017

View reviewed changes

thefourtheye approved these changes Oct 31, 2017

View reviewed changes

lance closed this Oct 31, 2017

cjihrig mentioned this pull request Nov 6, 2017

v9.1.0 proposal #16851

Merged

gibfahn added the land-on-v8.x label Nov 14, 2017

MylesBorins added the backport-requested-v6.x label Nov 14, 2017

gibfahn mentioned this pull request Nov 21, 2017

v8.9.2 proposal #17204

Merged

priyank-p deleted the repl-issue branch January 2, 2018 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: fix issue `throw null` and `throw undefined` and also `throw false` crash in repl #16574

lib: fix issue `throw null` and `throw undefined` and also `throw false` crash in repl #16574

priyank-p commented Oct 28, 2017 •

edited

Loading

priyank-p commented Oct 28, 2017

mscdex commented Oct 28, 2017

bnoordhuis Oct 28, 2017

bnoordhuis Oct 28, 2017

priyank-p Oct 28, 2017 •

edited

Loading

joyeecheung commented Oct 29, 2017

gireeshpunathil Oct 29, 2017

gireeshpunathil Oct 29, 2017

gireeshpunathil Oct 29, 2017

gireeshpunathil Oct 29, 2017

gireeshpunathil Oct 29, 2017

priyank-p commented Oct 29, 2017

gireeshpunathil left a comment

lpinca Oct 29, 2017

lpinca Oct 29, 2017

lpinca Oct 29, 2017

priyank-p commented Oct 30, 2017

priyank-p commented Oct 30, 2017

lance left a comment

Fishrock123 left a comment

lance commented Oct 31, 2017

MylesBorins commented Nov 14, 2017

lib: fix issue throw null and throw undefined and also throw false crash in repl #16574

lib: fix issue throw null and throw undefined and also throw false crash in repl #16574

Conversation

priyank-p commented Oct 28, 2017 • edited Loading

Checklist

Affected core subsystem(s)

priyank-p commented Oct 28, 2017

mscdex commented Oct 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyank-p Oct 28, 2017 • edited Loading

Choose a reason for hiding this comment

joyeecheung commented Oct 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyank-p commented Oct 29, 2017

gireeshpunathil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyank-p commented Oct 30, 2017

priyank-p commented Oct 30, 2017

lance left a comment

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

lance commented Oct 31, 2017

MylesBorins commented Nov 14, 2017

lib: fix issue `throw null` and `throw undefined` and also `throw false` crash in repl #16574

lib: fix issue `throw null` and `throw undefined` and also `throw false` crash in repl #16574

priyank-p commented Oct 28, 2017 •

edited

Loading

priyank-p Oct 28, 2017 •

edited

Loading