From 3d56671b5671445b6e97e364bde39cede1a131e5 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Mon, 22 Feb 2016 20:38:56 -0800 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/5392 Reviewed-By: Ben Noordhuis --- src/node_contextify.cc | 80 +++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 52 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 9e70bb01080c91..397d1a31b10c5a 100644 --- a/src/node_contextify.cc +++ b/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 sandbox_; Persistent context_; - int references_; public: - explicit ContextifyContext(Environment* env, Local 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); - sandbox_.MarkIndependent(); - references_++; + explicit ContextifyContext(Environment* env, Local sandbox_obj) + : env_(env) { + Local 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_.SetWeak(this, WeakCallback); context_.MarkIndependent(); - references_++; } ~ContextifyContext() { context_.Reset(); - sandbox_.Reset(); } @@ -99,6 +88,11 @@ class ContextifyContext { return context()->Global(); } + + inline Local sandbox() const { + return Local::Cast(context()->GetEmbedderData(kSandboxObjectIndex)); + } + // XXX(isaacs): This function only exists because of a shortcoming of // the V8 SetNamedPropertyHandler function. // @@ -126,7 +120,6 @@ class ContextifyContext { Local context = PersistentToLocal(env()->isolate(), context_); Local global = context->Global()->GetPrototype()->ToObject(env()->isolate()); - Local sandbox = PersistentToLocal(env()->isolate(), sandbox_); Local clone_property_method; @@ -134,7 +127,7 @@ class ContextifyContext { int length = names->Length(); for (int i = 0; i < length; i++) { Local 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::Cast(script->Run()); CHECK(clone_property_method->IsFunction()); } - Local args[] = { global, key, sandbox }; + Local args[] = { global, key, sandbox() }; clone_property_method->Call(global, ARRAY_SIZE(args), args); } } @@ -191,14 +184,13 @@ class ContextifyContext { } - Local CreateV8Context(Environment* env) { + Local CreateV8Context(Environment* env, Local sandbox_obj) { EscapableHandleScope scope(env->isolate()); Local function_template = FunctionTemplate::New(env->isolate()); function_template->SetHiddenPrototype(true); - Local sandbox = PersistentToLocal(env->isolate(), sandbox_); - function_template->SetClassName(sandbox->GetConstructorName()); + function_template->SetClassName(sandbox_obj->GetConstructorName()); Local 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 + template static void WeakCallback(const WeakCallbackData& 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,8 +328,6 @@ class ContextifyContext { static void GlobalPropertyGetterCallback( Local property, const PropertyCallbackInfo& args) { - Isolate* isolate = args.GetIsolate(); - ContextifyContext* ctx = Unwrap(args.Data().As()); @@ -349,9 +335,8 @@ class ContextifyContext { if (ctx->context_.IsEmpty()) return; - Local sandbox = PersistentToLocal(isolate, ctx->sandbox_); MaybeLocal 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); @@ -359,7 +344,7 @@ class ContextifyContext { Local rv; if (maybe_rv.ToLocal(&rv)) { - if (rv == ctx->sandbox_) + if (rv == ctx->sandbox()) rv = ctx->global_proxy(); args.GetReturnValue().Set(rv); @@ -371,8 +356,6 @@ class ContextifyContext { Local property, Local value, const PropertyCallbackInfo& args) { - Isolate* isolate = args.GetIsolate(); - ContextifyContext* ctx = Unwrap(args.Data().As()); @@ -380,15 +363,13 @@ class ContextifyContext { if (ctx->context_.IsEmpty()) return; - PersistentToLocal(isolate, ctx->sandbox_)->Set(property, value); + ctx->sandbox()->Set(property, value); } static void GlobalPropertyQueryCallback( Local property, const PropertyCallbackInfo& args) { - Isolate* isolate = args.GetIsolate(); - ContextifyContext* ctx = Unwrap(args.Data().As()); @@ -396,9 +377,9 @@ class ContextifyContext { if (ctx->context_.IsEmpty()) return; - Local sandbox = PersistentToLocal(isolate, ctx->sandbox_); Maybe maybe_prop_attr = - sandbox->GetRealNamedPropertyAttributes(ctx->context(), property); + ctx->sandbox()->GetRealNamedPropertyAttributes(ctx->context(), + property); if (maybe_prop_attr.IsNothing()) { maybe_prop_attr = @@ -416,8 +397,6 @@ class ContextifyContext { static void GlobalPropertyDeleterCallback( Local property, const PropertyCallbackInfo& args) { - Isolate* isolate = args.GetIsolate(); - ContextifyContext* ctx = Unwrap(args.Data().As()); @@ -425,9 +404,7 @@ class ContextifyContext { if (ctx->context_.IsEmpty()) return; - Local sandbox = PersistentToLocal(isolate, ctx->sandbox_); - - Maybe success = sandbox->Delete(ctx->context(), property); + Maybe 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 sandbox = PersistentToLocal(args.GetIsolate(), ctx->sandbox_); - args.GetReturnValue().Set(sandbox->GetPropertyNames()); + args.GetReturnValue().Set(ctx->sandbox()->GetPropertyNames()); } };