src,test,tools: forbid usage of v8::Persistent

doc/api/addons.md

@@ -157,11 +157,11 @@ The context-aware addon can be structured to avoid global static data by
 performing the following steps:
 
 * defining a class which will hold per-addon-instance data. Such
-a class should include a `v8::Persistent<v8::Object>` which will hold a weak
+a class should include a `v8::Global<v8::Object>` which will hold a weak
 reference to the addon's `exports` object. The callback associated with the weak
 reference will then destroy the instance of the class.
 * constructing an instance of this class in the addon initializer such that the
-`v8::Persistent<v8::Object>` is set to the `exports` object.
+`v8::Global<v8::Object>` is set to the `exports` object.
 * storing the instance of the class in a `v8::External`, and
 * passing the `v8::External` to all methods exposed to JavaScript by passing it
 to the `v8::FunctionTemplate` constructor which creates the native-backed
@@ -188,14 +188,6 @@ class AddonData {
     exports_.SetWeak(this, DeleteMe, WeakCallbackType::kParameter);
   }
 
-  ~AddonData() {
-    if (!exports_.IsEmpty()) {
-      // Reset the reference to avoid leaking data.
-      exports_.ClearWeak();
-      exports_.Reset();
-    }
-  }
-
   // Per-addon data.
   int call_count;
 
