镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

repl: fix auto-complete crash in REPL mode when set env NODE_MODULE_CONTEXTS - #1160

Closed
abbshr wants to merge 3 commits into
nodejs:v1.xfrom
abbshr:v1.x
Closed

abbshr wants to merge 3 commits into
nodejs:v1.xfrom
abbshr:v1.x

Conversation

@abbshr

@abbshr abbshr commented Mar 16, 2015

Copy link
Copy Markdown
Contributor

see commit

abbshr added 3 commits March 16, 2015 16:01
…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);
}
@bnoordhuis

Copy link
Copy Markdown
Member

@abbshr Can you add a regression test? It's not clear to me what this fixes.

@bnoordhuis

Copy link
Copy Markdown
Member

Sorry, those were unrelated statements. :-)

  1. I'd like to see a regression test get added to e.g. test/parallel/test-repl.js.

  2. I'd like to understand why changing a function to a function variable fixes a TypeError.

@bnoordhuis

Copy link
Copy Markdown
Member

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.

@cjihrig

cjihrig commented Mar 16, 2015

Copy link
Copy Markdown
Contributor

@bnoordhuis Does #1162 completely supersede this?

@bnoordhuis

Copy link
Copy Markdown
Member

@cjihrig Yes. There's nothing left to fix after #1162 lands. :-)

@cjihrig

cjihrig commented Mar 16, 2015

Copy link
Copy Markdown
Contributor

OK, just double checking. I'm going to close this in favor of #1162. Please reopen if you deem it necessary.

@cjihrig cjihrig closed this Mar 16, 2015
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Mar 16, 2015
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants