From 820a1aae045134e38c622fa58f505f15317aac1c Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Mon, 22 Feb 2016 20:02:00 -0800 Subject: [PATCH 1/4] contextify: cleanup weak ref for global proxy Cleanup how node_contextify keeps weak references in order to prepare for new style phantom weakness API. We didn't need to keep a weak reference to the context's global proxy, as the context holds it. --- src/node_contextify.cc | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 342b46b3517a79..cffb7aa96ff1eb 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -52,21 +52,19 @@ class ContextifyContext { protected: enum Kind { kSandbox, - kContext, - kProxyGlobal + kContext }; Environment* const env_; Persistent sandbox_; Persistent context_; - Persistent proxy_global_; int references_; public: explicit ContextifyContext(Environment* env, Local sandbox) : env_(env), sandbox_(env->isolate(), sandbox), - // Wait for sandbox_, proxy_global_, and context_ to die + // Wait for sandbox_ and context_ to die references_(0) { context_.Reset(env->isolate(), CreateV8Context(env)); @@ -80,17 +78,11 @@ class ContextifyContext { context_.SetWeak(this, WeakCallback); context_.MarkIndependent(); references_++; - - proxy_global_.Reset(env->isolate(), context()->Global()); - proxy_global_.SetWeak(this, WeakCallback); - proxy_global_.MarkIndependent(); - references_++; } ~ContextifyContext() { context_.Reset(); - proxy_global_.Reset(); sandbox_.Reset(); } @@ -105,6 +97,10 @@ class ContextifyContext { } + inline Local global_proxy() const { + return context()->Global(); + } + // XXX(isaacs): This function only exists because of a shortcoming of // the V8 SetNamedPropertyHandler function. // @@ -321,10 +317,8 @@ class ContextifyContext { ContextifyContext* context = data.GetParameter(); if (kind == kSandbox) context->sandbox_.ClearWeak(); - else if (kind == kContext) - context->context_.ClearWeak(); else - context->proxy_global_.ClearWeak(); + context->context_.ClearWeak(); if (--context->references_ == 0) delete context; @@ -362,15 +356,14 @@ class ContextifyContext { MaybeLocal maybe_rv = sandbox->GetRealNamedProperty(ctx->context(), property); if (maybe_rv.IsEmpty()) { - Local proxy_global = PersistentToLocal(isolate, - ctx->proxy_global_); - maybe_rv = proxy_global->GetRealNamedProperty(ctx->context(), property); + maybe_rv = + ctx->global_proxy()->GetRealNamedProperty(ctx->context(), property); } Local rv; if (maybe_rv.ToLocal(&rv)) { if (rv == ctx->sandbox_) - rv = PersistentToLocal(isolate, ctx->proxy_global_); + rv = ctx->global_proxy(); args.GetReturnValue().Set(rv); } @@ -411,11 +404,8 @@ class ContextifyContext { sandbox->GetRealNamedPropertyAttributes(ctx->context(), property); if (maybe_prop_attr.IsNothing()) { - Local proxy_global = PersistentToLocal(isolate, - ctx->proxy_global_); - maybe_prop_attr = - proxy_global->GetRealNamedPropertyAttributes(ctx->context(), + ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(), property); } From 6edaab5913f719f4b5d8d8481ac2a6a0df7e5763 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Mon, 22 Feb 2016 20:38:56 -0800 Subject: [PATCH 2/4] 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. --- 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 cffb7aa96ff1eb..82b14e468281cc 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -50,40 +50,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(); } @@ -101,6 +90,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. // @@ -128,7 +122,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; @@ -136,7 +129,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(context, key).FromJust(); + bool has = sandbox()->HasOwnProperty(context, key).FromJust(); if (!has) { // Could also do this like so: // @@ -168,7 +161,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); } } @@ -193,14 +186,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(); @@ -217,6 +209,7 @@ class ContextifyContext { CHECK(!ctx.IsEmpty()); ctx->SetSecurityToken(env->context()->GetSecurityToken()); + ctx->SetEmbedderData(kSandboxObjectIndex, sandbox_obj); env->AssignToContext(ctx); @@ -312,16 +305,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; } @@ -343,8 +331,6 @@ class ContextifyContext { static void GlobalPropertyGetterCallback( Local property, const PropertyCallbackInfo& args) { - Isolate* isolate = args.GetIsolate(); - ContextifyContext* ctx = Unwrap(args.Data().As()); @@ -352,9 +338,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); @@ -362,7 +347,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); @@ -374,8 +359,6 @@ class ContextifyContext { Local property, Local value, const PropertyCallbackInfo& args) { - Isolate* isolate = args.GetIsolate(); - ContextifyContext* ctx = Unwrap(args.Data().As()); @@ -383,15 +366,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()); @@ -399,9 +380,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 = @@ -419,8 +400,6 @@ class ContextifyContext { static void GlobalPropertyDeleterCallback( Local property, const PropertyCallbackInfo& args) { - Isolate* isolate = args.GetIsolate(); - ContextifyContext* ctx = Unwrap(args.Data().As()); @@ -428,9 +407,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()); @@ -446,8 +423,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()); } }; From 9f2d84ae318222800012399199c765b8f115cd8a Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Mon, 22 Feb 2016 20:57:50 -0800 Subject: [PATCH 3/4] contextify: replace deprecated SetWeak usage --- src/node_contextify.cc | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 82b14e468281cc..33f5e5b9a06ba9 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -45,7 +45,7 @@ using v8::Uint8Array; using v8::UnboundScript; using v8::V8; using v8::Value; -using v8::WeakCallbackData; +using v8::WeakCallbackInfo; class ContextifyContext { @@ -66,7 +66,7 @@ class ContextifyContext { // Allocation failure or maximum call stack size reached if (context_.IsEmpty()) return; - context_.SetWeak(this, WeakCallback); + context_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); context_.MarkIndependent(); } @@ -305,10 +305,8 @@ class ContextifyContext { } - template - static void WeakCallback(const WeakCallbackData& data) { + static void WeakCallback(const WeakCallbackInfo& data) { ContextifyContext* context = data.GetParameter(); - context->context_.ClearWeak(); delete context; } From fd21188f95e19c5ecc2c8cd4348fba0efb270584 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Wed, 24 Feb 2016 09:52:08 -0800 Subject: [PATCH 4/4] contextify: cache sandbox and context in locals --- src/node_contextify.cc | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 33f5e5b9a06ba9..5a7bf41437ab0c 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -58,8 +58,7 @@ class ContextifyContext { Persistent context_; public: - explicit ContextifyContext(Environment* env, Local sandbox_obj) - : env_(env) { + ContextifyContext(Environment* env, Local sandbox_obj) : env_(env) { Local v8_context = CreateV8Context(env, sandbox_obj); context_.Reset(env->isolate(), v8_context); @@ -122,6 +121,7 @@ class ContextifyContext { Local context = PersistentToLocal(env()->isolate(), context_); Local global = context->Global()->GetPrototype()->ToObject(env()->isolate()); + Local sandbox_obj = sandbox(); Local clone_property_method; @@ -129,7 +129,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(context, key).FromJust(); + bool has = sandbox_obj->HasOwnProperty(context, key).FromJust(); if (!has) { // Could also do this like so: // @@ -161,7 +161,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_obj }; clone_property_method->Call(global, ARRAY_SIZE(args), args); } } @@ -336,16 +336,18 @@ class ContextifyContext { if (ctx->context_.IsEmpty()) return; + Local context = ctx->context(); + Local sandbox = ctx->sandbox(); MaybeLocal maybe_rv = - ctx->sandbox()->GetRealNamedProperty(ctx->context(), property); + sandbox->GetRealNamedProperty(context, property); if (maybe_rv.IsEmpty()) { maybe_rv = - ctx->global_proxy()->GetRealNamedProperty(ctx->context(), property); + ctx->global_proxy()->GetRealNamedProperty(context, property); } Local rv; if (maybe_rv.ToLocal(&rv)) { - if (rv == ctx->sandbox()) + if (rv == sandbox) rv = ctx->global_proxy(); args.GetReturnValue().Set(rv); @@ -378,14 +380,14 @@ class ContextifyContext { if (ctx->context_.IsEmpty()) return; + Local context = ctx->context(); Maybe maybe_prop_attr = - ctx->sandbox()->GetRealNamedPropertyAttributes(ctx->context(), - property); + ctx->sandbox()->GetRealNamedPropertyAttributes(context, property); if (maybe_prop_attr.IsNothing()) { maybe_prop_attr = - ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(), - property); + ctx->global_proxy()->GetRealNamedPropertyAttributes(context, + property); } if (maybe_prop_attr.IsJust()) {