@@ -207,7 +199,7 @@ class AddonData {
 
   // Weak handle to the "exports" object. An instance of this class will be
   // destroyed along with the exports object to which it is weakly bound.
-  v8::Persistent<v8::Object> exports_;
+  v8::Global<v8::Object> exports_;
 };
 
 static void Method(const v8::FunctionCallbackInfo<v8::Value>& info) {
@@ -830,7 +822,7 @@ class MyObject : public node::ObjectWrap {
 
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void PlusOne(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static v8::Persistent<v8::Function> constructor;
+
   double value_;
 };
 
@@ -858,12 +850,10 @@ using v8::Local;
 using v8::NewStringType;
 using v8::Number;
 using v8::Object;
-using v8::Persistent;
+using v8::ObjectTemplate;
 using v8::String;
 using v8::Value;
 
-Persistent<Function> MyObject::constructor;
-
 MyObject::MyObject(double value) : value_(value) {
 }
 
@@ -872,21 +862,27 @@ MyObject::~MyObject() {
 
 void MyObject::Init(Local<Object> exports) {
   Isolate* isolate = exports->GetIsolate();
+  Local<Context> context = isolate->GetCurrentContext();
+
+  Local<ObjectTemplate> addon_data_tpl = ObjectTemplate::New(isolate);
+  addon_data_tpl->SetInternalFieldCount(1);  // 1 field for the MyObject::New()
+  Local<Object> addon_data =
+      addon_data_tpl->NewInstance(context).ToLocalChecked();
 
   // Prepare constructor template
-  Local<FunctionTemplate> tpl = FunctionTemplate::New(isolate, New);
+  Local<FunctionTemplate> tpl = FunctionTemplate::New(isolate, New, addon_data);
   tpl->SetClassName(String::NewFromUtf8(
       isolate, "MyObject", NewStringType::kNormal).ToLocalChecked());
   tpl->InstanceTemplate()->SetInternalFieldCount(1);
 
   // Prototype
   NODE_SET_PROTOTYPE_METHOD(tpl, "plusOne", PlusOne);
 
-  Local<Context> context = isolate->GetCurrentContext();
-  constructor.Reset(isolate, tpl->GetFunction(context).ToLocalChecked());
+  Local<Function> constructor = tpl->GetFunction(context).ToLocalChecked();
+  addon_data->SetInternalField(0, constructor);
   exports->Set(context, String::NewFromUtf8(
       isolate, "MyObject", NewStringType::kNormal).ToLocalChecked(),
-               tpl->GetFunction(context).ToLocalChecked()).FromJust();
+               constructor).FromJust();
 }
 
 void MyObject::New(const FunctionCallbackInfo<Value>& args) {
@@ -904,7 +900,8 @@ void MyObject::New(const FunctionCallbackInfo<Value>& args) {
     // Invoked as plain function `MyObject(...)`, turn into construct call.
     const int argc = 1;
     Local<Value> argv[argc] = { args[0] };
-    Local<Function> cons = Local<Function>::New(isolate, constructor);
+    Local<Function> cons =
+        args.Data().As<Object>()->GetInternalField(0).As<Function>();
     Local<Object> result =
         cons->NewInstance(context, argc, argv).ToLocalChecked();
     args.GetReturnValue().Set(result);
@@ -1029,7 +1026,7 @@ class MyObject : public node::ObjectWrap {
 
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void PlusOne(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static v8::Persistent<v8::Function> constructor;
+  static v8::Global<v8::Function> constructor;
   double value_;
 };
 
@@ -1047,20 +1044,23 @@ The implementation in `myobject.cc` is similar to the previous example:
 
 namespace demo {
 
+using node::AddEnvironmentCleanupHook;
 using v8::Context;
 using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
+using v8::Global;
 using v8::Isolate;
 using v8::Local;
 using v8::NewStringType;
 using v8::Number;
 using v8::Object;
-using v8::Persistent;
 using v8::String;
 using v8::Value;
 
-Persistent<Function> MyObject::constructor;
+// Warning! This is not thread-safe, this addon cannot be used for worker
+// threads.
+Global<Function> MyObject::constructor;
 
 MyObject::MyObject(double value) : value_(value) {
 }
@@ -1080,6 +1080,10 @@ void MyObject::Init(Isolate* isolate) {
 
   Local<Context> context = isolate->GetCurrentContext();
   constructor.Reset(isolate, tpl->GetFunction(context).ToLocalChecked());
+
+  AddEnvironmentCleanupHook(isolate, [](void*) {
+    constructor.Reset();
+  }, nullptr);
 }
 
 void MyObject::New(const FunctionCallbackInfo<Value>& args) {
@@ -1246,7 +1250,7 @@ class MyObject : public node::ObjectWrap {
   ~MyObject();
 
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static v8::Persistent<v8::Function> constructor;
+  static v8::Global<v8::Function> constructor;
   double value_;
 };
 
@@ -1264,19 +1268,22 @@ The implementation of `myobject.cc` is similar to before:
 
 namespace demo {
 
+using node::AddEnvironmentCleanupHook;
 using v8::Context;
 using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
+using v8::Global;
 using v8::Isolate;
 using v8::Local;
 using v8::NewStringType;
 using v8::Object;
-using v8::Persistent;
 using v8::String;
 using v8::Value;
 
-Persistent<Function> MyObject::constructor;
+// Warning! This is not thread-safe, this addon cannot be used for worker
+// threads.
+Global<Function> MyObject::constructor;
 
 MyObject::MyObject(double value) : value_(value) {
 }
@@ -1293,6 +1300,10 @@ void MyObject::Init(Isolate* isolate) {
 
   Local<Context> context = isolate->GetCurrentContext();
   constructor.Reset(isolate, tpl->GetFunction(context).ToLocalChecked());
+
+  AddEnvironmentCleanupHook(isolate, [](void*) {
+    constructor.Reset();
+  }, nullptr);
 }
 
 void MyObject::New(const FunctionCallbackInfo<Value>& args) {

src/api/async_resource.cc

@@ -24,7 +24,6 @@ AsyncResource::AsyncResource(Isolate* isolate,
 
 AsyncResource::~AsyncResource() {
   EmitAsyncDestroy(env_, async_context_);
-  resource_.Reset();
 }
 
 MaybeLocal<Value> AsyncResource::MakeCallback(Local<Function> callback,

src/node.h

@@ -893,7 +893,7 @@ class NODE_EXTERN AsyncResource {
 
  private:
   Environment* env_;
-  v8::Persistent<v8::Object> resource_;
+  v8::Global<v8::Object> resource_;
   async_context async_context_;
 };
 

src/node_object_wrap.h

@@ -65,6 +65,7 @@ class ObjectWrap {
   }
 
 
+  // NOLINTNEXTLINE(runtime/v8_persistent)
   inline v8::Persistent<v8::Object>& persistent() {
     return handle_;
   }
@@ -122,6 +123,7 @@ class ObjectWrap {
     delete wrap;
   }
 
+  // NOLINTNEXTLINE(runtime/v8_persistent)
   v8::Persistent<v8::Object> handle_;
 };
 

test/addons/async-hello-world/binding.cc

@@ -14,7 +14,7 @@ struct async_req {
   int input;
   int output;
   v8::Isolate* isolate;
-  v8::Persistent<v8::Function> callback;
+  v8::Global<v8::Function> callback;
   node::async_context context;
 };
 
@@ -61,7 +61,6 @@ void AfterAsync(uv_work_t* r) {
   v8::SealHandleScope seal_handle_scope(isolate);
   // cleanup
   node::EmitAsyncDestroy(isolate, req->context);
-  req->callback.Reset();
   delete req;
 
   if (try_catch.HasCaught()) {

test/addons/callback-scope/binding.cc

@@ -4,6 +4,7 @@
 
 #include <assert.h>
 #include <vector>
+#include <memory>
 
 namespace {
 
@@ -31,16 +32,14 @@ void RunInCallbackScope(const v8::FunctionCallbackInfo<v8::Value>& args) {
     args.GetReturnValue().Set(ret.ToLocalChecked());
 }
 
-static v8::Persistent<v8::Promise::Resolver> persistent;
-
 static void Callback(uv_work_t* req, int ignored) {
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
   v8::HandleScope scope(isolate);
   node::CallbackScope callback_scope(isolate, v8::Object::New(isolate),
                                      node::async_context{0, 0});
-
-  v8::Local<v8::Promise::Resolver> local =
-      v8::Local<v8::Promise::Resolver>::New(isolate, persistent);
+  std::unique_ptr<v8::Global<v8::Promise::Resolver>> persistent {
+      static_cast<v8::Global<v8::Promise::Resolver>*>(req->data) };
+  v8::Local<v8::Promise::Resolver> local = persistent->Get(isolate);
   local->Resolve(isolate->GetCurrentContext(),
                  v8::Undefined(isolate)).ToChecked();
   delete req;
@@ -49,20 +48,21 @@ static void Callback(uv_work_t* req, int ignored) {
 static void TestResolveAsync(const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();
 
-  if (persistent.IsEmpty()) {
-    persistent.Reset(isolate, v8::Promise::Resolver::New(
-        isolate->GetCurrentContext()).ToLocalChecked());
+  v8::Global<v8::Promise::Resolver>* persistent =
+      new v8::Global<v8::Promise::Resolver>(
+          isolate,
+          v8::Promise::Resolver::New(
+              isolate->GetCurrentContext()).ToLocalChecked());
 
-    uv_work_t* req = new uv_work_t;
+  uv_work_t* req = new uv_work_t;
+  req->data = static_cast<void*>(persistent);
 
-    uv_queue_work(node::GetCurrentEventLoop(isolate),
-                  req,
-                  [](uv_work_t*) {},
-                  Callback);
-  }
+  uv_queue_work(node::GetCurrentEventLoop(isolate),
+                req,
+                [](uv_work_t*) {},
+                Callback);
 
-  v8::Local<v8::Promise::Resolver> local =
-      v8::Local<v8::Promise::Resolver>::New(isolate, persistent);
+  v8::Local<v8::Promise::Resolver> local = persistent->Get(isolate);
 
   args.GetReturnValue().Set(local->GetPromise());
 }

tools/cpplint.py

@@ -321,6 +321,7 @@
     'runtime/string',
     'runtime/threadsafe_fn',
     'runtime/vlog',
+    'runtime/v8_persistent',
     'whitespace/blank_line',
     'whitespace/braces',
     'whitespace/comma',
@@ -627,6 +628,8 @@
 
 _NULL_TOKEN_PATTERN = re.compile(r'\bNULL\b')
 
+_V8_PERSISTENT_PATTERN = re.compile(r'\bv8::Persistent\b')
+
 _RIGHT_LEANING_POINTER_PATTERN = re.compile(r'[^=|(,\s><);&?:}]'
                                             r'(?<!(sizeof|return))'
                                             r'\s\*[a-zA-Z_][0-9a-zA-Z_]*')
@@ -4547,6 +4550,28 @@ def CheckNullTokens(filename, clean_lines, linenum, error):
     error(filename, linenum, 'readability/null_usage', 2,
           'Use nullptr instead of NULL')
 
+def CheckV8PersistentTokens(filename, clean_lines, linenum, error):
+  """Check v8::Persistent usage.
+
+  Args:
+    filename: The name of the current file.
+    clean_lines: A CleansedLines instance containing the file.
+    linenum: The number of the line to check.
+    error: The function to call with any errors found.
+  """
+  line = clean_lines.elided[linenum]
+
+  # Avoid preprocessor lines
+  if Match(r'^\s*#', line):
+    return
+
+  if line.find('/*') >= 0 or line.find('*/') >= 0:
+    return
+
+  for match in _V8_PERSISTENT_PATTERN.finditer(line):
+    error(filename, linenum, 'runtime/v8_persistent', 2,
+          'Use v8::Global instead of v8::Persistent')
+
 def CheckLeftLeaningPointer(filename, clean_lines, linenum, error):
   """Check for left-leaning pointer placement.
 
@@ -4723,6 +4748,7 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state,
   CheckCheck(filename, clean_lines, linenum, error)
   CheckAltTokens(filename, clean_lines, linenum, error)
   CheckNullTokens(filename, clean_lines, linenum, error)
+  CheckV8PersistentTokens(filename, clean_lines, linenum, error)
   CheckLeftLeaningPointer(filename, clean_lines, linenum, error)
   classinfo = nesting_state.InnermostClass()
   if classinfo: