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

Revert "deps: backport 03ef3cd from V8 upstream" #3237

Closed
wants to merge 1 commit into from

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Oct 7, 2015

This reverts commit 6fff47f as it is causing
issues in upstream: https://codereview.chromium.org/1390923004/

Original PR: #3165.

R=@bnoordhuis, @targos

@targos
Copy link
Member

targos commented Oct 7, 2015

LGTM.

The Chromium bug report is not accessible?

@targos targos added the v8 engine Issues and PRs related to the V8 dependency. label Oct 7, 2015
@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 7, 2015

Basically it has been causing crash reports on chrome canary. Access is probably restricted because of the existence of the crash report data. Here's the relevant comment:

The sequence of calls to NameBuffer in CodeEventLogger::CodeCreateEvent is:

  • AppendString(String::cast(source))
  • AppendByte(':')
  • AppendInt(line)

After the above patch, the utf16buffer used within AppendString() is large enough that copying its contents over to the utf8buffer can fill up the latter. AppendByte() properly checks for that and returns. AppendInt() calls SNPrintF() with a zero-length buffer (because there are zero remaining bytes in utf8buffer), which makes vsnprintf_s() rather unhappy: it calls the InvalidParameter handler which triggers a crash.

This reverts commit 6fff47f as it is causing
issues in upstream: https://codereview.chromium.org/1390923004/

PR-URL: nodejs#3237
Reviewed-By: targos - Michaël Zasso <[email protected]>
ofrobots added a commit that referenced this pull request Oct 7, 2015
This reverts commit 6fff47f as it is causing
issues in upstream: https://codereview.chromium.org/1390923004/

PR-URL: #3237
Reviewed-By: targos - Michaël Zasso <[email protected]>
@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 7, 2015

@bnoordhuis
Copy link
Member

LGTM

@targos targos closed this Oct 7, 2015
@targos
Copy link
Member

targos commented Oct 7, 2015

landed in a334ddc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants