fix disposal of phantom handles in GlobalValueMap

additionally, add a drive by fix to WeakCallbackInfo

R=jochen@chromium.org, erikcorry@chromium.org

BUG=

Review URL: https://codereview.chromium.org/1026283004

Cr-Commit-Position: refs/heads/master@{#27407}

include/v8-util.h

@@ -134,6 +134,9 @@ class DefaultGlobalMapTraits : public StdMapTraits<K, V> {
   }
   static void DisposeCallbackData(WeakCallbackInfoType* data) {}
   static void Dispose(Isolate* isolate, Global<V> value, K key) {}
+  static void DisposeWeak(Isolate* isolate,
+                          const WeakCallbackInfo<WeakCallbackInfoType>& data,
+                          K key) {}
 
  private:
   template <typename T>
@@ -319,6 +322,8 @@ class PersistentValueMapBase {
     return p.Pass();
   }
 
+  void RemoveWeak(const K& key) { Traits::Remove(&impl_, key); }
+
  private:
   PersistentValueMapBase(PersistentValueMapBase&);
   void operator=(PersistentValueMapBase&);
@@ -469,8 +474,8 @@ class GlobalValueMap : public PersistentValueMapBase<K, V, Traits> {
       GlobalValueMap<K, V, Traits>* persistentValueMap =
           Traits::MapFromWeakCallbackInfo(data);
       K key = Traits::KeyFromWeakCallbackInfo(data);
-      Traits::Dispose(data.GetIsolate(), persistentValueMap->Remove(key).Pass(),
-                      key);
+      persistentValueMap->RemoveWeak(key);
+      Traits::DisposeWeak(data.GetIsolate(), data, key);
       Traits::DisposeCallbackData(data.GetParameter());
     }
   }

include/v8.h

@@ -471,6 +471,9 @@ template <class T> class Eternal {
 };
 
 
