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

Unhandled promise rejection with a symbol crashes node #11637

Closed
apexskier opened this issue Mar 1, 2017 · 6 comments
Closed

Unhandled promise rejection with a symbol crashes node #11637

apexskier opened this issue Mar 1, 2017 · 6 comments
Labels
promises Issues and PRs related to ECMAScript promises.

Comments

@apexskier
Copy link
Contributor

  • Version: v7.2.1
  • Platform: Darwin Chelan.local 16.3.0 Darwin Kernel Version 16.3.0: Thu Nov 17 20:23:58 PST 2016; root:xnu-3789.31.2~1/RELEASE_X86_64 x86_64
  • Subsystem: internal/process/promises.js

Rejecting a promise with a Symbol will crash node. I'd expect it to handle this, or at least to crash in a different way. The traceback indicates node is unable to convert a Symbol to a string, though this is possible both in the REPL and when using .toString().

A simple demo session:

$ node
> const s = Symbol()
undefined
> s
Symbol()
> s.toString()
'Symbol()'
> const s2 = Symbol("label")
undefined
> s2
Symbol(label)
> s2.toString()
'Symbol(label)'
> Promise.reject(s2)
Promise { <rejected> Symbol(label) }
> internal/process/promises.js:60
                              `(rejection id: ${uid}): ${reason}`);
                                                         ^

TypeError: Cannot convert a Symbol value to a string
    at emitWarning (internal/process/promises.js:60:58)
    at emitPendingUnhandledRejections (internal/process/promises.js:86:11)
    at runMicrotasksCallback (internal/process/next_tick.js:61:9)
    at _combinedTickCallback (internal/process/next_tick.js:67:7)
    at process._tickDomainCallback (internal/process/next_tick.js:122:9)
1 ↩ 

Chrome (V8) handles this as I'd expect:
screen shot 2017-03-01 at 12 10 40 pm

@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Mar 1, 2017
@TimothyGu
Copy link
Member

The correct code would be to use ${String(reason)}, since on a spec level the String function has special handling for symbols in addition to the ToString abstract operation which template strings use, which throws on symbols.

What I'm more worried about is that we might have to conduct a comprehensive audit for template string usage in the code base for possibility of symbols, since it's possible that they are going to blow up as well.

@apexskier
Copy link
Contributor Author

Cool. I was thinking reason.toString(), but I'll try that. I've got a PR incoming that adds a test case and fixes the issue.

@TimothyGu
Copy link
Member

@apexskier, that wouldn't work in cases like Object.create(null) which doesn't have a toString().

@addaleax addaleax added promises Issues and PRs related to ECMAScript promises. and removed v8 engine Issues and PRs related to the V8 dependency. labels Apr 29, 2017
evanlucas pushed a commit that referenced this issue May 1, 2017
The unhandled promise rejection warning uses a template literal and
prints the reason a promise was rejected. If rejecting with a symbol,
the symbol failed to convert to a string and the process crashed. Now,
symbols are casted to strings and the process does not crash.

Fixes: #11637
PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>
evanlucas pushed a commit that referenced this issue May 2, 2017
The unhandled promise rejection warning uses a template literal and
prints the reason a promise was rejected. If rejecting with a symbol,
the symbol failed to convert to a string and the process crashed. Now,
symbols are casted to strings and the process does not crash.

Fixes: #11637
PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>
evanlucas pushed a commit that referenced this issue May 2, 2017
The unhandled promise rejection warning uses a template literal and
prints the reason a promise was rejected. If rejecting with a symbol,
the symbol failed to convert to a string and the process crashed. Now,
symbols are casted to strings and the process does not crash.

Fixes: #11637
PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>
evanlucas pushed a commit that referenced this issue May 3, 2017
The unhandled promise rejection warning uses a template literal and
prints the reason a promise was rejected. If rejecting with a symbol,
the symbol failed to convert to a string and the process crashed. Now,
symbols are casted to strings and the process does not crash.

Fixes: #11637
PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>
@muhammadfaizan
Copy link

node: v8.1.4
environment: windows 10 x64
Having same issue:

 `(rejection id: ${uid}): ${String(reason)}`);

 TypeError: Function.prototype.toString is not generic
    at InvalidValueError.toString (<anonymous>)
    at String (<anonymous>)
    at emitWarning (internal/process/promises.js:60:58)
    at emitPendingUnhandledRejections (internal/process/promises.js:86:11)
    at runMicrotasksCallback (internal/process/next_tick.js:89:9)
    at _combinedTickCallback (internal/process/next_tick.js:95:7)
    at process._tickCallback (internal/process/next_tick.js:161:9)

@ljharb
Copy link
Member

ljharb commented Sep 29, 2017

What does InvalidValueError look like?

@TimothyGu
Copy link
Member

@muhammadfaizan Try Node.js 8.5.0. It should be fixed in #13784.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants