-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: fix auto-complete crash in REPL mode when set env NODE_MODULE_CONTEXTS
#1160
Conversation
…ONTEXTS=1 Set env `NODE_MODULE_CONTEXTS` in REPL mode, when type the `Tab`, the process exit with an `TypeError`: `env NODE_MODULE_CONTEXTS=1 iojs` > repl.js:670 if (!hasOwnProperty(uniq, c)) { ^ TypeError: Cannot convert undefined or null to object at hasOwnProperty (native) at completionGroupsLoaded (repl.js:670:16) at REPLServer.complete (repl.js:571:11) at REPLServer.complete [as completer] (repl.js:169:10) at REPLServer.Interface._tabComplete (readline.js:362:8) at REPLServer.Interface._ttyWrite (readline.js:830:14) at ReadStream.onkeypress (readline.js:90:10) at emitTwo (events.js:87:13) at ReadStream.emit (events.js:169:7) at readline.js:1156:14 It's a bug in `hasOwnProperty` in repl.js: origin: function hasOwnProperty(obj, prop) { return Object.prototype.hasOwnProperty.call(obj, prop); } fix: var hasOwnProperty = function (obj, prop) { return Object.prototype.hasOwnProperty.call(obj, prop); }
The error you got before To reproduce your bug in source, just call |
@abbshr Can you add a regression test? It's not clear to me what this fixes. |
@bnoordhuis i guess there are some infos at https://github.com/abbshr/io.js/commit/9382e0985860bb6bf2e2e8336d5a34d52aa4fff7 |
Sorry, those were unrelated statements. :-)
|
Okay, I think I get it now. With NODE_MODULE_CONTEXTS=1, the hasOwnProperty method from the sandbox object ends up on the global object. The proper fix is this: diff --git a/lib/module.js b/lib/module.js
index 0d9f093..299c1c1 100644
--- a/lib/module.js
+++ b/lib/module.js
@@ -395,7 +395,7 @@ Module.prototype._compile = function(content, filename) {
if (self.id !== '.') {
debug('load submodule');
// not root module
- var sandbox = {};
+ var sandbox = Object.create(null);
for (var k in global) {
sandbox[k] = global[k];
} But the fact that this has been broken since forever is perhaps a strong hint that no one uses NODE_MODULE_CONTEXTS and it should just be removed. |
@bnoordhuis Does #1162 completely supersede this? |
OK, just double checking. I'm going to close this in favor of #1162. Please reopen if you deem it necessary. |
This feature has no tests and has been broken for ages, see for example nodejs#1160. Don't bother fixing it, it's pretty much broken by design and there can't be too many users because it's almost undocumented. A quick Google search suggests that it causes more grief than joy to the few that do use it. Remove it. PR-URL: nodejs#1162 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
see commit