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

contextify: tie lifetimes of context & sandbox #5786

Closed
wants to merge 1 commit into from

Conversation

ofrobots
Copy link
Contributor

Pull Request check-list

  • 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][0]?
  • 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

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.

Fixes: #5768
R=@bnoordhuis
/cc @evanlucas @cjihrig

Note that this is not trivially back-portable to 5.x. SetPrivate doesn't exist on 5.x, but I can create a new PR once this lands on master. This does continue to address the memory growth issue reported in #3113, but we should get this vetted before backporting.

CI: https://ci.nodejs.org/job/node-test-pull-request/1960/

@mscdex mscdex 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 18, 2016
@MylesBorins
Copy link
Contributor

@ofrobots since this will take time to backport to v5.x do you think we should be keeping #5782 open and cutting a v5.9.1 with the changes reverted?

@jasnell
Copy link
Member

jasnell commented Mar 18, 2016

LGTM

1 similar comment
@cjihrig
Copy link
Contributor

cjihrig commented Mar 18, 2016

LGTM

@ofrobots
Copy link
Contributor Author

I want to avoid a deeper hole, specially that this is going to be a Friday release. If bugs in this code still remain, they are going to be gc (i.e. timing) related. I think it would be best to proceed with a revert (i.e. #5782) and allow in depth review on this PR.

@Knighton910
Copy link

+1

@@ -1,3 +1,4 @@
// Flags: --expose-gc
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: other two locations where --expose-gc is used the comment is placed directly after the 'use strict';.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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.

Fixes: nodejs#5768
PR-URL: nodejs#5786
@trevnorris
Copy link
Contributor

LGTM

1 similar comment
@bnoordhuis
Copy link
Member

LGTM

ofrobots added a commit to ofrobots/node that referenced this pull request Mar 19, 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
@ofrobots ofrobots mentioned this pull request Mar 19, 2016
3 tasks
ofrobots added a commit that referenced this pull request Mar 19, 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.

Fixes: #5768
PR-URL: #5786
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@ofrobots
Copy link
Contributor Author

Thanks. Landed in a53b2ac. PR for backport to v5.x is here: #5800.

@ofrobots ofrobots closed this Mar 19, 2016
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>
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>
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.

vm: recent contextify changes cause segmentation fault in node v5.9
9 participants