src: do not add .domain to promises in VM contexts

The promises are still tracked, and their handlers will still execute in the correct domain. The creation domain is simply hidden. Backport-PR-URL: #16074 PR-URL: #15695 Fixes: #15673 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
nodejs · Oct 23, 2017 · 0b04869 · 0b04869
1 parent b1a68f5
commit 0b04869
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 9 deletions.
diff --git a/doc/api/domain.md b/doc/api/domain.md
@@ -1,6 +1,12 @@
 # Domain
 <!-- YAML
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/REPLACEME
+    description: Any `Promise`s created in VM contexts no longer have a
+                 `.domain` property. Their handlers are still executed in the
+                 proper domain, however, and `Promise`s created in the main
+                 context still possess a `.domain` property.
   - version: v8.0.0
     pr-url: https://github.com/nodejs/node/pull/12489
     description: Handlers for `Promise`s are now invoked in the domain in which

diff --git a/src/env.h b/src/env.h
@@ -93,6 +93,7 @@ class ModuleWrap;
   V(npn_buffer_private_symbol, "node:npnBuffer")                              \
   V(processed_private_symbol, "node:processed")                               \
   V(selected_npn_buffer_private_symbol, "node:selectedNpnBuffer")             \
+  V(domain_private_symbol, "node:domain")                                     \
 
 // Strings are per-isolate primitives but Environment proxies them
 // for the sake of convenience.  Strings should be ASCII-only.

diff --git a/src/node.cc b/src/node.cc
@@ -1161,8 +1161,19 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) {
 }
 
 
+Local<Value> GetDomainProperty(Environment* env, Local<Object> object) {
+  Local<Value> domain_v =
+      object->GetPrivate(env->context(), env->domain_private_symbol())
+          .ToLocalChecked();
+  if (domain_v->IsObject()) {
+    return domain_v;
+  }
+  return object->Get(env->context(), env->domain_string()).ToLocalChecked();
+}
+
+
 bool DomainEnter(Environment* env, Local<Object> object) {
-  Local<Value> domain_v = object->Get(env->domain_string());
+  Local<Value> domain_v = GetDomainProperty(env, object);
   if (domain_v->IsObject()) {
     Local<Object> domain = domain_v.As<Object>();
     if (domain->Get(env->disposed_string())->IsTrue())
@@ -1180,7 +1191,7 @@ bool DomainEnter(Environment* env, Local<Object> object) {
 
 
 bool DomainExit(Environment* env, v8::Local<v8::Object> object) {
-  Local<Value> domain_v = object->Get(env->domain_string());
+  Local<Value> domain_v = GetDomainProperty(env, object);
   if (domain_v->IsObject()) {
     Local<Object> domain = domain_v.As<Object>();
     if (domain->Get(env->disposed_string())->IsTrue())
@@ -1205,10 +1216,16 @@ void DomainPromiseHook(PromiseHookType type,
   Local<Context> context = env->context();
 
   if (type == PromiseHookType::kInit && env->in_domain()) {
-    promise->Set(context,
-                 env->domain_string(),
-                 env->domain_array()->Get(context,
-                                          0).ToLocalChecked()).FromJust();
+    Local<Value> domain_obj =
+        env->domain_array()->Get(context, 0).ToLocalChecked();
+    if (promise->CreationContext() == context) {
+      promise->Set(context, env->domain_string(), domain_obj).FromJust();
+    } else {
+      // Do not expose object from another context publicly in promises created
+      // in non-main contexts.
+      promise->SetPrivate(context, env->domain_private_symbol(), domain_obj)
+          .FromJust();
+    }
     return;
   }
 

diff --git a/test/parallel/test-domain-promise.js b/test/parallel/test-domain-promise.js
@@ -31,9 +31,13 @@ common.crashOnUnhandledRejection();
   const d = domain.create();
 
   d.run(common.mustCall(() => {
-    vm.runInNewContext(`Promise.resolve().then(common.mustCall(() => {
-      assert.strictEqual(process.domain, d);
-    }));`, { common, assert, process, d });
+    vm.runInNewContext(`
+      const promise = Promise.resolve();
+      assert.strictEqual(promise.domain, undefined);
+      promise.then(common.mustCall(() => {
+        assert.strictEqual(process.domain, d);
+      }));
+    `, { common, assert, process, d });
   }));
 }