+static const int kInternalFieldsInWeakCallback = 2;
+
+
 template <typename T>
 class WeakCallbackInfo {
  public:
@@ -478,21 +481,28 @@ class WeakCallbackInfo {
 
   WeakCallbackInfo(Isolate* isolate, T* parameter, void* internal_field1,
                    void* internal_field2)
-      : isolate_(isolate),
-        parameter_(parameter),
-        internal_field1_(internal_field1),
-        internal_field2_(internal_field2) {}
+      : isolate_(isolate), parameter_(parameter) {
+    internal_fields_[0] = internal_field1;
+    internal_fields_[1] = internal_field2;
+  }
 
   V8_INLINE Isolate* GetIsolate() const { return isolate_; }
   V8_INLINE T* GetParameter() const { return parameter_; }
-  V8_INLINE void* GetInternalField1() const { return internal_field1_; }
-  V8_INLINE void* GetInternalField2() const { return internal_field2_; }
+  V8_INLINE void* GetInternalField(int index) const;
+
+  V8_INLINE V8_DEPRECATE_SOON("use indexed version",
+                              void* GetInternalField1()) const {
+    return internal_fields_[0];
+  }
+  V8_INLINE V8_DEPRECATE_SOON("use indexed version",
+                              void* GetInternalField2()) const {
+    return internal_fields_[1];
+  }
 
  private:
   Isolate* isolate_;
   T* parameter_;
-  void* internal_field1_;
-  void* internal_field2_;
+  void* internal_fields_[2];
 };
 
 
@@ -5976,6 +5986,7 @@ class V8_EXPORT V8 {
 
   static void CheckIsJust(bool is_just);
   static void ToLocalEmpty();
+  static void InternalFieldOutOfBounds(int index);
 
   template <class T> friend class Handle;
   template <class T> friend class Local;
@@ -5983,6 +5994,8 @@ class V8_EXPORT V8 {
   friend class MaybeLocal;
   template <class T>
   friend class Maybe;
+  template <class T>
+  friend class WeakCallbackInfo;
   template <class T> friend class Eternal;
   template <class T> friend class PersistentBase;
   template <class T, class M> friend class Persistent;
@@ -6834,6 +6847,17 @@ Local<T> MaybeLocal<T>::ToLocalChecked() {
 }
 
 
+template <class T>
+void* WeakCallbackInfo<T>::GetInternalField(int index) const {
+#ifdef V8_ENABLE_CHECKS
+  if (index < 0 || index >= kInternalFieldsInWeakCallback) {
+    V8::InternalFieldOutOfBounds(index);
+  }
+#endif
+  return internal_fields_[index];
+}
+
+
 template <class T>
 T* PersistentBase<T>::New(Isolate* isolate, T* that) {
   if (that == NULL) return NULL;

src/api.cc

@@ -600,6 +600,13 @@ void V8::ToLocalEmpty() {
 }
 
 
+void V8::InternalFieldOutOfBounds(int index) {
+  Utils::ApiCheck(0 <= index && index < kInternalFieldsInWeakCallback,
+                  "WeakCallbackInfo::GetInternalField",
+                  "Internal field out of bounds.");
+}
+
+
 // --- H a n d l e s ---
 
 

test/cctest/test-api.cc

@@ -3193,6 +3193,124 @@ TEST(PersistentValueMap) {
 }
 
 
+namespace {
+
+void* IntKeyToVoidPointer(int key) { return reinterpret_cast<void*>(key << 1); }
+
+
+Local<v8::Object> NewObjectForIntKey(
+    v8::Isolate* isolate, const v8::Global<v8::ObjectTemplate>& templ,
+    int key) {
+  auto local = Local<v8::ObjectTemplate>::New(isolate, templ);
+  auto obj = local->NewInstance();
+  obj->SetAlignedPointerInInternalField(0, IntKeyToVoidPointer(key));
+  return obj;
+}
+
+
+template <typename K, typename V>
+class PhantomStdMapTraits : public v8::StdMapTraits<K, V> {
+ public:
+  typedef typename v8::GlobalValueMap<K, V, PhantomStdMapTraits<K, V>> MapType;
+  static const v8::PersistentContainerCallbackType kCallbackType =
+      v8::kWeakWithInternalFields;
+  struct WeakCallbackDataType {
+    MapType* map;
+    K key;
+  };
+  static WeakCallbackDataType* WeakCallbackParameter(MapType* map, const K& key,
+                                                     Local<V> value) {
+    WeakCallbackDataType* data = new WeakCallbackDataType;
+    data->map = map;
+    data->key = key;
+    return data;
+  }
+  static MapType* MapFromWeakCallbackInfo(
+      const v8::WeakCallbackInfo<WeakCallbackDataType>& data) {
+    return data.GetParameter()->map;
+  }
+  static K KeyFromWeakCallbackInfo(
+      const v8::WeakCallbackInfo<WeakCallbackDataType>& data) {
+    return data.GetParameter()->key;
+  }
+  static void DisposeCallbackData(WeakCallbackDataType* data) { delete data; }
+  static void Dispose(v8::Isolate* isolate, v8::Global<V> value, K key) {
+    CHECK_EQ(IntKeyToVoidPointer(key),
+             v8::Object::GetAlignedPointerFromInternalField(value, 0));
+  }
+  static void DisposeWeak(
+      v8::Isolate* isolate,
+      const v8::WeakCallbackInfo<WeakCallbackDataType>& info, K key) {
+    CHECK_EQ(IntKeyToVoidPointer(key), info.GetInternalField(0));
+  }
+};
+}
+
+
+TEST(GlobalValueMap) {
+  typedef v8::GlobalValueMap<int, v8::Object,
+                             PhantomStdMapTraits<int, v8::Object>> Map;
+  LocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+  v8::Global<ObjectTemplate> templ;
+  {
+    HandleScope scope(isolate);
+    auto t = ObjectTemplate::New(isolate);
+    t->SetInternalFieldCount(1);
+    templ.Reset(isolate, t);
+  }
+  Map map(isolate);
+  v8::internal::GlobalHandles* global_handles =
+      reinterpret_cast<v8::internal::Isolate*>(isolate)->global_handles();
+  int initial_handle_count = global_handles->global_handles_count();
+  CHECK_EQ(0, static_cast<int>(map.Size()));
+  {
+    HandleScope scope(isolate);
+    Local<v8::Object> obj = map.Get(7);
+    CHECK(obj.IsEmpty());
+    Local<v8::Object> expected = v8::Object::New(isolate);
+    map.Set(7, expected);
+    CHECK_EQ(1, static_cast<int>(map.Size()));
+    obj = map.Get(7);
+    CHECK(expected->Equals(obj));
+    {
+      Map::PersistentValueReference ref = map.GetReference(7);
+      CHECK(expected->Equals(ref.NewLocal(isolate)));
+    }
+    v8::Global<v8::Object> removed = map.Remove(7);
+    CHECK_EQ(0, static_cast<int>(map.Size()));
+    CHECK(expected == removed);
+    removed = map.Remove(7);
+    CHECK(removed.IsEmpty());
+    map.Set(8, expected);
+    CHECK_EQ(1, static_cast<int>(map.Size()));
+    map.Set(8, expected);
+    CHECK_EQ(1, static_cast<int>(map.Size()));
+    {
+      Map::PersistentValueReference ref;
+      Local<v8::Object> expected2 = NewObjectForIntKey(isolate, templ, 8);
+      removed = map.Set(8, v8::Global<v8::Object>(isolate, expected2), &ref);
+      CHECK_EQ(1, static_cast<int>(map.Size()));
+      CHECK(expected == removed);
+      CHECK(expected2->Equals(ref.NewLocal(isolate)));
+    }
+  }
+  CHECK_EQ(initial_handle_count + 1, global_handles->global_handles_count());
+  CcTest::i_isolate()->heap()->CollectAllGarbage(
+      i::Heap::kAbortIncrementalMarkingMask);
+  CHECK_EQ(0, static_cast<int>(map.Size()));
+  CHECK_EQ(initial_handle_count, global_handles->global_handles_count());
+  {
+    HandleScope scope(isolate);
+    Local<v8::Object> value = NewObjectForIntKey(isolate, templ, 9);
+    map.Set(9, value);
+    map.Clear();
+  }
+  CHECK_EQ(0, static_cast<int>(map.Size()));
+  CHECK_EQ(initial_handle_count, global_handles->global_handles_count());
+}
+
+
 TEST(PersistentValueVector) {
   LocalContext env;
   v8::Isolate* isolate = env->GetIsolate();