FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: backport 22c5e46 from V8 by MylesBorins · Pull Request #7584 · nodejs/node · GitHub

/ node Public

deps: backport 22c5e46 from V8 - #7584

Merged
MylesBorins merged 1 commit into
nodejs:v4.x-stagingfrom
MylesBorins:backport/22c5e46
Jul 11, 2016
ConversationCommits (1)ChecksFiles changed
Merged

deps: backport 22c5e46 from V8#7584
MylesBorins merged 1 commit into
nodejs:v4.x-stagingfrom
MylesBorins:backport/22c5e46

Conversation

MylesBorins commented Jul 7, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

deps

Description of change

This removes the diagnostic code for the issue described in
https://bugs.chromium.org/p/chromium/issues/detail?id=454297. That issue
is private, probably due to the fact that it contains information about
a security vulnerability.

The original issue was fixed in V8 by
https://codereview.chromium.org/1286343004, which was integrated into
node v4.x with c431725, so there's no
need for the corresponding diagnostic code anymore.

Original commit message:

[heap] Remove debugging code of crbug/454297.

BUG=

Review URL: https://codereview.chromium.org/1420253002

Cr-Commit-Position: refs/heads/master@{#31523}

Fixes #7574.

MylesBorins added v8 engine Issues and PRs related to the V8 dependency. v4.x labels Jul 7, 2016

MylesBorins commented Jul 7, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

v8 test: https://ci.nodejs.org/job/node-test-commit-v8-linux/181/

MylesBorins commented Jul 7, 2016

Copy link
Copy Markdown
Contributor Author

/cc @nodejs/lts @nodejs/v8 @misterdjules

bnoordhuis commented Jul 7, 2016

Copy link
Copy Markdown
Member

You need to bump the patch level in v8-version.h.

misterdjules commented Jul 7, 2016 •
edited
Loading

Copy link
Copy Markdown

It's still not clear to me why the diagnostic code was left, and why it still makes some node programs crash when the bug fix for the associated issue is present in the code base. I wouldn't want to merge this change before these two questions are answered.

misterdjules commented Jul 7, 2016

Copy link
Copy Markdown

It's still not clear to me why the diagnostic code was left, and why it still makes some node programs crash when the bug fix for the associated issue is present in the code base.

The reason why the diagnostic code crashes on some OSes (like SmartOS) while the fix for the original code is in the code base is now clear per #7574 (comment), so I'm OK with merging this, as long as @bnoordhuis' comments are addressed.

misterdjules commented Jul 7, 2016

Copy link
Copy Markdown

@bnoordhuis

You need to bump the patch level in v8-version.h.

Wouldn't we end up with a V8 patch level that doesn't exist in any V8 release line?

bnoordhuis commented Jul 7, 2016

Copy link
Copy Markdown
Member

Yes, but that's alright for 4.5 because we're the de facto upstream.

anandsuresh commented Jul 7, 2016

Copy link
Copy Markdown
Contributor

Any ideas when this would be merged and released for 4.5?

misterdjules commented Jul 7, 2016

Copy link
Copy Markdown

@thealphanerd Do you mind updating the patch level? Otherwise I'd need to create another PR from another branch.

misterdjules commented Jul 7, 2016

Copy link
Copy Markdown

@thealphanerd Thanks for bumping the patch level!

MylesBorins commented Jul 7, 2016

Copy link
Copy Markdown
Contributor Author

bumped patch level. Completely at a loss as to why I couldn't find the file at first. Will make sure to bump that in the future

I'm going to mention in nodejs/Release#111 that we should perhaps look at having an official LTS 4.5 release.

/cc @natorion

MylesBorins mentioned this pull request Jul 7, 2016
Closed

MylesBorins commented Jul 8, 2016

Copy link
Copy Markdown
Contributor Author

can I get some LGTM's on this please 😄

targos commented Jul 8, 2016

Copy link
Copy Markdown
Member

LGTM

1 similar comment

bnoordhuis commented Jul 8, 2016

Copy link
Copy Markdown
Member

LGTM

ofrobots commented Jul 11, 2016

Copy link
Copy Markdown
Contributor

LGTM.

deps: backport 22c5e46 from V8
4184cb0 
This removes the diagnostic code for the issue described in
https://bugs.chromium.org/p/chromium/issues/detail?id=454297. That issue
is private, probably due to the fact that it contains information about
a security vulnerability.

The original issue was fixed in V8 by
https://codereview.chromium.org/1286343004, which was integrated into
node v4.x with c431725, so there's no
need for the corresponding diagnostic code anymore.

Original commit message:

  [heap] Remove debugging code of crbug/454297.

  BUG=

  Review URL: https://codereview.chromium.org/1420253002

  Cr-Commit-Position: refs/heads/master@{nodejs#31523}

PR-URL: nodejs#7584
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins merged commit 4184cb0 into nodejs:v4.x-staging Jul 11, 2016

MylesBorins commented Jul 11, 2016

Copy link
Copy Markdown
Contributor Author

thank everyone landed in 4184cb0

MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
deps: backport 22c5e46 from V8
22bb7ca 
This removes the diagnostic code for the issue described in
https://bugs.chromium.org/p/chromium/issues/detail?id=454297. That issue
is private, probably due to the fact that it contains information about
a security vulnerability.

The original issue was fixed in V8 by
https://codereview.chromium.org/1286343004, which was integrated into
node v4.x with c431725, so there's no
need for the corresponding diagnostic code anymore.

Original commit message:

  [heap] Remove debugging code of crbug/454297.

  BUG=

  Review URL: https://codereview.chromium.org/1420253002

  Cr-Commit-Position: refs/heads/master@{#31523}

PR-URL: #7584
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
deps: backport 22c5e46 from V8
d2fd26a 
This removes the diagnostic code for the issue described in
https://bugs.chromium.org/p/chromium/issues/detail?id=454297. That issue
is private, probably due to the fact that it contains information about
a security vulnerability.

The original issue was fixed in V8 by
https://codereview.chromium.org/1286343004, which was integrated into
node v4.x with c431725, so there's no
need for the corresponding diagnostic code anymore.

Original commit message:

  [heap] Remove debugging code of crbug/454297.

  BUG=

  Review URL: https://codereview.chromium.org/1420253002

  Cr-Commit-Position: refs/heads/master@{#31523}

PR-URL: #7584
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins mentioned this pull request Jul 12, 2016
Merged
richardlau added a commit to ibmruntimes/node that referenced this pull request Jul 14, 2016
deps: port d2fd26a to deps/v8z
bce7b7a 
Original commit message:

deps backport 22c5e46 from V8

This removes the diagnostic code for the issue described in
https://bugs.chromium.org/p/chromium/issues/detail?id=454297. That issue
is private, probably due to the fact that it contains information about
a security vulnerability.

The original issue was fixed in V8 by
https://codereview.chromium.org/1286343004, which was integrated into
node v4.x with c431725, so there's no
need for the corresponding diagnostic code anymore.

Original commit message:

  [heap] Remove debugging code of crbug/454297.

  BUG=

  Review URL: https://codereview.chromium.org/1420253002

  Cr-Commit-Position: refs/heads/master@{#31523}

PR-URL: nodejs/node#7584
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
deps: backport 22c5e46 from V8
8a3e6ea 
This removes the diagnostic code for the issue described in
https://bugs.chromium.org/p/chromium/issues/detail?id=454297. That issue
is private, probably due to the fact that it contains information about
a security vulnerability.

The original issue was fixed in V8 by
https://codereview.chromium.org/1286343004, which was integrated into
node v4.x with c431725, so there's no
need for the corresponding diagnostic code anymore.

Original commit message:

  [heap] Remove debugging code of crbug/454297.

  BUG=

  Review URL: https://codereview.chromium.org/1420253002

  Cr-Commit-Position: refs/heads/master@{#31523}

PR-URL: #7584
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
deps: backport 22c5e46 from V8
4107b5d 
This removes the diagnostic code for the issue described in
https://bugs.chromium.org/p/chromium/issues/detail?id=454297. That issue
is private, probably due to the fact that it contains information about
a security vulnerability.

The original issue was fixed in V8 by
https://codereview.chromium.org/1286343004, which was integrated into
node v4.x with c431725, so there's no
need for the corresponding diagnostic code anymore.

Original commit message:

  [heap] Remove debugging code of crbug/454297.

  BUG=

  Review URL: https://codereview.chromium.org/1420253002

  Cr-Commit-Position: refs/heads/master@{#31523}

PR-URL: #7584
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
richardlau added a commit to ibmruntimes/node that referenced this pull request Jul 18, 2016
deps: port 4107b5d to deps/v8z
f5767fd 
Original commit message:

deps backport 22c5e46 from V8

This removes the diagnostic code for the issue described in
https://bugs.chromium.org/p/chromium/issues/detail?id=454297. That issue
is private, probably due to the fact that it contains information about
a security vulnerability.

The original issue was fixed in V8 by
https://codereview.chromium.org/1286343004, which was integrated into
node v4.x with c431725, so there's no
need for the corresponding diagnostic code anymore.

Original commit message:

  [heap] Remove debugging code of crbug/454297.

  BUG=

  Review URL: https://codereview.chromium.org/1420253002

  Cr-Commit-Position: refs/heads/master@{#31523}

PR-URL: nodejs/node#7584
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
misterdjules mentioned this pull request Sep 15, 2016
Closed
MylesBorins deleted the backport/22c5e46 branch October 11, 2016 18:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL