From faa94ff2b5fa106953c08c16bf6f8fe208771e11 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Mon, 22 Feb 2016 20:02:00 -0800 Subject: [PATCH 1/5] 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. PR-URL: /~https://github.com/nodejs/node/pull/5392 Reviewed-By: Ben Noordhuis --- 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 59a90c88c9993f..9e70bb01080c91 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -50,21 +50,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)); @@ -78,17 +76,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(); } @@ -103,6 +95,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. // @@ -318,10 +314,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; @@ -359,15 +353,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); } @@ -408,11 +401,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 3d56671b5671445b6e97e364bde39cede1a131e5 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Mon, 22 Feb 2016 20:38:56 -0800 Subject: [PATCH 2/5] 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()); } }; From ab00cf599096e33b133c48fc7c8142774cc6352c Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Mon, 22 Feb 2016 20:57:50 -0800 Subject: [PATCH 3/5] contextify: replace deprecated SetWeak usage PR-URL: /~https://github.com/nodejs/node/pull/5392 Reviewed-By: Ben Noordhuis --- 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 397d1a31b10c5a..17a10119f1de8b 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -43,7 +43,7 @@ using v8::TryCatch; using v8::UnboundScript; using v8::V8; using v8::Value; -using v8::WeakCallbackData; +using v8::WeakCallbackInfo; class ContextifyContext { @@ -64,7 +64,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(); } @@ -302,10 +302,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 4d484008816f91c665373d2d471118763c12728d Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Wed, 24 Feb 2016 09:52:08 -0800 Subject: [PATCH 4/5] contextify: cache sandbox and context in locals PR-URL: /~https://github.com/nodejs/node/pull/5392 Reviewed-By: Ben Noordhuis --- 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 17a10119f1de8b..16019a0ecb3002 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -56,8 +56,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); @@ -120,6 +119,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; @@ -127,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(context, key).FromJust(); + bool has = sandbox_obj->HasOwnProperty(context, key).FromJust(); if (!has) { // Could also do this like so: // @@ -160,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_obj }; clone_property_method->Call(global, ARRAY_SIZE(args), args); } } @@ -333,16 +333,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); @@ -375,14 +377,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()) { From c6db82241e527fc7992a5a79b404cb4a86eac126 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Fri, 18 Mar 2016 11:32:44 -0700 Subject: [PATCH 5/5] contextify: tie lifetimes of context & sandbox When the previous set of changes (bfff07b) it was possible to have the context get garbage collected while sandbox was still live. We need to tie the lifetime of the context to the lifetime of the sandbox. This is a backport of #5786 to v5.x. Ref: /~https://github.com/nodejs/node/pull/5786 PR-URL: /~https://github.com/nodejs/node/pull/5800 Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- src/node_contextify.cc | 10 ++++++++++ test/parallel/test-vm-create-and-run-in-context.js | 9 +++++++++ 2 files changed, 19 insertions(+) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 16019a0ecb3002..8235dcd49e093c 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -207,7 +207,17 @@ class ContextifyContext { CHECK(!ctx.IsEmpty()); ctx->SetSecurityToken(env->context()->GetSecurityToken()); + + // We need to tie the lifetime of the sandbox object with the lifetime of + // newly created context. We do this by making them hold references to each + // other. The context can directly hold a reference to the sandbox as an + // embedder data field. However, we cannot hold a reference to a v8::Context + // directly in an Object, we instead hold onto the new context's global + // object instead (which then has a reference to the context). ctx->SetEmbedderData(kSandboxObjectIndex, sandbox_obj); + sandbox_obj->SetHiddenValue( + FIXED_ONE_BYTE_STRING(env->isolate(), "_contextifyHiddenGlobal"), + ctx->Global()); env->AssignToContext(ctx); diff --git a/test/parallel/test-vm-create-and-run-in-context.js b/test/parallel/test-vm-create-and-run-in-context.js index 94527598ca27b1..15efc8f527b8c9 100644 --- a/test/parallel/test-vm-create-and-run-in-context.js +++ b/test/parallel/test-vm-create-and-run-in-context.js @@ -1,4 +1,5 @@ 'use strict'; +// Flags: --expose-gc require('../common'); var assert = require('assert'); @@ -18,3 +19,11 @@ console.error('test updating context'); result = vm.runInContext('var foo = 3;', context); assert.equal(3, context.foo); assert.equal('lala', context.thing); + +// /~https://github.com/nodejs/node/issues/5768 +console.error('run in contextified sandbox without referencing the context'); +var sandbox = {x: 1}; +vm.createContext(sandbox); +gc(); +vm.runInContext('x = 2', sandbox); +// Should not crash.