contextify: cleanup weak ref for sandbox

Simplify how node_contextify was keeping a weak reference to the
sandbox object in order to prepare for new style phantom weakness V8
API. It is simpler (and more robust) for the context to hold a
reference to the sandbox in an embedder data field. Doing otherwise
meant that the sandbox could become weak while the context was still
alive. This wasn't a problem because we would make the reference
strong at that point.

Since the sandbox must live at least as long as the context, it
would be better for the context to hold onto the sandbox.

PR-URL: #5392
Reviewed-By: Ben Noordhuis <[email protected]>

Author: ofrobots

src/node_contextify.cc

@@ -48,40 +48,29 @@ using v8::WeakCallbackData;
 
 class ContextifyContext {
  protected:
-  enum Kind {
-    kSandbox,
-    kContext
-  };
+  // V8 reserves the first field in context objects for the debugger. We use the
+  // second field to hold a reference to the sandbox object.
+  enum { kSandboxObjectIndex = 1 };
 
   Environment* const env_;
-  Persistent<Object> sandbox_;
   Persistent<Context> context_;
-  int references_;
 
  public:
-  explicit ContextifyContext(Environment* env, Local<Object> sandbox)
-      : env_(env),
-        sandbox_(env->isolate(), sandbox),
-        // Wait for sandbox_ and context_ to die
-        references_(0) {
-    context_.Reset(env->isolate(), CreateV8Context(env));
-
-    sandbox_.SetWeak(this, WeakCallback<Object, kSandbox>);
-    sandbox_.MarkIndependent();
-    references_++;
+  explicit ContextifyContext(Environment* env, Local<Object> sandbox_obj)
+      : env_(env) {
+    Local<Context> v8_context = CreateV8Context(env, sandbox_obj);
+    context_.Reset(env->isolate(), v8_context);
 
     // Allocation failure or maximum call stack size reached
     if (context_.IsEmpty())
       return;
-    context_.SetWeak(this, WeakCallback<Context, kContext>);
+    context_.SetWeak(this, WeakCallback<Context>);
     context_.MarkIndependent();
-    references_++;
   }
 
 
   ~ContextifyContext() {
     context_.Reset();
-    sandbox_.Reset();
   }
 
 
@@ -99,6 +88,11 @@ class ContextifyContext {
     return context()->Global();
   }
 
+
+  inline Local<Object> sandbox() const {
+    return Local<Object>::Cast(context()->GetEmbedderData(kSandboxObjectIndex));
+  }
+
   // XXX(isaacs): This function only exists because of a shortcoming of
   // the V8 SetNamedPropertyHandler function.
   //
@@ -126,15 +120,14 @@ class ContextifyContext {
     Local<Context> context = PersistentToLocal(env()->isolate(), context_);
     Local<Object> global =
         context->Global()->GetPrototype()->ToObject(env()->isolate());
-    Local<Object> sandbox = PersistentToLocal(env()->isolate(), sandbox_);
 
     Local<Function> clone_property_method;
 
     Local<Array> names = global->GetOwnPropertyNames();
     int length = names->Length();
     for (int i = 0; i < length; i++) {
       Local<String> key = names->Get(i)->ToString(env()->isolate());
-      bool has = sandbox->HasOwnProperty(key);
+      bool has = sandbox()->HasOwnProperty(context, key).FromJust();
       if (!has) {
         // Could also do this like so:
         //
@@ -167,7 +160,7 @@ class ContextifyContext {
           clone_property_method = Local<Function>::Cast(script->Run());
           CHECK(clone_property_method->IsFunction());
         }
-        Local<Value> args[] = { global, key, sandbox };
+        Local<Value> args[] = { global, key, sandbox() };
         clone_property_method->Call(global, ARRAY_SIZE(args), args);
       }
     }
@@ -191,14 +184,13 @@ class ContextifyContext {
   }
 
 
-  Local<Context> CreateV8Context(Environment* env) {
+  Local<Context> CreateV8Context(Environment* env, Local<Object> sandbox_obj) {
     EscapableHandleScope scope(env->isolate());
     Local<FunctionTemplate> function_template =
         FunctionTemplate::New(env->isolate());
     function_template->SetHiddenPrototype(true);
 
-    Local<Object> sandbox = PersistentToLocal(env->isolate(), sandbox_);
-    function_template->SetClassName(sandbox->GetConstructorName());
+    function_template->SetClassName(sandbox_obj->GetConstructorName());
 
     Local<ObjectTemplate> object_template =
         function_template->InstanceTemplate();
@@ -215,6 +207,7 @@ class ContextifyContext {
 
     CHECK(!ctx.IsEmpty());
     ctx->SetSecurityToken(env->context()->GetSecurityToken());
+    ctx->SetEmbedderData(kSandboxObjectIndex, sandbox_obj);
 
     env->AssignToContext(ctx);
 
@@ -309,16 +302,11 @@ class ContextifyContext {
   }
 
 
-  template <class T, Kind kind>
+  template <class T>
   static void WeakCallback(const WeakCallbackData<T, ContextifyContext>& data) {
     ContextifyContext* context = data.GetParameter();
-    if (kind == kSandbox)
-      context->sandbox_.ClearWeak();
-    else
-      context->context_.ClearWeak();
-
-    if (--context->references_ == 0)
-      delete context;
+    context->context_.ClearWeak();
+    delete context;
   }
 
 
@@ -340,26 +328,23 @@ class ContextifyContext {
   static void GlobalPropertyGetterCallback(
       Local<Name> property,
       const PropertyCallbackInfo<Value>& args) {
-    Isolate* isolate = args.GetIsolate();
-
     ContextifyContext* ctx =
         Unwrap<ContextifyContext>(args.Data().As<Object>());
 
     // Stil initializing
     if (ctx->context_.IsEmpty())
       return;
 
-    Local<Object> sandbox = PersistentToLocal(isolate, ctx->sandbox_);
     MaybeLocal<Value> maybe_rv =
-        sandbox->GetRealNamedProperty(ctx->context(), property);
+        ctx->sandbox()->GetRealNamedProperty(ctx->context(), property);
     if (maybe_rv.IsEmpty()) {
       maybe_rv =
           ctx->global_proxy()->GetRealNamedProperty(ctx->context(), property);
     }
 
     Local<Value> rv;
     if (maybe_rv.ToLocal(&rv)) {
-      if (rv == ctx->sandbox_)
+      if (rv == ctx->sandbox())
         rv = ctx->global_proxy();
 
       args.GetReturnValue().Set(rv);
@@ -371,34 +356,30 @@ class ContextifyContext {
       Local<Name> property,
       Local<Value> value,
       const PropertyCallbackInfo<Value>& args) {
-    Isolate* isolate = args.GetIsolate();
-
     ContextifyContext* ctx =
         Unwrap<ContextifyContext>(args.Data().As<Object>());
 
     // Stil initializing
     if (ctx->context_.IsEmpty())
       return;
 
-    PersistentToLocal(isolate, ctx->sandbox_)->Set(property, value);
+    ctx->sandbox()->Set(property, value);
   }
 
 
   static void GlobalPropertyQueryCallback(
       Local<Name> property,
       const PropertyCallbackInfo<Integer>& args) {
-    Isolate* isolate = args.GetIsolate();
-
     ContextifyContext* ctx =
         Unwrap<ContextifyContext>(args.Data().As<Object>());
 
     // Stil initializing
     if (ctx->context_.IsEmpty())
       return;
 
-    Local<Object> sandbox = PersistentToLocal(isolate, ctx->sandbox_);
     Maybe<PropertyAttribute> maybe_prop_attr =
-        sandbox->GetRealNamedPropertyAttributes(ctx->context(), property);
+        ctx->sandbox()->GetRealNamedPropertyAttributes(ctx->context(),
+                                                       property);
 
     if (maybe_prop_attr.IsNothing()) {
       maybe_prop_attr =
@@ -416,18 +397,14 @@ class ContextifyContext {
   static void GlobalPropertyDeleterCallback(
       Local<Name> property,
       const PropertyCallbackInfo<Boolean>& args) {
-    Isolate* isolate = args.GetIsolate();
-
     ContextifyContext* ctx =
         Unwrap<ContextifyContext>(args.Data().As<Object>());
 
     // Stil initializing
     if (ctx->context_.IsEmpty())
       return;
 
-    Local<Object> sandbox = PersistentToLocal(isolate, ctx->sandbox_);
-
-    Maybe<bool> success = sandbox->Delete(ctx->context(), property);
+    Maybe<bool> success = ctx->sandbox()->Delete(ctx->context(), property);
 
     if (success.IsJust())
       args.GetReturnValue().Set(success.FromJust());
@@ -443,8 +420,7 @@ class ContextifyContext {
     if (ctx->context_.IsEmpty())
       return;
 
-    Local<Object> sandbox = PersistentToLocal(args.GetIsolate(), ctx->sandbox_);
-    args.GetReturnValue().Set(sandbox->GetPropertyNames());
+    args.GetReturnValue().Set(ctx->sandbox()->GetPropertyNames());
   }
 };