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

deps: backport 22c5e46 from V8 #7584

Merged
merged 1 commit into from
Jul 11, 2016
Merged

deps: backport 22c5e46 from V8 #7584

merged 1 commit into from
Jul 11, 2016

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Jul 7, 2016 •
edited
Loading

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 MylesBorins added v8 engine Issues and PRs related to the V8 dependency. v4.x labels Jul 7, 2016
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Jul 7, 2016 •
edited
Loading

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

@MylesBorins
Copy link
Contributor Author

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

@bnoordhuis
Copy link
Member

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

@misterdjules
Copy link

misterdjules commented Jul 7, 2016 •
edited
Loading

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
Copy link

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
Copy link

@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
Copy link
Member

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

@anandsuresh
Copy link
Contributor

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

@misterdjules
Copy link

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

@misterdjules
Copy link

@thealphanerd Thanks for bumping the patch level!

@MylesBorins
Copy link
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 MylesBorins mentioned this pull request Jul 7, 2016
Closed
@MylesBorins
Copy link
Contributor Author

can I get some LGTM's on this please 😄

@targos
Copy link
Member

targos commented Jul 8, 2016

LGTM

1 similar comment
@bnoordhuis
Copy link
Member

LGTM

@ofrobots
Copy link
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
@MylesBorins MylesBorins merged commit 4184cb0 into nodejs:v4.x-staging Jul 11, 2016
@MylesBorins
Copy link
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
Merged
richardlau added a commit to ibmruntimes/node that referenced this pull request Jul 14, 2016
@richardlau
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
richardlau added a commit to ibmruntimes/node that referenced this pull request Jul 18, 2016
@richardlau
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
@misterdjules misterdjules mentioned this pull request Sep 15, 2016
Closed
@MylesBorins MylesBorins deleted the backport/22c5e46 branch October 11, 2016 18:59
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
@MylesBorins @bnoordhuis @misterdjules @anandsuresh @targos @ofrobots