Skip to content

Commit 67af1ad

Browse files
AnnaMagfhinkel
AnnaMag
authored and
fhinkel
committed
src: refactor CopyProperties to remove JS
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: #11102 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 8b04bc9 commit 67af1ad

File tree

2 files changed

+46
-32
lines changed

2 files changed

+46
-32
lines changed

‎src/env.h

+5
Original file line numberDiff line numberDiff line change
@@ -82,13 +82,15 @@ namespace node {
8282
V(oncertcb_string, "oncertcb") \
8383
V(onclose_string, "_onclose") \
8484
V(code_string, "code") \
85+
V(configurable_string, "configurable") \
8586
V(cwd_string, "cwd") \
8687
V(dest_string, "dest") \
8788
V(detached_string, "detached") \
8889
V(disposed_string, "_disposed") \
8990
V(domain_string, "domain") \
9091
V(emitting_top_level_domain_error_string, "_emittingTopLevelDomainError") \
9192
V(exchange_string, "exchange") \
93+
V(enumerable_string, "enumerable") \
9294
V(idle_string, "idle") \
9395
V(irq_string, "irq") \
9496
V(encoding_string, "encoding") \
@@ -112,6 +114,7 @@ namespace node {
112114
V(file_string, "file") \
113115
V(fingerprint_string, "fingerprint") \
114116
V(flags_string, "flags") \
117+
V(get_string, "get") \
115118
V(gid_string, "gid") \
116119
V(handle_string, "handle") \
117120
V(heap_total_string, "heapTotal") \
@@ -191,6 +194,7 @@ namespace node {
191194
V(service_string, "service") \
192195
V(servername_string, "servername") \
193196
V(session_id_string, "sessionId") \
197+
V(set_string, "set") \
194198
V(shell_string, "shell") \
195199
V(signal_string, "signal") \
196200
V(size_string, "size") \
@@ -217,6 +221,7 @@ namespace node {
217221
V(username_string, "username") \
218222
V(valid_from_string, "valid_from") \
219223
V(valid_to_string, "valid_to") \
224+
V(value_string, "value") \
220225
V(verify_error_string, "verifyError") \
221226
V(version_string, "version") \
222227
V(weight_string, "weight") \

‎src/node_contextify.cc

+41-32
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ using v8::ObjectTemplate;
3333
using v8::Persistent;
3434
using v8::PropertyAttribute;
3535
using v8::PropertyCallbackInfo;
36+
using v8::PropertyDescriptor;
3637
using v8::Script;
3738
using v8::ScriptCompiler;
3839
using v8::ScriptOrigin;
@@ -132,41 +133,49 @@ class ContextifyContext {
132133
return;
133134

134135
if (!has.FromJust()) {
135-
// Could also do this like so:
136-
//
137-
// PropertyAttribute att = global->GetPropertyAttributes(key_v);
138-
// Local<Value> val = global->Get(key_v);
139-
// sandbox->ForceSet(key_v, val, att);
140-
//
141-
// However, this doesn't handle ES6-style properties configured with
142-
// Object.defineProperty, and that's exactly what we're up against at
143-
// this point. ForceSet(key,val,att) only supports value properties
144-
// with the ES3-style attribute flags (DontDelete/DontEnum/ReadOnly),
145-
// which doesn't faithfully capture the full range of configurations
146-
// that can be done using Object.defineProperty.
147-
if (clone_property_method.IsEmpty()) {
148-
Local<String> code = FIXED_ONE_BYTE_STRING(env()->isolate(),
149-
"(function cloneProperty(source, key, target) {\n"
150-
" if (key === 'Proxy') return;\n"
151-
" try {\n"
152-
" var desc = Object.getOwnPropertyDescriptor(source, key);\n"
153-
" if (desc.value === source) desc.value = target;\n"
154-
" Object.defineProperty(target, key, desc);\n"
155-
" } catch (e) {\n"
156-
" // Catch sealed properties errors\n"
157-
" }\n"
158-
"})");
159-
160-
Local<Script> script =
161-
Script::Compile(context, code).ToLocalChecked();
162-
clone_property_method = Local<Function>::Cast(script->Run());
163-
CHECK(clone_property_method->IsFunction());
136+
Local<Object> desc_vm_context =
137+
global->GetOwnPropertyDescriptor(context, key)
138+
.ToLocalChecked().As<Object>();
139+
140+
bool is_accessor =
141+
desc_vm_context->Has(context, env()->get_string()).FromJust() ||
142+
desc_vm_context->Has(context, env()->set_string()).FromJust();
143+
144+
auto define_property_on_sandbox = [&] (PropertyDescriptor* desc) {
145+
desc->set_configurable(desc_vm_context
146+
->Get(context, env()->configurable_string()).ToLocalChecked()
147+
->BooleanValue(context).FromJust());
148+
desc->set_enumerable(desc_vm_context
149+
->Get(context, env()->enumerable_string()).ToLocalChecked()
150+
->BooleanValue(context).FromJust());
151+
sandbox_obj->DefineProperty(context, key, *desc);
152+
};
153+
154+
if (is_accessor) {
155+
Local<Function> get =
156+
desc_vm_context->Get(context, env()->get_string())
157+
.ToLocalChecked().As<Function>();
158+
Local<Function> set =
159+
desc_vm_context->Get(context, env()->set_string())
160+
.ToLocalChecked().As<Function>();
161+
162+
PropertyDescriptor desc(get, set);
163+
define_property_on_sandbox(&desc);
164+
} else {
165+
Local<Value> value =
166+
desc_vm_context->Get(context, env()->value_string())
167+
.ToLocalChecked();
168+
169+
bool writable =
170+
desc_vm_context->Get(context, env()->writable_string())
171+
.ToLocalChecked()->BooleanValue(context).FromJust();
172+
173+
PropertyDescriptor desc(value, writable);
174+
define_property_on_sandbox(&desc);
164175
}
165-
Local<Value> args[] = { global, key, sandbox_obj };
166-
clone_property_method->Call(global, arraysize(args), args);
167-
}
168176
}
169177
}
178+
}
170179

171180

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

0 commit comments

Comments
 (0)