node, async-wrap: remove MakeDomainCallback

C++ won't deoptimize like JS if specific conditional branches are
sporadically met in the future. Combined with the amount of code
duplication removal and simplified maintenance complexity, it makes more
sense to merge MakeCallback and MakeDomainCallback.

Additionally, type casting in V8 before verifying what that type is will
cause V8 to abort in debug mode if that type isn't what was expected.
Fix this by first checking the v8::Value before casting.

PR-URL: https://github.com/joyent/node/pull/8110
Signed-off-by: Trevor Norris <trev.norris@gmail.com>
Reviewed-by: Fedor Indutny <fedor@indutny.com>
Reviewed-by: Alexis Campailla <alexis@janeasystems.com>
Reviewed-by: Julien Gilli <julien.gilli@joyent.com>

diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h
index f3270c9837b81435609ed1b424f229c6833e0c14..9bd5744b16419740b5dcdadef09376f835ff6a5d 100644 (file)
--- a/src/async-wrap-inl.h
+++ b/src/async-wrap-inl.h
@@ -31,7 +31,6 @@
 #include "util-inl.h"
 
 #include "v8.h"
-#include <assert.h>
 
 namespace node {
 
@@ -46,6 +45,7 @@ inline AsyncWrap::AsyncWrap(Environment* env,
 inline AsyncWrap::~AsyncWrap() {
 }
 
+
 inline uint32_t AsyncWrap::provider_type() const {
   return provider_type_;
 }
@@ -56,10 +56,8 @@ inline v8::Handle<v8::Value> AsyncWrap::MakeCallback(
     int argc,
     v8::Handle<v8::Value>* argv) {
   v8::Local<v8::Value> cb_v = object()->Get(symbol);
-  v8::Local<v8::Function> cb = cb_v.As<v8::Function>();
-  assert(cb->IsFunction());
-
-  return MakeCallback(cb, argc, argv);
+  ASSERT(cb_v->IsFunction());
+  return MakeCallback(cb_v.As<v8::Function>(), argc, argv);
 }
 
 
@@ -68,10 +66,8 @@ inline v8::Handle<v8::Value> AsyncWrap::MakeCallback(
     int argc,
     v8::Handle<v8::Value>* argv) {
   v8::Local<v8::Value> cb_v = object()->Get(index);
-  v8::Local<v8::Function> cb = cb_v.As<v8::Function>();
-  assert(cb->IsFunction());
-
-  return MakeCallback(cb, argc, argv);
+  ASSERT(cb_v->IsFunction());
+  return MakeCallback(cb_v.As<v8::Function>(), argc, argv);
 }
 
 }  // namespace node

diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index 6f3f1e92123a49a00b3df09687290145ca903fee..66455a3046e196f7c4ba063b63ed1f0d0c290519 100644 (file)
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -37,30 +37,33 @@ using v8::Value;
 
 namespace node {
 
-Handle<Value> AsyncWrap::MakeDomainCallback(const Handle<Function> cb,
-                                            int argc,
-                                            Handle<Value>* argv) {
+Handle<Value> AsyncWrap::MakeCallback(const Handle<Function> cb,
+                                      int argc,
+                                      Handle<Value>* argv) {
   CHECK(env()->context() == env()->isolate()->GetCurrentContext());
 
   Local<Object> context = object();
   Local<Object> process = env()->process_object();
-  Local<Value> domain_v = context->Get(env()->domain_string());
   Local<Object> domain;
+  bool has_domain = false;
+
+  if (env()->using_domains()) {
+    Local<Value> domain_v = context->Get(env()->domain_string());
+    has_domain = domain_v->IsObject();
+    if (has_domain) {
+      domain = domain_v.As<Object>();
+      if (domain->Get(env()->disposed_string())->IsTrue())
+        return Undefined(env()->isolate());
+    }
+  }
 
   TryCatch try_catch;
   try_catch.SetVerbose(true);
 
-  bool has_domain = domain_v->IsObject();
   if (has_domain) {
-    domain = domain_v.As<Object>();
-
-    if (domain->Get(env()->disposed_string())->IsTrue())
-      return Undefined(env()->isolate());
-
-    Local<Function> enter =
-      domain->Get(env()->enter_string()).As<Function>();
-    if (enter->IsFunction()) {
-      enter->Call(domain, 0, NULL);
+    Local<Value> enter_v = domain->Get(env()->enter_string());
+    if (enter_v->IsFunction()) {
+      enter_v.As<Function>()->Call(domain, 0, NULL);
       if (try_catch.HasCaught())
         return Undefined(env()->isolate());
     }
@@ -73,10 +76,9 @@ Handle<Value> AsyncWrap::MakeDomainCallback(const Handle<Function> cb,
   }
 
   if (has_domain) {
-    Local<Function> exit =
-      domain->Get(env()->exit_string()).As<Function>();
-    if (exit->IsFunction()) {
-      exit->Call(domain, 0, NULL);
+    Local<Value> exit_v = domain->Get(env()->exit_string());
+    if (exit_v->IsFunction()) {
+      exit_v.As<Function>()->Call(domain, 0, NULL);
       if (try_catch.HasCaught())
         return Undefined(env()->isolate());
     }
@@ -111,54 +113,4 @@ Handle<Value> AsyncWrap::MakeDomainCallback(const Handle<Function> cb,
   return ret;
 }
 
-
-Handle<Value> AsyncWrap::MakeCallback(const Handle<Function> cb,
-                                      int argc,
-                                      Handle<Value>* argv) {
-  if (env()->using_domains())
-    return MakeDomainCallback(cb, argc, argv);
-
-  CHECK(env()->context() == env()->isolate()->GetCurrentContext());
-
-  Local<Object> context = object();
-  Local<Object> process = env()->process_object();
-
-  TryCatch try_catch;
-  try_catch.SetVerbose(true);
-
-  Local<Value> ret = cb->Call(context, argc, argv);
-
-  if (try_catch.HasCaught()) {
-    return Undefined(env()->isolate());
-  }
-
-  Environment::TickInfo* tick_info = env()->tick_info();
-
-  if (tick_info->in_tick()) {
-    return ret;
-  }
-
-  if (tick_info->length() == 0) {
-    env()->isolate()->RunMicrotasks();
-  }
-
-  if (tick_info->length() == 0) {
-    tick_info->set_index(0);
-    return ret;
-  }
-
-  tick_info->set_in_tick(true);
-
-  env()->tick_callback_function()->Call(process, 0, NULL);
-
-  tick_info->set_in_tick(false);
-
-  if (try_catch.HasCaught()) {
-    tick_info->set_last_threw(true);
-    return Undefined(env()->isolate());
-  }
-
-  return ret;
-}
-
 }  // namespace node

diff --git a/src/async-wrap.h b/src/async-wrap.h
index 1d30a3b1db45175812bc2c285c19096e86032d4f..b1f1fef3bded7f615777566a8c1b5c1998be4b1d 100644 (file)
--- a/src/async-wrap.h
+++ b/src/async-wrap.h
@@ -74,13 +74,6 @@ class AsyncWrap : public BaseObject {
  private:
   inline AsyncWrap();
 
-  // TODO(trevnorris): BURN IN FIRE! Remove this as soon as a suitable
-  // replacement is committed.
-  v8::Handle<v8::Value> MakeDomainCallback(
-      const v8::Handle<v8::Function> cb,
-      int argc,
-      v8::Handle<v8::Value>* argv);
-
   uint32_t provider_type_;
 };
 

diff --git a/src/node.cc b/src/node.cc
index df1c8eb1bf373817db3672dd480e6d06aa955a73..f4809a5147acd9130df73b8ef8b58251ddc0844b 100644 (file)
--- a/src/node.cc
+++ b/src/node.cc
@@ -978,111 +978,53 @@ void SetupNextTick(const FunctionCallbackInfo<Value>& args) {
 }
 
 
-Handle<Value> MakeDomainCallback(Environment* env,
-                                 Handle<Value> recv,
-                                 const Handle<Function> callback,
-                                 int argc,
-                                 Handle<Value> argv[]) {
+Handle<Value> MakeCallback(Environment* env,
+                           Handle<Value> recv,
+                           const Handle<Function> callback,
+                           int argc,
+                           Handle<Value> argv[]) {
   // If you hit this assertion, you forgot to enter the v8::Context first.
-  assert(env->context() == env->isolate()->GetCurrentContext());
+  CHECK(env->context() == env->isolate()->GetCurrentContext());
 
   Local<Object> process = env->process_object();
   Local<Object> object, domain;
-  Local<Value> domain_v;
-
-  TryCatch try_catch;
-  try_catch.SetVerbose(true);
-
   bool has_domain = false;
 
-  if (!object.IsEmpty()) {
-    domain_v = object->Get(env->domain_string());
+  if (env->using_domains()) {
+    CHECK(recv->IsObject());
+    object = recv.As<Object>();
+    Local<Value> domain_v = object->Get(env->domain_string());
     has_domain = domain_v->IsObject();
     if (has_domain) {
       domain = domain_v.As<Object>();
-
-      if (domain->Get(env->disposed_string())->IsTrue()) {
-        // domain has been disposed of.
+      if (domain->Get(env->disposed_string())->IsTrue())
         return Undefined(env->isolate());
-      }
-
-      Local<Function> enter = domain->Get(env->enter_string()).As<Function>();
-      if (enter->IsFunction()) {
-        enter->Call(domain, 0, NULL);
-        if (try_catch.HasCaught())
-          return Undefined(env->isolate());
-      }
     }
   }
 
-  Local<Value> ret = callback->Call(recv, argc, argv);
-
-  if (try_catch.HasCaught()) {
-    return Undefined(env->isolate());
-  }
+  TryCatch try_catch;
+  try_catch.SetVerbose(true);
 
   if (has_domain) {
-    Local<Function> exit = domain->Get(env->exit_string()).As<Function>();
-    if (exit->IsFunction()) {
-      exit->Call(domain, 0, NULL);
+    Local<Value> enter_v = domain->Get(env->enter_string());
+    if (enter_v->IsFunction()) {
+      enter_v.As<Function>()->Call(domain, 0, NULL);
       if (try_catch.HasCaught())
         return Undefined(env->isolate());
     }
   }
 
-  Environment::TickInfo* tick_info = env->tick_info();
-
-  if (tick_info->last_threw() == 1) {
-    tick_info->set_last_threw(0);
-    return ret;
-  }
-
-  if (tick_info->in_tick()) {
-    return ret;
-  }
-
-  if (tick_info->length() == 0) {
-    env->isolate()->RunMicrotasks();
-  }
-
-  if (tick_info->length() == 0) {
-    tick_info->set_index(0);
-    return ret;
-  }
-
-  tick_info->set_in_tick(true);
-
-  env->tick_callback_function()->Call(process, 0, NULL);
-
-  tick_info->set_in_tick(false);
+  Local<Value> ret = callback->Call(recv, argc, argv);
 
-  if (try_catch.HasCaught()) {
-    tick_info->set_last_threw(true);
-    return Undefined(env->isolate());
+  if (has_domain) {
+    Local<Value> exit_v = domain->Get(env->exit_string());
+    if (exit_v->IsFunction()) {
+      exit_v.As<Function>()->Call(domain, 0, NULL);
+      if (try_catch.HasCaught())
+        return Undefined(env->isolate());
+    }
   }
 
-  return ret;
-}
-
-
-Handle<Value> MakeCallback(Environment* env,
-                           Handle<Value> recv,
-                           const Handle<Function> callback,
-                           int argc,
-                           Handle<Value> argv[]) {
-  if (env->using_domains())
-    return MakeDomainCallback(env, recv, callback, argc, argv);
-
-  // If you hit this assertion, you forgot to enter the v8::Context first.
-  assert(env->context() == env->isolate()->GetCurrentContext());
-
-  Local<Object> process = env->process_object();
-
-  TryCatch try_catch;
-  try_catch.SetVerbose(true);
-
-  Local<Value> ret = callback->Call(recv, argc, argv);
-
   if (try_catch.HasCaught()) {
     return Undefined(env->isolate());
   }
@@ -1124,10 +1066,9 @@ Handle<Value> MakeCallback(Environment* env,
                            uint32_t index,
                            int argc,
                            Handle<Value> argv[]) {
-  Local<Function> callback = recv->Get(index).As<Function>();
-  assert(callback->IsFunction());
-
-  return MakeCallback(env, recv.As<Value>(), callback, argc, argv);
+  Local<Value> cb_v = recv->Get(index);
+  CHECK(cb_v->IsFunction());
+  return MakeCallback(env, recv.As<Value>(), cb_v.As<Function>(), argc, argv);
 }
 
 
@@ -1136,9 +1077,9 @@ Handle<Value> MakeCallback(Environment* env,
                            Handle<String> symbol,
                            int argc,
                            Handle<Value> argv[]) {
-  Local<Function> callback = recv->Get(symbol).As<Function>();
-  assert(callback->IsFunction());
-  return MakeCallback(env, recv.As<Value>(), callback, argc, argv);
+  Local<Value> cb_v = recv->Get(symbol);
+  CHECK(cb_v->IsFunction());
+  return MakeCallback(env, recv.As<Value>(), cb_v.As<Function>(), argc, argv);
 }
 
 
@@ -1195,20 +1136,6 @@ Handle<Value> MakeCallback(Isolate* isolate,
 }
 
 
-Handle<Value> MakeDomainCallback(Handle<Object> recv,
-                                 Handle<Function> callback,
-                                 int argc,
-                                 Handle<Value> argv[]) {
-  Local<Context> context = recv->CreationContext();
-  Environment* env = Environment::GetCurrent(context);
-  Context::Scope context_scope(context);
-  EscapableHandleScope handle_scope(env->isolate());
-  return handle_scope.Escape(Local<Value>::New(
-      env->isolate(),
-      MakeDomainCallback(env, recv, callback, argc, argv)));
-}
-
-
 enum encoding ParseEncoding(Isolate* isolate,
                             Handle<Value> encoding_v,
                             enum encoding _default) {