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

new Map(wrong_iterable) swallow TypeError in file scripts #16856

Closed
vsemozhetbyt opened this issue Nov 7, 2017 · 15 comments
Closed

new Map(wrong_iterable) swallow TypeError in file scripts #16856

vsemozhetbyt opened this issue Nov 7, 2017 · 15 comments
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.

Comments

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Nov 7, 2017

  • Version: 8–10
  • Platform: Windows 7 x64
  • Subsystem: ?
'use strict';

new Map('a');
// Node.js 4.8.5
new Map('a');
^
TypeError: Iterator value a is not an entry object

// Node.js 6.11.5
new Map('a');
^
TypeError: Iterator value a is not an entry object

// Node.js 8.9.0
<no output>

// Node.js 9.0.0
<no output>

// Node.js 10.0.0 nightly 2017.11.06
<no output>

This only happens with a file script. In the REPL, new Map('a') throws TypeError in all these versions.

@vsemozhetbyt vsemozhetbyt added the console Issues and PRs related to the console subsystem. label Nov 7, 2017
@vsemozhetbyt vsemozhetbyt changed the title console.log(new Map(wrong_iterable)) swallow TypeError new Map(wrong_iterable) swallow TypeError in file scripts Nov 7, 2017
@vsemozhetbyt vsemozhetbyt removed the console Issues and PRs related to the console subsystem. label Nov 7, 2017
@vsemozhetbyt
Copy link
Contributor Author

@nodejs/v8, Is it an upstream bug?

@joyeecheung
Copy link
Member

joyeecheung commented Nov 7, 2017

▶ ./d8
V8 version 6.2.414.32
d8> new Map('a');
TypeError: Iterator value a is not an entry object
TypeError: Iterator value a is not an entry object
    at new Map (<anonymous>)
    at (d8):1:1
▶ ./d8 file.js
TypeError: Iterator value a is not an entry object
TypeError: Iterator value a is not an entry object
    at new Map (<anonymous>)
    at file.js:3:1

@vsemozhetbyt Doesn't look like it

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Nov 7, 2017

@joyeecheung Maybe we should also check with the module wrapper to be sure (compare #15386):

> (function (exports, require, module, __filename, __dirname) { new Map('a'); })();
TypeError: Iterator value a is not an entry object
    at new Map (<anonymous>)
    at repl:1:63

@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Nov 7, 2017
@joyeecheung
Copy link
Member

@vsemozhetbyt d8 still throws with the wrapper

▶ ./d8 file.js
TypeError: Iterator value a is not an entry object
TypeError: Iterator value a is not an entry object
    at new Map (<anonymous>)
    at file.js:1:63
    at file.js:1:79

@targos
Copy link
Member

targos commented Nov 7, 2017

I can reproduce with the latest canary (v10.0.0-v8-canary2017110614c987bd6b)

@TimothyGu
Copy link
Member

Looks like some kind of crash.

'use strict';

console.log('a');
try {
  console.log(new Map('a'));
} catch (err) {
  console.log(err);
}
console.log('b');

prints a, the error, and b as expected, yet

'use strict';

console.log('a');
console.log(new Map('a'));
console.log('b');

only prints a.

@bmeurer
Copy link
Member

bmeurer commented Nov 7, 2017

cc @gsathya

@vsemozhetbyt
Copy link
Contributor Author

@bmeurer Should not this be an early error at the parsing time?

@hashseed
Copy link
Member

hashseed commented Nov 7, 2017

It's correct syntax. Why would it be an early error?

@vsemozhetbyt
Copy link
Contributor Author

@hashseed We can know that a string is not an appropriate iterable in all such cases?

@hashseed
Copy link
Member

hashseed commented Nov 7, 2017

At parse time you can't know what Map refers to. I could simply overwrite the global Map object.

@hashseed
Copy link
Member

hashseed commented Nov 7, 2017

Interestingly, with the following code

'use strict';
console.log('a');
new Map('a');
console.log('b');

v8::internal::Isolate::Throw never triggers. Instead, we call Rethrow.

@hashseed
Copy link
Member

hashseed commented Nov 7, 2017

This is a bug in V8. The Map constructor does not throw correctly.
I filed an upstream issue.

cc @gsathya @caitp.

@hashseed
Copy link
Member

hashseed commented Nov 8, 2017

The upcoming fix is here: https://chromium-review.googlesource.com/c/v8/v8/+/758372

@hashseed
Copy link
Member

hashseed commented Nov 8, 2017

Fix has landed upstream.

@fhinkel fhinkel added the confirmed-bug Issues with confirmed bugs. label Nov 9, 2017
fhinkel added a commit to fhinkel/node that referenced this issue Nov 12, 2017
Original commit message:

  [map] Fix map constructor to correctly throw.

  We need to throw before rethrowing, otherwise the exception does
  not trigger a debugger event and is not reported if uncaught.

  [email protected], [email protected]

  Bug: v8:7047
  Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
  Reviewed-on: https://chromium-review.googlesource.com/758372
  Reviewed-by: Jakob Gruber <[email protected]>
  Reviewed-by: Sathya Gunasekaran <[email protected]>
  Commit-Queue: Yang Guo <[email protected]>
  Cr-Commit-Position: refs/heads/master@{nodejs#49237}

Fixes: nodejs#16856
evanlucas pushed a commit that referenced this issue Nov 14, 2017
Original commit message:

  [map] Fix map constructor to correctly throw.

  We need to throw before rethrowing, otherwise the exception does
  not trigger a debugger event and is not reported if uncaught.

  [email protected], [email protected]

  Bug: v8:7047
  Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
  Reviewed-on: https://chromium-review.googlesource.com/758372
  Reviewed-by: Jakob Gruber <[email protected]>
  Reviewed-by: Sathya Gunasekaran <[email protected]>
  Commit-Queue: Yang Guo <[email protected]>
  Cr-Commit-Position: refs/heads/master@{#49237}

PR-URL: #16897
Fixes: #16856
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants