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: cherry-pick 37a3a15c3 from V8 upstream #16294

Closed
wants to merge 1 commit into from
Closed

deps: cherry-pick 37a3a15c3 from V8 upstream #16294

wants to merge 1 commit into from

Conversation

fhinkel
Copy link
Member

@fhinkel fhinkel commented Oct 18, 2017 •
edited
Loading

This is a backport of a fix for some V8 interceptor functions. I'd like to back port this so we can fix the vm module in Node 9. What do you think? Or should we wait until V8 64 lands regularly?

Original commit message:
[api] Intercept DefineProperty after Descriptor query

Analog to other interceptors, intercept the DefineProperty
call only after obtaining the property descriptor.

This behavior allows us to mirror calls on a sandboxed object
as it is needed in Node. See for example
#13265

Bug:
Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3
Reviewed-on: https://chromium-review.googlesource.com/725295
Reviewed-by: Andreas Haas ahaas@chromium.org
Commit-Queue: Franziska Hinkelmann franzih@chromium.org
Cr-Commit-Position: refs/heads/master@{#48683}

Checklist
Affected core subsystem(s)

deps

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Oct 18, 2017
@fhinkel fhinkel mentioned this pull request Oct 18, 2017
Closed
8 tasks
@fhinkel
Copy link
Member Author

fhinkel commented Oct 18, 2017

Depending PR #16293

@fhinkel fhinkel added vm Issues and PRs related to the vm subsystem. dont-land-on-v4.x labels Oct 18, 2017
jasnell
jasnell approved these changes Oct 18, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rubber stamp lgtm

bnoordhuis
bnoordhuis approved these changes Oct 18, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but shouldn't this cook for a little while longer upstream?

@targos targos added this to the 9.0.0 milestone Oct 18, 2017
@targos targos removed the v9.x label Oct 18, 2017
@fhinkel
Copy link
Member Author

fhinkel commented Oct 18, 2017

Definitely wait a few days (I assumed review and merge would take a bit anyways). AFAIK this interceptor is not used in Chrome, so we wouldn't gain much from Canary coverage.

targos
targos reviewed Oct 19, 2017
View reviewed changes
deps/v8/include/v8-version.h Outdated
@@ -11,7 +11,7 @@
#define V8_MAJOR_VERSION 6
#define V8_MINOR_VERSION 1
#define V8_BUILD_NUMBER 534
#define V8_PATCH_LEVEL 42
#define V8_PATCH_LEVEL 43
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that V8 6.2 has landed, we can update the embedder string instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Updated

@fhinkel fhinkel force-pushed the Oct/backport-api-change branch from 43a502f to f44e65a Compare October 19, 2017 06:33
@targos
Copy link
Member

targos commented Oct 21, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/10883/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1004/

@fhinkel
Copy link
Member Author

fhinkel commented Oct 21, 2017

@targos thanks for running the CI. FreeBSD failure looks unrelated to me:
not ok 1807 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max

@targos
Copy link
Member

targos commented Oct 21, 2017

Indeed. FreeBSD is a known issue: #16354
Do you have an idea for the failing V8 test? https://ci.nodejs.org/job/node-test-commit-v8-linux/1004/nodes=benchmark,v8test=v8test/testReport/junit/(root)/v8tests/message_console___nohard_abort/

@fhinkel
Copy link
Member Author

fhinkel commented Oct 21, 2017

That's weird. I don't see how that would be affected by my change. Could this have come in by a different PR on which we didn't run the V8 tests? 🤔

@targos
Copy link
Member

targos commented Oct 21, 2017

Maybe it's flaky. Let's try again: https://ci.nodejs.org/job/node-test-commit-v8-linux/1005/

@fhinkel
Copy link
Member Author

fhinkel commented Oct 21, 2017

I ran V8 6.2.414.32 with that patch and the message tests pass.

@fhinkel
Copy link
Member Author

fhinkel commented Oct 22, 2017 •
edited
Loading

Looks like the V8 CI is failing on master, too. #16398. So this should be good to go.

@fhinkel
deps: cherry-pick 37a3a15c3 from V8 upstream
2bc63c4 
Original commit message:
  [api] Intercept DefineProperty after Descriptor query

  Analog to other interceptors, intercept the DefineProperty
  call only after obtaining the property descriptor.

  This behavior allows us to mirror calls on a sandboxed object
  as it is needed in Node. See for example
  nodejs#13265

  Bug:
  Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3
  Reviewed-on: https://chromium-review.googlesource.com/725295
  Reviewed-by: Andreas Haas <ahaas@chromium.org>
  Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#48683}
@fhinkel fhinkel force-pushed the Oct/backport-api-change branch from f44e65a to 2bc63c4 Compare October 23, 2017 03:52
@targos
Copy link
Member

targos commented Oct 23, 2017

I think this can land before we figure out the issue with CI

fhinkel added a commit to fhinkel/node that referenced this pull request Oct 23, 2017
@fhinkel
deps: cherry-pick 37a3a15c3 from V8 upstream
801e61a 
Original commit message:
  [api] Intercept DefineProperty after Descriptor query

  Analog to other interceptors, intercept the DefineProperty
  call only after obtaining the property descriptor.

  This behavior allows us to mirror calls on a sandboxed object
  as it is needed in Node. See for example
  nodejs#13265

  Bug:
  Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3
  Reviewed-on: https://chromium-review.googlesource.com/725295
  Reviewed-by: Andreas Haas <ahaas@chromium.org>
  Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#48683}

PR-URL: nodejs#16294
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@fhinkel
Copy link
Member Author

fhinkel commented Oct 23, 2017

Thanks. Landed in 801e61a

New issue for unrelated CI issues: #16398

@fhinkel fhinkel closed this Oct 23, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@fhinkel @addaleax
deps: cherry-pick 37a3a15c3 from V8 upstream
cf84837 
Original commit message:
  [api] Intercept DefineProperty after Descriptor query

  Analog to other interceptors, intercept the DefineProperty
  call only after obtaining the property descriptor.

  This behavior allows us to mirror calls on a sandboxed object
  as it is needed in Node. See for example
  nodejs/node#13265

  Bug:
  Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3
  Reviewed-on: https://chromium-review.googlesource.com/725295
  Reviewed-by: Andreas Haas <ahaas@chromium.org>
  Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#48683}

PR-URL: nodejs/node#16294
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@ofrobots
Copy link
Contributor

@fhinkel I suspect this wouldn't get accepted for merge back to stable upstream, but I though I'd ask.

@fhinkel
Copy link
Member Author

fhinkel commented Nov 11, 2017

@ofrobots This is not really a bug fix (interceptors are not spec'ed) but it changes API behavior. Can't merge this upstream.

targos pushed a commit to targos/node that referenced this pull request Dec 6, 2017
@fhinkel @targos
deps: cherry-pick 37a3a15c3 from V8 upstream
3038d53 
Original commit message:
  [api] Intercept DefineProperty after Descriptor query

  Analog to other interceptors, intercept the DefineProperty
  call only after obtaining the property descriptor.

  This behavior allows us to mirror calls on a sandboxed object
  as it is needed in Node. See for example
  nodejs#13265

  Bug:
  Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3
  Reviewed-on: https://chromium-review.googlesource.com/725295
  Reviewed-by: Andreas Haas <ahaas@chromium.org>
  Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#48683}

PR-URL: nodejs#16294
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request Dec 6, 2017
@fhinkel @targos
deps: cherry-pick 37a3a15c3 from V8 upstream
e01a210 
Original commit message:
  [api] Intercept DefineProperty after Descriptor query

  Analog to other interceptors, intercept the DefineProperty
  call only after obtaining the property descriptor.

  This behavior allows us to mirror calls on a sandboxed object
  as it is needed in Node. See for example
  #13265

  Bug:
  Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3
  Reviewed-on: https://chromium-review.googlesource.com/725295
  Reviewed-by: Andreas Haas <ahaas@chromium.org>
  Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#48683}

PR-URL: #16294
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@fhinkel @addaleax
deps: cherry-pick 37a3a15c3 from V8 upstream
8326b78 
Original commit message:
  [api] Intercept DefineProperty after Descriptor query

  Analog to other interceptors, intercept the DefineProperty
  call only after obtaining the property descriptor.

  This behavior allows us to mirror calls on a sandboxed object
  as it is needed in Node. See for example
  nodejs/node#13265

  Bug:
  Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3
  Reviewed-on: https://chromium-review.googlesource.com/725295
  Reviewed-by: Andreas Haas <ahaas@chromium.org>
  Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#48683}

PR-URL: nodejs/node#16294
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit to targos/node that referenced this pull request Jan 15, 2018
@fhinkel @targos
deps: cherry-pick 37a3a15c3 from V8 upstream
2c8d93a 
Original commit message:
  [api] Intercept DefineProperty after Descriptor query

  Analog to other interceptors, intercept the DefineProperty
  call only after obtaining the property descriptor.

  This behavior allows us to mirror calls on a sandboxed object
  as it is needed in Node. See for example
  nodejs#13265

  Bug:
  Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3
  Reviewed-on: https://chromium-review.googlesource.com/725295
  Reviewed-by: Andreas Haas <ahaas@chromium.org>
  Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#48683}

PR-URL: nodejs#16294
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 16, 2018
@fhinkel @gibfahn
deps: cherry-pick 37a3a15c3 from V8 upstream
3c99f39 
Original commit message:
  [api] Intercept DefineProperty after Descriptor query

  Analog to other interceptors, intercept the DefineProperty
  call only after obtaining the property descriptor.

  This behavior allows us to mirror calls on a sandboxed object
  as it is needed in Node. See for example
  nodejs#13265

  Bug:
  Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3
  Reviewed-on: https://chromium-review.googlesource.com/725295
  Reviewed-by: Andreas Haas <ahaas@chromium.org>
  Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#48683}

PR-URL: nodejs#16294
Backport-PR-URL: nodejs#16413
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 22, 2018
@fhinkel @gibfahn
deps: cherry-pick 37a3a15c3 from V8 upstream
91dc965 
Original commit message:
  [api] Intercept DefineProperty after Descriptor query

  Analog to other interceptors, intercept the DefineProperty
  call only after obtaining the property descriptor.

  This behavior allows us to mirror calls on a sandboxed object
  as it is needed in Node. See for example
  nodejs#13265

  Bug:
  Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3
  Reviewed-on: https://chromium-review.googlesource.com/725295
  Reviewed-by: Andreas Haas <ahaas@chromium.org>
  Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#48683}

PR-URL: nodejs#16294
Backport-PR-URL: nodejs#16413
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit to targos/node that referenced this pull request Feb 7, 2018
@fhinkel @MylesBorins
deps: cherry-pick 37a3a15c3 from V8 upstream
21bfde6 
Original commit message:
  [api] Intercept DefineProperty after Descriptor query

  Analog to other interceptors, intercept the DefineProperty
  call only after obtaining the property descriptor.

  This behavior allows us to mirror calls on a sandboxed object
  as it is needed in Node. See for example
  nodejs#13265

  Bug:
  Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3
  Reviewed-on: https://chromium-review.googlesource.com/725295
  Reviewed-by: Andreas Haas <ahaas@chromium.org>
  Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#48683}

PR-URL: nodejs#16294
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
gibfahn pushed a commit that referenced this pull request Feb 18, 2018
@fhinkel @gibfahn
deps: cherry-pick 37a3a15c3 from V8 upstream
0b690a9 
Original commit message:
  [api] Intercept DefineProperty after Descriptor query

  Analog to other interceptors, intercept the DefineProperty
  call only after obtaining the property descriptor.

  This behavior allows us to mirror calls on a sandboxed object
  as it is needed in Node. See for example
  #13265

  Bug:
  Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3
  Reviewed-on: https://chromium-review.googlesource.com/725295
  Reviewed-by: Andreas Haas <ahaas@chromium.org>
  Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#48683}

PR-URL: #16294
Backport-PR-URL: #16413
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
8 participants
@fhinkel @targos @ofrobots @bnoordhuis @jasnell @cjihrig @MylesBorins @nodejs-github-bot