Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repl: Prevent REPL crash when tab-completed with Proxy objects #2120

Closed

Conversation

thefourtheye
Copy link
Contributor

If the proxy objects don't have a valid hasOwnPropertyNames trap,
REPL crashes with a TypeError, as per the bug report
#2119

> var proxy = Proxy.create({ fix: function() { return {}; } });
undefined
> proxy.<tab>
TypeError: Proxy handler #<Object> has no 'getOwnPropertyNames' trap
    at Function.getOwnPropertyNames (native)
    at repl.js:644:40
    at REPLServer.defaultEval (repl.js:169:5)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.complete (repl.js:639:14)
    at REPLServer.complete [as completer] (repl.js:207:10)
    at REPLServer.Interface._tabComplete (readline.js:377:8)
    at REPLServer.Interface._ttyWrite (readline.js:845:14)
    at ReadStream.onkeypress (readline.js:105:10)

This patch traps the error thrown and issues Error message.

@thefourtheye
Copy link
Contributor Author

I don't know how to write a test for this, as we need to

  1. enable --harmony-proxies
  2. do tab complete

I am not sure how to do both of them in the same test.

@evanlucas
Copy link
Contributor

@rlidwka
Copy link
Contributor

rlidwka commented Jul 7, 2015

Can something else except proxy objects cause TypeError? That error check seems a bit too broad.

Also, I'm not sure adding an error message is useful. Maybe just silencing that particular error would be better.

@thefourtheye
Copy link
Contributor Author

@evanlucas I tried that now and it doesn't work. This is the patch which I tried,

diff --git a/test/parallel/test-repl-tab-complete.js b/test/parallel/test-repl-tab-complete.js
index 1bf0c70..c560a3f 100644
--- a/test/parallel/test-repl-tab-complete.js
+++ b/test/parallel/test-repl-tab-complete.js
@@ -1,4 +1,6 @@
 'use strict';
+// Flags: --harmony-proxies
+
 var common = require('../common');
 var assert = require('assert');
 var util = require('util');
@@ -242,3 +244,14 @@ testMe.complete('cus', function(error, data) {
   completionCount++;
   assert.deepEqual(data, [['custom'], 'cus']);
 });
+
+// Make sure tab completion doesn't crash REPL with proxy objects
+putIn.run(['.clear']);
+
+putIn.run([
+  'var proxy = Proxy.create({});'
+]);
+
+testMe.complete('proxy.', function(error, data) {
+  console.error(arguments);
+});

It still throws a ReferenceError for Proxy. Is there anything else I need to include?

@rlidwka We can tighten the error checking, but silencing it will not give a hint to the user. Will that be okay?

@evanlucas
Copy link
Contributor

Did you still get the ReferenceError running it with python tools/test.py?

@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Jul 7, 2015
@thefourtheye
Copy link
Contributor Author

@evanlucas Awesome, it works :-) Thanks for the help. I removed the error check as suggested by @rlidwka now.

'var proxy = Proxy.create({});'
]);

testMe.complete('proxy.', function(error, data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use either the referenceErrors or completionCount variable here (see the rest of the test). This test is known to fail silently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjihrig Oh, why not common.mustCall then? That should work here, right? I think that would make the tests better, without explicitly counting.

Copy link
Contributor

Choose a reason for hiding this comment

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

That should work for the tests using completionCount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjihrig Can you PTAL at #2122? I PRed that idea.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 7, 2015

This can no longer be merged.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 7, 2015

Not a huge fan of just dropping errors, but it's also just REPL tab completion. LGTM

@thefourtheye thefourtheye force-pushed the fix-repl-proxy-completion branch 3 times, most recently from b77a4c9 to c598b9a Compare July 7, 2015 16:43
If the proxy objects don't have a valid `hasOwnPropertyNames` trap,
REPL crashes with a `TypeError`, as per the bug report
nodejs#2119

    > var proxy = Proxy.create({ fix: function() { return {}; } });
    undefined
    > proxy.<tab>
    TypeError: Proxy handler #<Object> has no 'getOwnPropertyNames' trap
        at Function.getOwnPropertyNames (native)
        at repl.js:644:40
        at REPLServer.defaultEval (repl.js:169:5)
        at bound (domain.js:254:14)
        at REPLServer.runBound [as eval] (domain.js:267:12)
        at REPLServer.complete (repl.js:639:14)
        at REPLServer.complete [as completer] (repl.js:207:10)
        at REPLServer.Interface._tabComplete (readline.js:377:8)
        at REPLServer.Interface._ttyWrite (readline.js:845:14)
        at ReadStream.onkeypress (readline.js:105:10)

This patch traps the error thrown and suppresses it.
@thefourtheye
Copy link
Contributor Author

@cjihrig Thanks for reviewing and notifying about the merge conflict :-)

I rebased and squashed the commits.

@rlidwka @evanlucas PTAL :-)

@thefourtheye thefourtheye changed the title repl: throw error when tab-completed with Proxy objects repl: Prevent REPL crash when tab-completed with Proxy objects Jul 7, 2015
@evanlucas
Copy link
Contributor

evanlucas pushed a commit that referenced this pull request Jul 10, 2015
If the proxy objects don't have a valid `hasOwnPropertyNames` trap,
REPL crashes with a `TypeError`, as per the bug report
#2119

    > var proxy = Proxy.create({ fix: function() { return {}; } });
    undefined
    > proxy.<tab>
    TypeError: Proxy handler #<Object> has no 'getOwnPropertyNames' trap
        at Function.getOwnPropertyNames (native)
        at repl.js:644:40
        at REPLServer.defaultEval (repl.js:169:5)
        at bound (domain.js:254:14)
        at REPLServer.runBound [as eval] (domain.js:267:12)
        at REPLServer.complete (repl.js:639:14)
        at REPLServer.complete [as completer] (repl.js:207:10)
        at REPLServer.Interface._tabComplete (readline.js:377:8)
        at REPLServer.Interface._ttyWrite (readline.js:845:14)
        at ReadStream.onkeypress (readline.js:105:10)

This patch traps the error thrown and suppresses it.

PR-URL: #2120
Fixes: #2119
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@evanlucas
Copy link
Contributor

Landed in 59f6b5d with a slightly modified commit message to conform to the collaborator guidelines. Thanks @thefourtheye!