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

deps: cherry-pick 37a3a15c3 from V8 upstream by fhinkel · Pull Request #16294 · nodejs/node · GitHub

/ node Public

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

Closed
fhinkel wants to merge 1 commit into
nodejs:masterfrom
fhinkel:Oct/backport-api-change
ConversationCommits (1)ChecksFiles changed
Closed

deps: cherry-pick 37a3a15c3 from V8 upstream#16294
fhinkel wants to merge 1 commit into
nodejs:masterfrom
fhinkel:Oct/backport-api-change

Conversation

fhinkel commented Oct 18, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 added the v8 engine Issues and PRs related to the V8 dependency. label Oct 18, 2017
fhinkel mentioned this pull request Oct 18, 2017
Closed
8 tasks

fhinkel commented Oct 18, 2017

Copy link
Copy Markdown
Contributor Author

Depending PR #16293

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

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

rubber stamp lgtm

bnoordhuis approved these changes Oct 18, 2017
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

targos added this to the 9.0.0 milestone Oct 18, 2017
targos removed the v9.x label Oct 18, 2017

fhinkel commented Oct 18, 2017

Copy link
Copy Markdown
Contributor Author

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 reviewed Oct 19, 2017
View reviewed changes
Comment thread deps/v8/include/v8-version.h Outdated

targos Oct 19, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

fhinkel Oct 19, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks! Updated

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

targos commented Oct 21, 2017

Copy link
Copy Markdown
Member

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 commented Oct 21, 2017

Copy link
Copy Markdown
Contributor Author

@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 commented Oct 21, 2017

Copy link
Copy Markdown
Member

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 commented Oct 21, 2017

Copy link
Copy Markdown
Contributor Author

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 commented Oct 21, 2017

Copy link
Copy Markdown
Member

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

fhinkel commented Oct 21, 2017

Copy link
Copy Markdown
Contributor Author

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

fhinkel commented Oct 22, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

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@{#48683}
fhinkel force-pushed the Oct/backport-api-change branch from f44e65a to 2bc63c4 Compare October 23, 2017 03:52

targos commented Oct 23, 2017

Copy link
Copy Markdown
Member

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
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@{#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 commented Oct 23, 2017

Copy link
Copy Markdown
Contributor Author

Thanks. Landed in 801e61a

New issue for unrelated CI issues: #16398

fhinkel closed this Oct 23, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
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 mentioned this pull request Oct 30, 2017
Merged

ofrobots commented Nov 10, 2017

Copy link
Copy Markdown
Contributor

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

fhinkel commented Nov 11, 2017

Copy link
Copy Markdown
Contributor Author

@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
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@{#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
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
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
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@{#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
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@{#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
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@{#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
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@{#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
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>
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

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL