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.

Sign up for GitHub

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

Jump to bottom

Array and Object have different values in REPL and in files #7788

Closed
calebsander opened this issue Jul 19, 2016 · 15 comments
Closed

Array and Object have different values in REPL and in files #7788

calebsander opened this issue Jul 19, 2016 · 15 comments
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem. repro-exists Issues with reproductions.

Comments

@calebsander
Copy link
Contributor

calebsander commented Jul 19, 2016 •
edited
Loading

  • Version: v6.3.0
  • Platform: 4.4.0-31-generic #50-Ubuntu SMP Wed Jul 13 00:07:12 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

I have one file, say is.js, that contains the following:

exports.isObject = (obj) => obj.constructor === Object
exports.isNumber = (obj) => obj.constructor === Number

In the console, require('./is.js').isObject({}) returns false. However, ({}).constructor === Object returns true. Strangely, isNumber(2) works as expected. When required from another file, everything works as expected.

Affected classes:

  • Object
  • Array
  • Date
  • Error
  • RegExp

Unaffected classes:

  • Number
  • String
  • Boolean
  • Map
  • Set
  • Symbol
  • Typed arrays
@addaleax addaleax added confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem. labels Jul 19, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Jul 19, 2016

#5703 may be the cause of this.

@addaleax
Copy link
Member

Yep, confirmed by bisecting.

@cjihrig cjihrig mentioned this issue Jul 19, 2016
Merged
3 tasks
@cjihrig
Copy link
Contributor

cjihrig commented Jul 19, 2016

I opened #7793 as a known issue test. Once this is resolved, the test can be moved to the normal test suite.

@addaleax
Copy link
Member

Tbh I think reverting #5703 might be the best idea for now, this doesn’t seem like something that’s easy to resolve… @lance?

@cjihrig
Copy link
Contributor

cjihrig commented Jul 19, 2016

Revert is in #7795. I can combine the test from #7793 into that PR.

cjihrig added a commit to cjihrig/node that referenced this issue Jul 20, 2016
@cjihrig
test: add known issue test for nodejs#7788
8c0f776 
15157c3 changed the CLI REPL
to default to useGlobal: false by default. This caused the
regression seen in nodejs#7788.
This commit adds a known issue test while a proper resolution
is determined.

Refs: nodejs#5703
Refs: nodejs#7788
PR-URL: nodejs#7793
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrig cjihrig added the repro-exists Issues with reproductions. label Jul 20, 2016
@lance
Copy link
Member

lance commented Jul 20, 2016

@addaleax @cjihrig reverting makes sense, I suppose. It's not obvious to me why this is happening at the moment.

Since #5703 was also a fix for #6802 should this be reopened?

@cjihrig
Copy link
Contributor

cjihrig commented Jul 20, 2016

Yes, it should be reopened if the revert PR lands.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 20, 2016

@lance as a side note, I believe the problem is that require() runs code in the global context, and when useGlobal is false, the code run in the CLI executes in a different context.

@lance
Copy link
Member

lance commented Jul 20, 2016

@cjihrig @addaleax - I think the problem is instead with some of the code here: https://github.com/nodejs/node/blob/master/lib/repl.js#L602-L618. In fact, @cjihrig, your known issue test will continue to fail even if we revert #5703 won't it? Because reverting that PR only causes the default to change and that's all. The known issue test is explicitly setting useGlobal to false so the code path will be identical after the revert. The behavior will be mitigated in the CLI REPL by a revert, but I don't that fixes the underlying cause.

I think that perhaps this issue is related to https://github.com/nodejs/node/pull/7369/files#r68335215.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 20, 2016

The known issue test shows the relationship between useGlobal: false and require(). The revert PR in #7795 has a slightly modified test that is tied to the CLI REPL. That test currently fails, and passes with the revert. The known issue test will live on independent of the revert.

@lance
Copy link
Member

lance commented Jul 20, 2016

/me should have read the revert PR first :)

