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

Backport 5786 to v5.x #5800

Closed
wants to merge 1 commit into from
Closed

Backport 5786 to v5.x #5800

wants to merge 1 commit into from

Conversation

ofrobots
Copy link
Contributor

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?

Affected core subsystem(s)

contextify

Description of change

Backport of #5786 to v5.x. The implementation is slightly different from master because SetPrivate is not available in v5.x. This fixes the contextify regression in 5.9.0 (#5768).

R=@bnoordhuis
/cc @nodejs/release
CI: https://ci.nodejs.org/job/node-test-pull-request/1969/

When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

This is a backport of nodejs#5786 to v5.x.

Ref: nodejs#5786
@ofrobots ofrobots added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Mar 19, 2016
@mscdex mscdex added the v5.x label Mar 19, 2016
@bnoordhuis
Copy link
Member

LGTM

1 similar comment
@cjihrig
Copy link
Contributor

cjihrig commented Mar 20, 2016

LGTM

@rvagg
Copy link
Member

rvagg commented Mar 20, 2016

The same thing would apply to the v4.x version of the contextify changes?

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

LGTM if CI is green: https://ci.nodejs.org/job/node-test-pull-request/1993/

@MylesBorins
Copy link
Contributor

@rvagg the code that this fixes has not yet landed on LTS.

--> #5392

MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

This is a backport of #5786 to v5.x.

Ref: #5786
PR-URL: #5800
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

landed in c5d8369

evanlucas added a commit that referenced this pull request Mar 21, 2016
Notable changes:

* **contextify**: Fix vm regression introduced in v5.9.0 (Ali Ijaz
Sheikh) #5800
@rvagg
Copy link
Member

rvagg commented Mar 22, 2016

@thealphanerd right but I'm asking if when we do land the v4.x version of the original changes, does this one need to apply over the top of that as well. It think the answer is yes but confirmation would be nice.

@MylesBorins
Copy link
Contributor

@rvagg so this is the original change ==> #5392

It is currently labelled for LTS watch. As far as I know the changes should backport to v4.x, but since the fix needed to be modified for v5.x, I don't really know. I think @ofrobots can likely give us a better view on this.

@ofrobots
Copy link
Contributor Author

Yes, this fix will need to be ported when #5392 gets back-ported to v4.x.

Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
Notable changes:

* **contextify**: Fix vm regression introduced in v5.9.0 (Ali Ijaz
Sheikh) #5800
Fishrock123 added a commit that referenced this pull request Mar 23, 2016
Notable changes:

* buffer: Now properly throws RangeErrors on out-of-bounds writes (Matt
Loring) #5605
  - This effects write{Float|Double} when the noAssert option is not
used.
* timers:
  - Returned timeout objects now have a Timeout constructor name
(Jeremiah Senkpiel) #5793
  - Performance of Immediate processing is now ~20-40% faster (Brian
White) #4169
* vm: Fixed a contextify regression introduced in v5.9.0 (Ali Ijaz
Sheikh) #5800

PR-URL: #5831
Fishrock123 added a commit that referenced this pull request Mar 23, 2016
Notable changes:

* buffer: Now properly throws RangeErrors on out-of-bounds writes (Matt
Loring) #5605
  - This effects write{Float|Double} when the noAssert option is not
used.
* timers:
  - Returned timeout objects now have a Timeout constructor name
(Jeremiah Senkpiel) #5793
  - Performance of Immediate processing is now ~20-40% faster (Brian
White) #4169
* vm: Fixed a contextify regression introduced in v5.9.0 (Ali Ijaz
Sheikh) #5800

PR-URL: #5831
Fishrock123 added a commit that referenced this pull request Mar 23, 2016
Notable changes:

* buffer: Now properly throws RangeErrors on out-of-bounds writes (Matt
Loring) #5605
  - This effects write{Float|Double} when the noAssert option is not
used.
* timers:
  - Returned timeout objects now have a Timeout constructor name
(Jeremiah Senkpiel) #5793
  - Performance of Immediate processing is now ~20-40% faster (Brian
White) #4169
* vm: Fixed a contextify regression introduced in v5.9.0 (Ali Ijaz
Sheikh) #5800

PR-URL: #5831
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 24, 2016
Notable changes:

* buffer: Now properly throws RangeErrors on out-of-bounds writes (Matt
Loring) nodejs#5605
  - This effects write{Float|Double} when the noAssert option is not
used.
* timers:
  - Returned timeout objects now have a Timeout constructor name
(Jeremiah Senkpiel) nodejs#5793
  - Performance of Immediate processing is now ~20-40% faster (Brian
White) nodejs#4169
* vm: Fixed a contextify regression introduced in v5.9.0 (Ali Ijaz
Sheikh) nodejs#5800

PR-URL: nodejs#5831
rvagg pushed a commit to rvagg/io.js that referenced this pull request May 20, 2016
When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

This is a backport of nodejs#5786 to v5.x.

Ref: nodejs#5786
PR-URL: nodejs#5800
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 20, 2016
When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

This is a backport of #5786 to v5.x.

Ref: #5786
PR-URL: #5800
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants