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

vm: use SetterCallback to set func declarations #12051

Closed
wants to merge 1 commit into from
Closed

vm: use SetterCallback to set func declarations #12051

wants to merge 1 commit into from

Conversation

AnnaMag
Copy link
Member

@AnnaMag AnnaMag commented Mar 26, 2017 •
edited
Loading

Currently, when in strict mode, function
declarations are copied on the sandbox by
CopyProperties(), which is not necessary
because GlobalPropertySetterCallback now
also intercepts function declarations.

Current version will break when CopyProperties()
is removed. This change maintains the behavior,
letting GlobalPropertySetterCallback do the task.

This is a preparation step for the removal
of CopyProperties().

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

vm

@nodejs-github-bot nodejs-github-bot 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 26, 2017
@AnnaMag
Copy link
Member Author

AnnaMag commented Mar 26, 2017

cc/ @fhinkel

@AnnaMag AnnaMag changed the title src: use SetterCallback to set func declarations vm: use SetterCallback to set func declarations Mar 26, 2017
bnoordhuis
bnoordhuis requested changes Mar 26, 2017
View reviewed changes
src/node_contextify.cc Outdated
bool is_function = value->IsFunction();

if (!is_declared && args.ShouldThrowOnError() && is_contextual_store
&& !is_function)
Copy link
Member

Choose a reason for hiding this comment

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

Style: && should go on the previous line.

I'm curious, what does this do for function f() {} vs. var f = function() vs. this.f = function() {}?

Copy link
Member Author

@AnnaMag AnnaMag Mar 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@bnoordhuis, thanks! I realized I should have clarified that this applies to strict mode (edited the commits). Otherwise, args.ShouldThrowOnError() = false , so this does not apply.
When in strict mode, the only change in behavior happens for function f() {}, where now we pass the condition: it is not declared, nor defined and "we are strict", so without checking that it might be a function declaration, it returns and never calls ctx->sandbox()->Set(property, value);, relying on CopyProperties() to set f on the sandbox.
For var f = function(), is_declared = true and
for this.f = function() {} is_contextual_store=false, so is_function does not make a difference.
Does this answer your question?

@AnnaMag AnnaMag force-pushed the set_func_declarations branch 2 times, most recently from 8ea51a4 to 3542e19 Compare March 27, 2017 15:19
@fhinkel
Copy link
Member

fhinkel commented Mar 29, 2017

ping @bnoordhuis

bnoordhuis
bnoordhuis approved these changes Mar 29, 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 if you add a comment explaining what you wrote in #12051 (comment).

@AnnaMag AnnaMag force-pushed the set_func_declarations branch 4 times, most recently from 4f64e2b to 78de0e6 Compare March 30, 2017 10:58
@AnnaMag
Copy link
Member Author

AnnaMag commented Mar 30, 2017

@bnoordhuis, comment added. Thanks!

bnoordhuis
bnoordhuis approved these changes Mar 30, 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. CI: https://ci.nodejs.org/job/node-test-pull-request/7107/ (last 15 or so runs were red for some reason so perhaps not a 100% reliable gauge right now.)

TimothyGu
TimothyGu reviewed Mar 30, 2017
View reviewed changes
src/node_contextify.cc Outdated
bool is_function = value->IsFunction();

if (!is_declared && args.ShouldThrowOnError() && is_contextual_store &&
!is_function)
Copy link
Member

Choose a reason for hiding this comment

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

nit: missing space before !is_function

@AnnaMag
vm: use SetterCallback to set func declarations
0836402 
Currently, when in strict mode, function
declarations are copied on the sandbox by
CopyProperties(), which is not necessary
and will break when CP is removed.

This change maintains current behavior,
letting GlobalPropertySetterCallback
copy functions on the sandbox instead
of using CP to do the task.
@addaleax
Copy link
Member

Landed in 241de51

@addaleax addaleax closed this Mar 31, 2017
addaleax pushed a commit that referenced this pull request Mar 31, 2017
@AnnaMag @addaleax
vm: use SetterCallback to set func declarations
241de51 
Currently, when in strict mode, function
declarations are copied on the sandbox by
CopyProperties(), which is not necessary
and will break when CP is removed.

This change maintains current behavior,
letting GlobalPropertySetterCallback
copy functions on the sandbox instead
of using CP to do the task.

PR-URL: #12051
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
@AnnaMag @italoacasas
vm: use SetterCallback to set func declarations
2f88de1 
Currently, when in strict mode, function
declarations are copied on the sandbox by
CopyProperties(), which is not necessary
and will break when CP is removed.

This change maintains current behavior,
letting GlobalPropertySetterCallback
copy functions on the sandbox instead
of using CP to do the task.

PR-URL: nodejs#12051
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
@MylesBorins
Copy link
Contributor

should this be backported?

@AnnaMag
Copy link
Member Author

AnnaMag commented Apr 20, 2017

It works as of
https://chromium.googlesource.com/v8/v8/+/8439401d2dfdb9fa328c758cca689289922a32d1
Prior to that CP was needed.

@MylesBorins
Copy link
Contributor

This does not land cleanly please backport

@AnnaMag AnnaMag deleted the set_func_declarations branch March 15, 2018 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@fhinkel fhinkel fhinkel approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@AnnaMag @fhinkel @addaleax @MylesBorins @bnoordhuis @jasnell @TimothyGu @nodejs-github-bot