Still, I think it would be better to figure out what the real problem is and fix that rather than revert which just masks the problem. The issue doesn't go a way with a programmatic REPL started with { useGlobal: false }.

@addaleax
Copy link
Member

#7788 (comment) – Object refers to different things in different vm contexts, and e.g. {} will have a different prototype depending on where it’s coming from. I forgot why, but apparently that’s the intended behaviour of V8.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 20, 2016

I agree that we should try to get to the bottom of it, but if it isn't a straightforward fix, we should revert until we have a fix.

@addaleax
Copy link
Member

See also e.g. #7351

@lance
Copy link
Member

lance commented Jul 20, 2016

And maybe this is related somehow? #855

I don't really understand V8 Global proxy yet, so still trying to wrap my head around this.

@MylesBorins MylesBorins mentioned this issue Jul 20, 2016
Closed
4 tasks
evanlucas pushed a commit that referenced this issue Jul 21, 2016
@cjihrig @evanlucas
test: add known issue test for #7788
2e05e65 
15157c3 changed the CLI REPL
to default to useGlobal: false by default. This caused the
regression seen in #7788.
This commit adds a known issue test while a proper resolution
is determined.

Refs: #5703
Refs: #7788
PR-URL: #7793
Reviewed-By: Rich Trott <rtrott@gmail.com>
@evanlucas evanlucas mentioned this issue Jul 21, 2016
Merged
@calebsander calebsander changed the title Array and Object have different values in the console and in files Array and Object have different values in REPL and in files Jul 23, 2016
@addaleax addaleax mentioned this issue Jul 27, 2016
Closed
evanlucas pushed a commit that referenced this issue Aug 2, 2016
@cjihrig @evanlucas
repl: default useGlobal to true
6e6cf36 
This is a partial revert of
15157c3. This change lead to a
regression that broke require() in the CLI REPL, as imported
files were evaluated in a different context.

Refs: #5703
Fixes: #7788
PR-URL: #7795
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
@addaleax addaleax mentioned this issue Aug 9, 2016
Closed
@addaleax addaleax mentioned this issue Aug 13, 2016
Closed
@neumino neumino mentioned this issue Aug 13, 2016
Closed
@addaleax addaleax mentioned this issue Aug 18, 2016
Closed
@addaleax addaleax mentioned this issue Apr 3, 2017
Closed
@bnoordhuis bnoordhuis mentioned this issue Oct 6, 2017
Closed
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Oct 6, 2017
@bnoordhuis
vm: run runInThisContext() in last entered context
4ce3242 
Change `vm.runInThisContext()` to use the last entered context, not the
top-level context that `vm.runInThisContext()` belongs to.

Note the call chain:

    1. `node::ContextifyScript::RunInThisContext()`
    2. `node::ContextifyScript::EvalMachine()`
    3. `v8::Script::BindToCurrentContext()`

The current context in `RunInThisContext()` is that of the top-level
context which was then bound to the script to execute.

It works when `vm.runInThisContext()` is called from the top-level
context (the common case and hence why this bug went unnoticed for
so long) but not when called from an inferior context.

As a pleasant side effect, this commit fixes a bug in the REPL where
writes to `process.stdout` and `process.stderr` still went to the real
stdout and stderr instead of being captured redirected.

Fixes previously failing test `known_issues/test-repl-require-context`
which has been moved to `test/parallel` and renamed to avoid a conflict
with an existing test of the same name.

Note that this commit changes the behavior of the following snippet:

    const s = '(function() { return vm.runInThisContext(this) })()';
    const f = vm.runInNewContext(s, vm);
    f();

Before this commit, it returned the |this| of the new context, now
it returns the |this| of the enclosing context.  Two steps forward,
one step back.

Fixes: nodejs#7788
Refs: nodejs#14757
@Trott Trott mentioned this issue Nov 25, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem. repro-exists Issues with reproductions.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@lance @addaleax @cjihrig @calebsander