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

src: make cross-context MakeCallback() calls work #9221

Merged
merged 1 commit into from
Oct 25, 2016

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Oct 21, 2016

return handle_scope.Escape(Local<Value>::New(
isolate,
MakeCallback(env, recv.As<Value>(), callback, argc, argv)));
Environment* env = Environment::GetCurrent(callback->CreationContext());
Copy link
Member Author

Choose a reason for hiding this comment

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

callback->CreationContext() seems a little more correct than recv->CreationContext() because the former is the context the actual callback takes place in but it's admittedly a mostly academic difference.

@mscdex mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 21, 2016
@bnoordhuis
Copy link
Member Author

@addaleax Your thoughts, please?

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

(sorry for not reviewing sooner, as mentioned I’ll be somewhat more busy now that classes have started again)

@addaleax addaleax added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 24, 2016
Check that invoking a callback on a receiver from a different context
works.

It ran afoul of an `env->context() == isolate->GetCurrentContext()`
assertion so retrieve the environment from the callback context and
the context to enter from the environment's context() method.

We could also have retrieved the environment from the receiver's context
and that would have made little practical difference.  It just seemed
more correct to get it from the callback context because that is the
actual execution context.

PR-URL: nodejs#9221
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@bnoordhuis bnoordhuis closed this Oct 25, 2016
@bnoordhuis bnoordhuis deleted the fix-foreign-context-callback branch October 25, 2016 11:30
@bnoordhuis
Copy link
Member Author

No problem, thanks for reviewing.

@bnoordhuis bnoordhuis merged commit 921d2b0 into nodejs:master Oct 25, 2016
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
Check that invoking a callback on a receiver from a different context
works.

It ran afoul of an `env->context() == isolate->GetCurrentContext()`
assertion so retrieve the environment from the callback context and
the context to enter from the environment's context() method.

We could also have retrieved the environment from the receiver's context
and that would have made little practical difference.  It just seemed
more correct to get it from the callback context because that is the
actual execution context.

PR-URL: #9221
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

@bnoordhuis should this be backported?

@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

@bnoordhuis should this be backported?

ping @bnoordhuis

@addaleax
Copy link
Member

@gibfahn I think this is good to be backported too.

gibfahn pushed a commit that referenced this pull request Jun 18, 2017
Check that invoking a callback on a receiver from a different context
works.

It ran afoul of an `env->context() == isolate->GetCurrentContext()`
assertion so retrieve the environment from the callback context and
the context to enter from the environment's context() method.

We could also have retrieved the environment from the receiver's context
and that would have made little practical difference.  It just seemed
more correct to get it from the callback context because that is the
actual execution context.

PR-URL: #9221
Reviewed-By: Anna Henningsen <anna@addaleax.net>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Check that invoking a callback on a receiver from a different context
works.

It ran afoul of an `env->context() == isolate->GetCurrentContext()`
assertion so retrieve the environment from the callback context and
the context to enter from the environment's context() method.

We could also have retrieved the environment from the receiver's context
and that would have made little practical difference.  It just seemed
more correct to get it from the callback context because that is the
actual execution context.

PR-URL: #9221
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Check that invoking a callback on a receiver from a different context
works.

It ran afoul of an `env->context() == isolate->GetCurrentContext()`
assertion so retrieve the environment from the callback context and
the context to enter from the environment's context() method.

We could also have retrieved the environment from the receiver's context
and that would have made little practical difference.  It just seemed
more correct to get it from the callback context because that is the
actual execution context.

PR-URL: #9221
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants