From 44b17a21ad331f59787f33edb7f09bc2e870ec60 Mon Sep 17 00:00:00 2001
From: AnnaMag <AnnaMag@users.noreply.github.com>
Date: Wed, 1 Feb 2017 13:00:40 +0000
Subject: [PATCH] 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: https://github.com/nodejs/node/pull/11102
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 src/env.h              |  5 +++
 src/node_contextify.cc | 73 ++++++++++++++++++++++++------------------
 2 files changed, 46 insertions(+), 32 deletions(-)

diff --git a/src/env.h b/src/env.h
index 8c256ca9c7faa4..581d7e9aef3c67 100644
--- a/src/env.h
+++ b/src/env.h
@@ -82,6 +82,7 @@ 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")                                              \
@@ -89,6 +90,7 @@ namespace node {
   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")                                              \
@@ -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")                                           \
@@ -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")                                                      \
@@ -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")                                                  \
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 5ed7d5faf0932e..4efb420fa128b7 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -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;
@@ -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