vm: use SetterCallback to set func declarations#12051
vm: use SetterCallback to set func declarations#12051AnnaMag wants to merge 1 commit intonodejs:masterfrom
Conversation
nodejs-github-bot
added
c++
|
cc/ @fhinkel |
AnnaMag
force-pushed
the
set_func_declarations
branch
from
f280b5c to
fae64ee
Compare
AnnaMag
changed the title
src/node_contextify.cc
Outdated
There was a problem hiding this comment.
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() {}?
There was a problem hiding this comment.
@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
force-pushed
the
set_func_declarations
branch
2 times, most recently
from
8ea51a4 to
3542e19
Compare
|
ping @bnoordhuis |
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
LGTM if you add a comment explaining what you wrote in #12051 (comment).
AnnaMag
force-pushed
the
set_func_declarations
branch
4 times, most recently
from
4f64e2b to
78de0e6
Compare
|
@bnoordhuis, comment added. Thanks! |
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
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.)
src/node_contextify.cc
Outdated
There was a problem hiding this comment.
nit: missing space before !is_function
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.
AnnaMag
force-pushed
the
set_func_declarations
branch
from
78de0e6 to
0836402
Compare
|
Landed in 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>
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>
|
should this be backported? |
|
It works as of |
|
This does not land cleanly please backport |
8 participants
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), orvcbuild test(Windows) passesAffected core subsystem(s)
vm