Prevent gin from recreating wrappers after GC

This changes gin::Wrappable to track its cleanup
state in order to prevent async operations from
recreating a wrapper between first and second weak
callbacks.

GetWrapper is changed to return a MaybeLocal,
and callers are updated accordingly; checking
in some cases and failing gracefully in others.

Mojo JS's WaitingCallback is changed to silently
ignore handle notifications if its wrapper is
no longer alive.

BUG=707689

Change-Id: I3fc11a24209f0ef35bfa18556f5734c4b18ae229
Reviewed-on: https://chromium-review.googlesource.com/475077
Commit-Queue: Ken Rockot <rockot@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#464134}

extensions/renderer/api_binding_bridge.cc

@@ -35,7 +35,7 @@ APIBindingBridge::APIBindingBridge(APIBindingHooks* hooks,
       context_type_(context_type),
       run_js_(run_js) {
   v8::Isolate* isolate = context->GetIsolate();
-  v8::Local<v8::Object> wrapper = GetWrapper(isolate);
+  v8::Local<v8::Object> wrapper = GetWrapper(isolate).ToLocalChecked();
   v8::Maybe<bool> result = wrapper->SetPrivate(
       context, GetPrivatePropertyName(isolate, kApiObjectKey), api_object);
   if (!result.IsJust() || !result.FromJust()) {
@@ -64,7 +64,10 @@ void APIBindingBridge::RegisterCustomHook(v8::Isolate* isolate,
   // functions in binding.js.
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
   v8::Local<v8::Object> hook_object = v8::Object::New(isolate);
-  v8::Local<v8::Object> wrapper = GetWrapper(isolate);
+  v8::Local<v8::Object> wrapper;
+  if (!GetWrapper(isolate).ToLocal(&wrapper))
+    return;
+
   v8::Local<v8::Value> hook_interface =
       wrapper->GetPrivate(
           context, GetPrivatePropertyName(isolate, kHookInterfaceKey))

extensions/renderer/chrome_setting.cc

@@ -127,7 +127,7 @@ v8::Local<v8::Value> ChromeSetting::GetOnChangeEvent(
     gin::Arguments* arguments) {
   v8::Isolate* isolate = arguments->isolate();
   v8::Local<v8::Context> context = arguments->GetHolderCreationContext();
-  v8::Local<v8::Object> wrapper = GetWrapper(isolate);
+  v8::Local<v8::Object> wrapper = GetWrapper(isolate).ToLocalChecked();
   v8::Local<v8::Private> key = v8::Private::ForApi(
       isolate, gin::StringToSymbol(isolate, "onChangeEvent"));
   v8::Local<v8::Value> event;

gin/converter.h

@@ -27,7 +27,7 @@ bool SetProperty(v8::Isolate* isolate,
   return !maybe.IsNothing() && maybe.FromJust();
 }
 
-template<typename T>
+template <typename T, typename Enable = void>
 struct ToV8ReturnsMaybe {
   static const bool value = false;
 };

gin/handle.h

@@ -60,8 +60,8 @@ struct Converter<gin::Handle<T> > {
 // without having to write out the type of the object explicitly.
 template<typename T>
 gin::Handle<T> CreateHandle(v8::Isolate* isolate, T* object) {
-  v8::Local<v8::Object> wrapper = object->GetWrapper(isolate);
-  if (wrapper.IsEmpty())
+  v8::Local<v8::Object> wrapper;
+  if (!object->GetWrapper(isolate).ToLocal(&wrapper) || wrapper.IsEmpty())
     return gin::Handle<T>();
   return gin::Handle<T>(wrapper, object);
 }

gin/interceptor_unittest.cc

@@ -149,7 +149,9 @@ class InterceptorTest : public V8Test {
     EXPECT_FALSE(val.IsEmpty());
     v8::Local<v8::Function> func;
     EXPECT_TRUE(ConvertFromV8(isolate, val, &func));
-    v8::Local<v8::Value> argv[] = {ConvertToV8(isolate, obj.get()), };
+    v8::Local<v8::Value> argv[] = {
+        ConvertToV8(isolate->GetCurrentContext(), obj.get()).ToLocalChecked(),
+    };
     func->Call(v8::Undefined(isolate), 1, argv);
     EXPECT_FALSE(try_catch.HasCaught());
     EXPECT_EQ("", try_catch.GetStackTrace());

gin/modules/timer.cc

@@ -46,6 +46,7 @@ Timer::Timer(v8::Isolate* isolate, bool repeating, int delay_ms,
           isolate->GetCurrentContext())->runner()->GetWeakPtr()),
       weak_factory_(this) {
   GetWrapper(runner_->GetContextHolder()->isolate())
+      .ToLocalChecked()
       ->SetPrivate(isolate->GetCurrentContext(), GetHiddenPropertyName(isolate),
                    function)
       .FromJust();
@@ -68,6 +69,7 @@ void Timer::OnTimerFired() {
   v8::Isolate* isolate = runner_->GetContextHolder()->isolate();
   v8::Local<v8::Function> function = v8::Local<v8::Function>::Cast(
       GetWrapper(isolate)
+          .ToLocalChecked()
           ->GetPrivate(runner_->GetContextHolder()->context(),
                        GetHiddenPropertyName(isolate))
           .ToLocalChecked());
@@ -87,7 +89,7 @@ Handle<TimerModule> TimerModule::Create(v8::Isolate* isolate) {
 
 // static
 v8::Local<v8::Value> TimerModule::GetModule(v8::Isolate* isolate) {
-  return Create(isolate)->GetWrapper(isolate);
+  return Create(isolate)->GetWrapper(isolate).ToLocalChecked();
 }
 
 TimerModule::TimerModule() {

gin/modules/timer_unittest.cc

@@ -62,9 +62,9 @@ struct TestHelper {
         result(Result::Create(isolate)) {
     EXPECT_FALSE(runner->global().IsEmpty());
     runner->global()->Set(StringToV8(isolate, "timer"),
-                          timer_module->GetWrapper(isolate));
+                          timer_module->GetWrapper(isolate).ToLocalChecked());
     runner->global()->Set(StringToV8(isolate, "result"),
-                          result->GetWrapper(isolate));
+                          result->GetWrapper(isolate).ToLocalChecked());
   }
 
   void QuitSoon(base::MessageLoop* message_loop) {

gin/wrappable.cc

@@ -25,6 +25,7 @@ ObjectTemplateBuilder WrappableBase::GetObjectTemplateBuilder(
 void WrappableBase::FirstWeakCallback(
     const v8::WeakCallbackInfo<WrappableBase>& data) {
   WrappableBase* wrappable = data.GetParameter();
+  wrappable->dead_ = true;
   wrappable->wrapper_.Reset();
   data.SetSecondPassCallback(SecondWeakCallback);
 }
@@ -35,12 +36,16 @@ void WrappableBase::SecondWeakCallback(
   delete wrappable;
 }
 
-v8::Local<v8::Object> WrappableBase::GetWrapperImpl(v8::Isolate* isolate,
-                                                    WrapperInfo* info) {
+v8::MaybeLocal<v8::Object> WrappableBase::GetWrapperImpl(v8::Isolate* isolate,
+                                                         WrapperInfo* info) {
   if (!wrapper_.IsEmpty()) {
-    return v8::Local<v8::Object>::New(isolate, wrapper_);
+    return v8::MaybeLocal<v8::Object>(
+        v8::Local<v8::Object>::New(isolate, wrapper_));
   }
 
+  if (dead_)
+    return v8::MaybeLocal<v8::Object>();
+
   PerIsolateData* data = PerIsolateData::From(isolate);
   v8::Local<v8::ObjectTemplate> templ = data->GetObjectTemplate(info);
   if (templ.IsEmpty()) {
@@ -56,15 +61,15 @@ v8::Local<v8::Object> WrappableBase::GetWrapperImpl(v8::Isolate* isolate,
     // The current wrappable object will be no longer managed by V8. Delete this
     // now.
     delete this;
-    return wrapper;
+    return v8::MaybeLocal<v8::Object>(wrapper);
   }
 
   int indices[] = {kWrapperInfoIndex, kEncodedValueIndex};
   void* values[] = {info, this};
   wrapper->SetAlignedPointerInInternalFields(2, indices, values);
   wrapper_.Reset(isolate, wrapper);
   wrapper_.SetWeak(this, FirstWeakCallback, v8::WeakCallbackType::kParameter);
-  return wrapper;
+  return v8::MaybeLocal<v8::Object>(wrapper);
 }
 
 namespace internal {

gin/wrappable.h

@@ -70,15 +70,16 @@ class GIN_EXPORT WrappableBase {
   // Overrides of this method should be declared final and not overridden again.
   virtual ObjectTemplateBuilder GetObjectTemplateBuilder(v8::Isolate* isolate);
 
-  v8::Local<v8::Object> GetWrapperImpl(v8::Isolate* isolate,
-                                        WrapperInfo* wrapper_info);
+  v8::MaybeLocal<v8::Object> GetWrapperImpl(v8::Isolate* isolate,
+                                            WrapperInfo* wrapper_info);
 
  private:
   static void FirstWeakCallback(
       const v8::WeakCallbackInfo<WrappableBase>& data);
   static void SecondWeakCallback(
       const v8::WeakCallbackInfo<WrappableBase>& data);
 
+  bool dead_ = false;
   v8::Global<v8::Object> wrapper_;  // Weak
 
   DISALLOW_COPY_AND_ASSIGN(WrappableBase);
@@ -89,7 +90,7 @@ template<typename T>
 class Wrappable : public WrappableBase {
  public:
   // Retrieve (or create) the v8 wrapper object cooresponding to this object.
-  v8::Local<v8::Object> GetWrapper(v8::Isolate* isolate) {
+  v8::MaybeLocal<v8::Object> GetWrapper(v8::Isolate* isolate) {
     return GetWrapperImpl(isolate, &T::kWrapperInfo);
   }
 
@@ -101,14 +102,25 @@ class Wrappable : public WrappableBase {
   DISALLOW_COPY_AND_ASSIGN(Wrappable);
 };
 
+template <typename T>
+struct ToV8ReturnsMaybe<
+    T*,
+    typename std::enable_if<
+        std::is_convertible<T*, WrappableBase*>::value>::type> {
+  static const bool value = true;
+};
 
 // This converter handles any subclass of Wrappable.
 template <typename T>
 struct Converter<T*,
                  typename std::enable_if<
                      std::is_convertible<T*, WrappableBase*>::value>::type> {
-  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate, T* val) {
-    return val->GetWrapper(isolate);
+  static v8::MaybeLocal<v8::Value> ToV8(v8::Local<v8::Context> context,
+                                        T* val) {
+    v8::Local<v8::Object> wrapper;
+    if (!val->GetWrapper(context->GetIsolate()).ToLocal(&wrapper))
+      return v8::MaybeLocal<v8::Value>();
+    return v8::MaybeLocal<v8::Value>(wrapper);
   }
 
   static bool FromV8(v8::Isolate* isolate, v8::Local<v8::Value> val, T** out) {

gin/wrappable_unittest.cc

@@ -105,7 +105,8 @@ TEST_F(WrappableTest, WrapAndUnwrap) {
 
   Handle<MyObject> obj = MyObject::Create(isolate);
 
-  v8::Local<v8::Value> wrapper = ConvertToV8(isolate, obj.get());
+  v8::Local<v8::Value> wrapper =
+      ConvertToV8(isolate->GetCurrentContext(), obj.get()).ToLocalChecked();
   EXPECT_FALSE(wrapper.IsEmpty());
 
   MyObject* unwrapped = NULL;
@@ -130,7 +131,8 @@ TEST_F(WrappableTest, UnwrapFailures) {
 
   // An object that's wrapping a C++ object of the wrong type.
   thing.Clear();
-  thing = ConvertToV8(isolate, new MyObject2());
+  thing = ConvertToV8(isolate->GetCurrentContext(), new MyObject2())
+              .ToLocalChecked();
   EXPECT_FALSE(ConvertFromV8(isolate, thing, &unwrapped));
   EXPECT_FALSE(unwrapped);
 }
@@ -158,7 +160,7 @@ TEST_F(WrappableTest, GetAndSetProperty) {
   v8::Local<v8::Function> func;
   EXPECT_TRUE(ConvertFromV8(isolate, val, &func));
   v8::Local<v8::Value> argv[] = {
-    ConvertToV8(isolate, obj.get()),
+      ConvertToV8(isolate->GetCurrentContext(), obj.get()).ToLocalChecked(),
   };
   func->Call(v8::Undefined(isolate), 1, argv);
   EXPECT_FALSE(try_catch.HasCaught());
@@ -183,7 +185,7 @@ TEST_F(WrappableTest, CallAsFunction) {
   v8::Local<v8::Function> func;
   EXPECT_TRUE(ConvertFromV8(isolate, val, &func));
   v8::Local<v8::Value> argv[] = {
-    ConvertToV8(isolate, object.get())
+      ConvertToV8(isolate->GetCurrentContext(), object.get()).ToLocalChecked(),
   };
   func->Call(v8::Undefined(isolate), 1, argv);
   EXPECT_FALSE(try_catch.HasCaught());
@@ -206,7 +208,7 @@ TEST_F(WrappableTest, CallAsConstructor) {
   v8::Local<v8::Function> func;
   EXPECT_TRUE(ConvertFromV8(isolate, val, &func));
   v8::Local<v8::Value> argv[] = {
-    ConvertToV8(isolate, object.get())
+      ConvertToV8(isolate->GetCurrentContext(), object.get()).ToLocalChecked(),
   };
   func->Call(v8::Undefined(isolate), 1, argv);
   EXPECT_TRUE(try_catch.HasCaught());

mojo/edk/js/waiting_callback.cc

@@ -57,9 +57,9 @@ WaitingCallback::WaitingCallback(v8::Isolate* isolate,
       weak_factory_(this) {
   v8::Handle<v8::Context> context = isolate->GetCurrentContext();
   runner_ = gin::PerContextData::From(context)->runner()->GetWeakPtr();
-  GetWrapper(isolate)
-      ->SetPrivate(context, GetHiddenPropertyName(isolate), callback)
-      .FromJust();
+  v8::Maybe<bool> result = GetWrapper(isolate).ToLocalChecked()->SetPrivate(
+      context, GetHiddenPropertyName(isolate), callback);
+  DCHECK(result.IsJust() && result.FromJust());
 }
 
 WaitingCallback::~WaitingCallback() {
@@ -73,15 +73,22 @@ void WaitingCallback::OnHandleReady(MojoResult result) {
   gin::Runner::Scope scope(runner_.get());
   v8::Isolate* isolate = runner_->GetContextHolder()->isolate();
 
-  v8::Handle<v8::Value> hidden_value =
-      GetWrapper(isolate)
+  v8::Local<v8::Object> wrapper;
+  if (!GetWrapper(isolate).ToLocal(&wrapper)) {
+    Cancel();
+    return;
+  }
+
+  v8::Local<v8::Value> hidden_value =
+      wrapper
           ->GetPrivate(runner_->GetContextHolder()->context(),
                        GetHiddenPropertyName(isolate))
           .ToLocalChecked();
-  v8::Handle<v8::Function> callback;
-  CHECK(gin::ConvertFromV8(isolate, hidden_value, &callback));
+  v8::Local<v8::Function> callback;
+  bool convert_result = gin::ConvertFromV8(isolate, hidden_value, &callback);
+  DCHECK(convert_result);
 
-  v8::Handle<v8::Value> args[] = { gin::ConvertToV8(isolate, result) };
+  v8::Local<v8::Value> args[] = {gin::ConvertToV8(isolate, result)};
   runner_->Call(callback, runner_->global(), 1, args);
 
   if (one_shot_ || result == MOJO_RESULT_CANCELLED) {