Skip to content

Commit

Permalink
src: refactor CopyProperties to remove JS
Browse files Browse the repository at this point in the history 
CopyProperties() is refactored to use
the V8 5.5 DefineProperty() API call.
The change does not alter current behaviour.
It is a step prior to removing the function
CopyProperties, which becomes reduntant
after fixes of V8 SetNamedPropertyHandler
in 5.5. V8.

Strings used as property attributes
(value, enumerable etc) and accessors
are defined as persistent strings
in src/env.h

PR-URL: nodejs#11102
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
@AnnaMag @krydos
AnnaMag authored and krydos committed Feb 25, 2017
1 parent 27eaa49 commit 4378df8
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 32 deletions.
5 changes: 5 additions & 0 deletions src/env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,15 @@ namespace node {
V(oncertcb_string, "oncertcb") \
V(onclose_string, "_onclose") \
V(code_string, "code") \
V(configurable_string, "configurable") \
V(cwd_string, "cwd") \
V(dest_string, "dest") \
V(detached_string, "detached") \
V(disposed_string, "_disposed") \
V(domain_string, "domain") \
V(emitting_top_level_domain_error_string, "_emittingTopLevelDomainError") \
V(exchange_string, "exchange") \
V(enumerable_string, "enumerable") \
V(idle_string, "idle") \
V(irq_string, "irq") \
V(encoding_string, "encoding") \
Expand All @@ -112,6 +114,7 @@ namespace node {
V(file_string, "file") \
V(fingerprint_string, "fingerprint") \
V(flags_string, "flags") \
V(get_string, "get") \
V(gid_string, "gid") \
V(handle_string, "handle") \
V(heap_total_string, "heapTotal") \
Expand Down Expand Up @@ -191,6 +194,7 @@ namespace node {
V(service_string, "service") \
V(servername_string, "servername") \
V(session_id_string, "sessionId") \
V(set_string, "set") \
V(shell_string, "shell") \
V(signal_string, "signal") \
V(size_string, "size") \
Expand All @@ -217,6 +221,7 @@ namespace node {
V(username_string, "username") \
V(valid_from_string, "valid_from") \
V(valid_to_string, "valid_to") \
V(value_string, "value") \
V(verify_error_string, "verifyError") \
V(version_string, "version") \
V(weight_string, "weight") \
Expand Down
73 changes: 41 additions & 32 deletions src/node_contextify.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ using v8::ObjectTemplate;
using v8::Persistent;
using v8::PropertyAttribute;
using v8::PropertyCallbackInfo;
using v8::PropertyDescriptor;
using v8::Script;
using v8::ScriptCompiler;
using v8::ScriptOrigin;
Expand Down Expand Up @@ -132,41 +133,49 @@ class ContextifyContext {
return;

if (!has.FromJust()) {
// Could also do this like so:
//
// PropertyAttribute att = global->GetPropertyAttributes(key_v);
// Local<Value> val = global->Get(key_v);
// sandbox->ForceSet(key_v, val, att);
//
// However, this doesn't handle ES6-style properties configured with
// Object.defineProperty, and that's exactly what we're up against at
// this point. ForceSet(key,val,att) only supports value properties
// with the ES3-style attribute flags (DontDelete/DontEnum/ReadOnly),
// which doesn't faithfully capture the full range of configurations
// that can be done using Object.defineProperty.
if (clone_property_method.IsEmpty()) {
Local<String> code = FIXED_ONE_BYTE_STRING(env()->isolate(),
"(function cloneProperty(source, key, target) {\n"
" if (key === 'Proxy') return;\n"
" try {\n"
" var desc = Object.getOwnPropertyDescriptor(source, key);\n"
" if (desc.value === source) desc.value = target;\n"
" Object.defineProperty(target, key, desc);\n"
" } catch (e) {\n"
" // Catch sealed properties errors\n"
" }\n"
"})");

Local<Script> script =
Script::Compile(context, code).ToLocalChecked();
clone_property_method = Local<Function>::Cast(script->Run());
CHECK(clone_property_method->IsFunction());
Local<Object> desc_vm_context =
global->GetOwnPropertyDescriptor(context, key)
.ToLocalChecked().As<Object>();

bool is_accessor =
desc_vm_context->Has(context, env()->get_string()).FromJust() ||
desc_vm_context->Has(context, env()->set_string()).FromJust();

auto define_property_on_sandbox = [&] (PropertyDescriptor* desc) {
desc->set_configurable(desc_vm_context
->Get(context, env()->configurable_string()).ToLocalChecked()
->BooleanValue(context).FromJust());
desc->set_enumerable(desc_vm_context
->Get(context, env()->enumerable_string()).ToLocalChecked()
->BooleanValue(context).FromJust());
sandbox_obj->DefineProperty(context, key, *desc);
};

if (is_accessor) {
Local<Function> get =
desc_vm_context->Get(context, env()->get_string())
.ToLocalChecked().As<Function>();
Local<Function> set =
desc_vm_context->Get(context, env()->set_string())
.ToLocalChecked().As<Function>();

PropertyDescriptor desc(get, set);
define_property_on_sandbox(&desc);
} else {
Local<Value> value =
desc_vm_context->Get(context, env()->value_string())
.ToLocalChecked();

bool writable =
desc_vm_context->Get(context, env()->writable_string())
.ToLocalChecked()->BooleanValue(context).FromJust();

PropertyDescriptor desc(value, writable);
define_property_on_sandbox(&desc);
}
Local<Value> args[] = { global, key, sandbox_obj };
clone_property_method->Call(global, arraysize(args), args);
}
}
}
}


// This is an object that just keeps an internal pointer to this
Expand Down

0 comments on commit 4378df8

Please sign in to comment.