Revert 'Introduce phantom weak handles in the API and use them internally for debug info'

Revert of https://codereview.chromium.org/649563006/ due to layout test
failures.
BUG=
R=mstarzinger@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#24901}
git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@24901 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

include/v8.h

@@ -516,18 +516,6 @@ template <class T> class PersistentBase {
       P* parameter,
       typename WeakCallbackData<S, P>::Callback callback);
 
-  // Phantom persistents work like weak persistents, except that the pointer to
-  // the object being collected is not available in the finalization callback.
-  // This enables the garbage collector to collect the object and any objects
-  // it references transitively in one GC cycle.
-  template <typename P>
-  V8_INLINE void SetPhantom(P* parameter,
-                            typename WeakCallbackData<T, P>::Callback callback);
-
-  template <typename S, typename P>
-  V8_INLINE void SetPhantom(P* parameter,
-                            typename WeakCallbackData<S, P>::Callback callback);
-
   template<typename P>
   V8_INLINE P* ClearWeak();
 
@@ -5342,15 +5330,14 @@ class V8_EXPORT V8 {
  private:
   V8();
 
-  enum WeakHandleType { PhantomHandle, NonphantomHandle };
-
   static internal::Object** GlobalizeReference(internal::Isolate* isolate,
                                                internal::Object** handle);
   static internal::Object** CopyPersistent(internal::Object** handle);
   static void DisposeGlobal(internal::Object** global_handle);
   typedef WeakCallbackData<Value, void>::Callback WeakCallback;
-  static void MakeWeak(internal::Object** global_handle, void* data,
-                       WeakCallback weak_callback, WeakHandleType phantom);
+  static void MakeWeak(internal::Object** global_handle,
+                       void* data,
+                       WeakCallback weak_callback);
   static void* ClearWeak(internal::Object** global_handle);
   static void Eternalize(Isolate* isolate,
                          Value* handle,
@@ -6213,8 +6200,9 @@ void PersistentBase<T>::SetWeak(
     typename WeakCallbackData<S, P>::Callback callback) {
   TYPE_CHECK(S, T);
   typedef typename WeakCallbackData<Value, void>::Callback Callback;
-  V8::MakeWeak(reinterpret_cast<internal::Object**>(this->val_), parameter,
-               reinterpret_cast<Callback>(callback), V8::NonphantomHandle);
+  V8::MakeWeak(reinterpret_cast<internal::Object**>(this->val_),
+               parameter,
+               reinterpret_cast<Callback>(callback));
 }
 
 
@@ -6228,26 +6216,7 @@ void PersistentBase<T>::SetWeak(
 
 
 template <class T>
-template <typename S, typename P>
-void PersistentBase<T>::SetPhantom(
-    P* parameter, typename WeakCallbackData<S, P>::Callback callback) {
-  TYPE_CHECK(S, T);
-  typedef typename WeakCallbackData<Value, void>::Callback Callback;
-  V8::MakeWeak(reinterpret_cast<internal::Object**>(this->val_), parameter,
-               reinterpret_cast<Callback>(callback), V8::PhantomHandle);
-}
-
-
-template <class T>
-template <typename P>
-void PersistentBase<T>::SetPhantom(
-    P* parameter, typename WeakCallbackData<T, P>::Callback callback) {
-  SetPhantom<T, P>(parameter, callback);
-}
-
-
-template <class T>
-template <typename P>
+template<typename P>
 P* PersistentBase<T>::ClearWeak() {
   return reinterpret_cast<P*>(
     V8::ClearWeak(reinterpret_cast<internal::Object**>(this->val_)));

src/api.cc

@@ -509,12 +509,10 @@ i::Object** V8::CopyPersistent(i::Object** obj) {
 }
 
 
-void V8::MakeWeak(i::Object** object, void* parameters,
-                  WeakCallback weak_callback, V8::WeakHandleType weak_type) {
-  i::GlobalHandles::PhantomState phantom;
-  phantom = weak_type == V8::PhantomHandle ? i::GlobalHandles::Phantom
-                                           : i::GlobalHandles::Nonphantom;
-  i::GlobalHandles::MakeWeak(object, parameters, weak_callback, phantom);
+void V8::MakeWeak(i::Object** object,
+                  void* parameters,
+                  WeakCallback weak_callback) {
+  i::GlobalHandles::MakeWeak(object, parameters, weak_callback);
 }
 
 

src/compiler.cc

@@ -1294,7 +1294,7 @@ Handle<SharedFunctionInfo> Compiler::BuildFunctionInfo(
 
   if (outer_info->is_toplevel() && outer_info->will_serialize()) {
     // Make sure that if the toplevel code (possibly to be serialized),
-    // the inner function must be allowed to be compiled lazily.
+    // the inner unction must be allowed to be compiled lazily.
     DCHECK(allow_lazy);
   }
 

src/debug.cc

@@ -594,7 +594,10 @@ ScriptCache::ScriptCache(Isolate* isolate) : HashMap(HashMap::PointersMatch),
   Heap* heap = isolate_->heap();
   HandleScope scope(isolate_);
 
-  // Perform a GC to get rid of all unreferenced scripts.
+  // Perform two GCs to get rid of all unreferenced scripts. The first GC gets
+  // rid of all the cached script wrappers and the second gets rid of the
+  // scripts which are no longer referenced.
+  heap->CollectAllGarbage(Heap::kMakeHeapIterableMask, "ScriptCache");
   heap->CollectAllGarbage(Heap::kMakeHeapIterableMask, "ScriptCache");
 
   // Scan heap for Script objects.
@@ -691,7 +694,13 @@ void Debug::HandleWeakDebugInfo(
   Debug* debug = reinterpret_cast<Isolate*>(data.GetIsolate())->debug();
   DebugInfoListNode* node =
       reinterpret_cast<DebugInfoListNode*>(data.GetParameter());
-  debug->RemoveDebugInfo(node->debug_info().location());
+  // We need to clear all breakpoints associated with the function to restore
+  // original code and avoid patching the code twice later because
+  // the function will live in the heap until next gc, and can be found by
+  // Debug::FindSharedFunctionInfoInScript.
+  BreakLocationIterator it(node->debug_info(), ALL_BREAK_LOCATIONS);
+  it.ClearAllDebugBreak();
+  debug->RemoveDebugInfo(node->debug_info());
 #ifdef DEBUG
   for (DebugInfoListNode* n = debug->debug_info_list_;
        n != NULL;
@@ -707,8 +716,8 @@ DebugInfoListNode::DebugInfoListNode(DebugInfo* debug_info): next_(NULL) {
   GlobalHandles* global_handles = debug_info->GetIsolate()->global_handles();
   debug_info_ = Handle<DebugInfo>::cast(global_handles->Create(debug_info));
   GlobalHandles::MakeWeak(reinterpret_cast<Object**>(debug_info_.location()),
-                          this, Debug::HandleWeakDebugInfo,
-                          GlobalHandles::Phantom);
+                          this,
+                          Debug::HandleWeakDebugInfo);
 }
 
 
@@ -1163,7 +1172,7 @@ void Debug::ClearBreakPoint(Handle<Object> break_point_object) {
       // If there are no more break points left remove the debug info for this
       // function.
       if (debug_info->GetBreakPointCount() == 0) {
-        RemoveDebugInfoAndClearFromShared(debug_info);
+        RemoveDebugInfo(debug_info);
       }
 
       return;
@@ -1184,7 +1193,7 @@ void Debug::ClearAllBreakPoints() {
 
   // Remove all debug info.
   while (debug_info_list_ != NULL) {
-    RemoveDebugInfoAndClearFromShared(debug_info_list_->debug_info());
+    RemoveDebugInfo(debug_info_list_->debug_info());
   }
 }
 
@@ -2186,23 +2195,21 @@ bool Debug::EnsureDebugInfo(Handle<SharedFunctionInfo> shared,
 }
 
 
-// This uses the location of a handle to look up the debug info in the debug
-// info list, but it doesn't use the actual debug info for anything.  Therefore
-// if the debug info has been collected by the GC, we can be sure that this
-// method will not attempt to resurrect it.
-void Debug::RemoveDebugInfo(DebugInfo** debug_info) {
+void Debug::RemoveDebugInfo(Handle<DebugInfo> debug_info) {
   DCHECK(debug_info_list_ != NULL);
   // Run through the debug info objects to find this one and remove it.
   DebugInfoListNode* prev = NULL;
   DebugInfoListNode* current = debug_info_list_;
   while (current != NULL) {
-    if (current->debug_info().location() == debug_info) {
+    if (*current->debug_info() == *debug_info) {
       // Unlink from list. If prev is NULL we are looking at the first element.
       if (prev == NULL) {
         debug_info_list_ = current->next();
       } else {
         prev->set_next(current->next());
       }
+      current->debug_info()->shared()->set_debug_info(
+              isolate_->heap()->undefined_value());
       delete current;
 
       // If there are no more debug info objects there are not more break
@@ -2219,16 +2226,6 @@ void Debug::RemoveDebugInfo(DebugInfo** debug_info) {
 }
 
 
-void Debug::RemoveDebugInfoAndClearFromShared(Handle<DebugInfo> debug_info) {
-  HandleScope scope(isolate_);
-  Handle<SharedFunctionInfo> shared(debug_info->shared());
-
-  RemoveDebugInfo(debug_info.location());
-
-  shared->set_debug_info(isolate_->heap()->undefined_value());
-}
-
-
 void Debug::SetAfterBreakTarget(JavaScriptFrame* frame) {
   after_break_target_ = NULL;
 
@@ -2323,7 +2320,7 @@ bool Debug::IsBreakAtReturn(JavaScriptFrame* frame) {
   HandleScope scope(isolate_);
 
   // If there are no break points this cannot be break at return, as
-  // the debugger statement and stack guard debug break cannot be at
+  // the debugger statement and stack guard bebug break cannot be at
   // return.
   if (!has_break_points_) {
     return false;
@@ -3243,7 +3240,7 @@ v8::Handle<v8::String> MessageImpl::GetJSON() const {
 v8::Handle<v8::Context> MessageImpl::GetEventContext() const {
   Isolate* isolate = event_data_->GetIsolate();
   v8::Handle<v8::Context> context = GetDebugEventContext(isolate);
-  // Isolate::context() may be NULL when "script collected" event occurs.
+  // Isolate::context() may be NULL when "script collected" event occures.
   DCHECK(!context.IsEmpty());
   return context;
 }

src/debug.h

@@ -559,8 +559,8 @@ class Debug {
   void ClearStepIn();
   void ActivateStepOut(StackFrame* frame);
   void ClearStepNext();
-  void RemoveDebugInfoAndClearFromShared(Handle<DebugInfo> debug_info);
-  void RemoveDebugInfo(DebugInfo** debug_info);
+  // Returns whether the compile succeeded.
+  void RemoveDebugInfo(Handle<DebugInfo> debug_info);
   Handle<Object> CheckBreakPoints(Handle<Object> break_point);
   bool CheckBreakPoint(Handle<Object> break_point_object);
 

src/global-handles.cc

@@ -146,13 +146,6 @@ class GlobalHandles::Node {
     flags_ = IsInNewSpaceList::update(flags_, v);
   }
 
-  bool is_zapped_during_weak_callback() {
-    return IsZappedDuringWeakCallback::decode(flags_);
-  }
-  void set_is_zapped_during_weak_callback(bool v) {
-    flags_ = IsZappedDuringWeakCallback::update(flags_, v);
-  }
-
   bool IsNearDeath() const {
     // Check for PENDING to ensure correct answer when processing callbacks.
     return state() == PENDING || state() == NEAR_DEATH;
@@ -211,14 +204,12 @@ class GlobalHandles::Node {
     parameter_or_next_free_.next_free = value;
   }
 
-  void MakeWeak(void* parameter, WeakCallback weak_callback,
-                bool is_zapped_during_weak_callback = false) {
+  void MakeWeak(void* parameter, WeakCallback weak_callback) {
     DCHECK(weak_callback != NULL);
     DCHECK(state() != FREE);
     CHECK(object_ != NULL);
     set_state(WEAK);
     set_parameter(parameter);
-    set_is_zapped_during_weak_callback(is_zapped_during_weak_callback);
     weak_callback_ = weak_callback;
   }
 
@@ -236,7 +227,7 @@ class GlobalHandles::Node {
       Release();
       return false;
     }
-    void* param = parameter();
+    void* par = parameter();
     set_state(NEAR_DEATH);
     set_parameter(NULL);
 
@@ -249,28 +240,14 @@ class GlobalHandles::Node {
       DCHECK(!object_->IsExternalTwoByteString() ||
              ExternalTwoByteString::cast(object_)->resource() != NULL);
       // Leaving V8.
-      VMState<EXTERNAL> vmstate(isolate);
+      VMState<EXTERNAL> state(isolate);
       HandleScope handle_scope(isolate);
-      if (is_zapped_during_weak_callback()) {
-        // Phantom weak pointer case.
-        DCHECK(*object == Smi::FromInt(kPhantomReferenceZap));
-        // Make data with a null handle.
-        v8::WeakCallbackData<v8::Value, void> data(
-            reinterpret_cast<v8::Isolate*>(isolate), v8::Local<v8::Object>(),
-            param);
-        weak_callback_(data);
-        if (state() != FREE) {
-          // Callback does not have to clear the global handle if it is a
-          // phantom handle.
-          Release();
-        }
-      } else {
-        Handle<Object> handle(object);
-        v8::WeakCallbackData<v8::Value, void> data(
-            reinterpret_cast<v8::Isolate*>(isolate), v8::Utils::ToLocal(handle),
-            param);
-        weak_callback_(data);
-      }
+      Handle<Object> handle(*object, isolate);
+      v8::WeakCallbackData<v8::Value, void> data(
+          reinterpret_cast<v8::Isolate*>(isolate),
+          v8::Utils::ToLocal(handle),
+          par);
+      weak_callback_(data);
     }
     // Absence of explicit cleanup or revival of weak handle
     // in most of the cases would lead to memory leak.
@@ -300,11 +277,10 @@ class GlobalHandles::Node {
 
   // This stores three flags (independent, partially_dependent and
   // in_new_space_list) and a State.
-  class NodeState : public BitField<State, 0, 4> {};
-  class IsIndependent : public BitField<bool, 4, 1> {};
-  class IsPartiallyDependent : public BitField<bool, 5, 1> {};
-  class IsInNewSpaceList : public BitField<bool, 6, 1> {};
-  class IsZappedDuringWeakCallback : public BitField<bool, 7, 1> {};
+  class NodeState:            public BitField<State, 0, 4> {};
+  class IsIndependent:        public BitField<bool,  4, 1> {};
+  class IsPartiallyDependent: public BitField<bool,  5, 1> {};
+  class IsInNewSpaceList:     public BitField<bool,  6, 1> {};
 
   uint8_t flags_;
 
@@ -499,10 +475,10 @@ void GlobalHandles::Destroy(Object** location) {
 }
 
 
-void GlobalHandles::MakeWeak(Object** location, void* parameter,
-                             WeakCallback weak_callback, PhantomState phantom) {
-  Node::FromLocation(location)
-      ->MakeWeak(parameter, weak_callback, phantom == Phantom);
+void GlobalHandles::MakeWeak(Object** location,
+                             void* parameter,
+                             WeakCallback weak_callback) {
+  Node::FromLocation(location)->MakeWeak(parameter, weak_callback);
 }
 
 
@@ -538,15 +514,7 @@ bool GlobalHandles::IsWeak(Object** location) {
 
 void GlobalHandles::IterateWeakRoots(ObjectVisitor* v) {
   for (NodeIterator it(this); !it.done(); it.Advance()) {
-    Node* node = it.node();
-    if (node->IsWeakRetainer()) {
-      if (node->state() == Node::PENDING &&
-          node->is_zapped_during_weak_callback()) {
-        *(node->location()) = Smi::FromInt(kPhantomReferenceZap);
-      } else {
-        v->VisitPointer(node->location());
-      }
-    }
+    if (it.node()->IsWeakRetainer()) v->VisitPointer(it.node()->location());
   }
 }
 
@@ -591,11 +559,7 @@ void GlobalHandles::IterateNewSpaceWeakIndependentRoots(ObjectVisitor* v) {
     DCHECK(node->is_in_new_space_list());
     if ((node->is_independent() || node->is_partially_dependent()) &&
         node->IsWeakRetainer()) {
-      if (node->is_zapped_during_weak_callback()) {
-        *(node->location()) = Smi::FromInt(kPhantomReferenceZap);
-      } else {
-        v->VisitPointer(node->location());
-      }
+      v->VisitPointer(node->location());
     }
   }
 }

src/global-handles.h

@@ -112,24 +112,15 @@ class GlobalHandles {
 
   typedef WeakCallbackData<v8::Value, void>::Callback WeakCallback;
 
-  // For a phantom weak reference, the callback does not have access to the
-  // dying object.  Phantom weak references are preferred because they allow
-  // memory to be reclaimed in one GC cycle rather than two.  However, for
-  // historical reasons the default is non-phantom.
-  enum PhantomState { Nonphantom, Phantom };
-
   // Make the global handle weak and set the callback parameter for the
   // handle.  When the garbage collector recognizes that only weak global
-  // handles point to an object the callback function is invoked (for each
-  // handle) with the handle and corresponding parameter as arguments.  By
-  // default the handle still contains a pointer to the object that is being
-  // collected.  For this reason the object is not collected until the next
-  // GC.  For a phantom weak handle the handle is cleared (set to a Smi)
-  // before the callback is invoked, but the handle can still be identified
-  // in the callback by using the location() of the handle.
-  static void MakeWeak(Object** location, void* parameter,
-                       WeakCallback weak_callback,
-                       PhantomState phantom = Nonphantom);
+  // handles point to an object the handles are cleared and the callback
+  // function is invoked (for each handle) with the handle and corresponding
+  // parameter as arguments.  Note: cleared means set to Smi::FromInt(0). The
+  // reason is that Smi::FromInt(0) does not change during garage collection.
+  static void MakeWeak(Object** location,
+                       void* parameter,
+                       WeakCallback weak_callback);
 
   void RecordStats(HeapStats* stats);
 

src/globals.h

@@ -292,7 +292,6 @@ const uint32_t kFreeListZapValue = 0xfeed1eaf;
 #endif
 
 const int kCodeZapValue = 0xbadc0de;
-const uint32_t kPhantomReferenceZap = 0xca11bac;
 
 // On Intel architecture, cache line size is 64 bytes.
 // On ARM it may be less (32 bytes), but as far this constant is

test/cctest/test-api.cc

@@ -7469,13 +7469,14 @@ struct FlagAndPersistent {
 };
 
 
-static void SetFlag(
+static void DisposeAndSetFlag(
     const v8::WeakCallbackData<v8::Object, FlagAndPersistent>& data) {
+  data.GetParameter()->handle.Reset();
   data.GetParameter()->flag = true;
 }
 
 
-static void IndependentWeakHandle(bool global_gc, bool interlinked) {
+THREADED_TEST(IndependentWeakHandle) {
   v8::Isolate* iso = CcTest::isolate();
   v8::HandleScope scope(iso);
   v8::Handle<Context> context = Context::New(iso);
@@ -7483,59 +7484,26 @@ static void IndependentWeakHandle(bool global_gc, bool interlinked) {
 
   FlagAndPersistent object_a, object_b;
 
-  intptr_t big_heap_size;
-
   {
     v8::HandleScope handle_scope(iso);
-    Local<Object> a(v8::Object::New(iso));
-    Local<Object> b(v8::Object::New(iso));
-    object_a.handle.Reset(iso, a);
-    object_b.handle.Reset(iso, b);
-    if (interlinked) {
-      a->Set(v8_str("x"), b);
-      b->Set(v8_str("x"), a);
-    }
-    if (global_gc) {
-      CcTest::heap()->CollectAllGarbage(TestHeap::Heap::kNoGCFlags);
-    } else {
-      CcTest::heap()->CollectGarbage(i::NEW_SPACE);
-    }
-    // We are relying on this creating a big flag array and reserving the space
-    // up front.
-    v8::Handle<Value> big_array = CompileRun("new Array(50000)");
-    a->Set(v8_str("y"), big_array);
-    big_heap_size = CcTest::heap()->SizeOfObjects();
+    object_a.handle.Reset(iso, v8::Object::New(iso));
+    object_b.handle.Reset(iso, v8::Object::New(iso));
   }
 
   object_a.flag = false;
   object_b.flag = false;
-  object_a.handle.SetPhantom(&object_a, &SetFlag);
-  object_b.handle.SetPhantom(&object_b, &SetFlag);
+  object_a.handle.SetWeak(&object_a, &DisposeAndSetFlag);
+  object_b.handle.SetWeak(&object_b, &DisposeAndSetFlag);
   CHECK(!object_b.handle.IsIndependent());
   object_a.handle.MarkIndependent();
   object_b.handle.MarkIndependent();
   CHECK(object_b.handle.IsIndependent());
-  if (global_gc) {
-    CcTest::heap()->CollectAllGarbage(TestHeap::Heap::kNoGCFlags);
-  } else {
-    CcTest::heap()->CollectGarbage(i::NEW_SPACE);
-  }
-  // A single GC should be enough to reclaim the memory, since we are using
-  // phantom handles.
-  CHECK_LT(CcTest::heap()->SizeOfObjects(), big_heap_size - 200000);
+  CcTest::heap()->CollectGarbage(i::NEW_SPACE);
   CHECK(object_a.flag);
   CHECK(object_b.flag);
 }
 
 
-THREADED_TEST(IndependentWeakHandle) {
-  IndependentWeakHandle(false, false);
-  IndependentWeakHandle(false, true);
-  IndependentWeakHandle(true, false);
-  IndependentWeakHandle(true, true);
-}
-
-
 static void InvokeScavenge() {
   CcTest::heap()->CollectGarbage(i::NEW_SPACE);
 }