From dd4c82bbb3c3e51a610892c4285cbad3e7ee978c Mon Sep 17 00:00:00 2001
From: "jarin@chromium.org"
 <jarin@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Date: Tue, 20 May 2014 13:19:21 +0000
Subject: [PATCH] Fix Heap::IsHeapIterable.

We only consider heap iterable if the new space is empty (in addition to the exisiting old space check).

The change also moves the iterability forcing + allocation prevention gadgets to HeapIterator so that it is impossible to miss them when iterating the heap.

R=hpayer@chromium.org
BUG=373283
LOG=N

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

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21387 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
---
 src/debug.cc                           |  9 +---
 src/heap-profiler.cc                   |  3 --
 src/heap-snapshot-generator.cc         |  4 --
 src/heap.cc                            | 15 ++++---
 src/heap.h                             | 11 ++++-
 src/liveedit.cc                        | 12 ++----
 src/runtime.cc                         | 76 ++++++++++++++++------------------
 test/cctest/test-api.cc                |  1 -
 test/cctest/test-heap.cc               |  4 +-
 test/cctest/test-object-observe.cc     |  1 -
 test/mjsunit/regress/regress-373283.js | 18 ++++++++
 11 files changed, 80 insertions(+), 74 deletions(-)
 create mode 100644 test/mjsunit/regress/regress-373283.js

diff --git a/src/debug.cc b/src/debug.cc
index a994eb0..9a9d737 100644
--- a/src/debug.cc
+++ b/src/debug.cc
@@ -2048,6 +2048,7 @@ void Debug::PrepareForBreakPoints() {
       Heap* heap = isolate_->heap();
       heap->CollectAllGarbage(Heap::kMakeHeapIterableMask,
                               "preparing for breakpoints");
+      HeapIterator iterator(heap);
 
       // Ensure no GC in this scope as we are going to use gc_metadata
       // field in the Code object to mark active functions.
@@ -2067,7 +2068,6 @@ void Debug::PrepareForBreakPoints() {
       // Scan the heap for all non-optimized functions which have no
       // debug break slots and are not active or inlined into an active
       // function and mark them for lazy compilation.
-      HeapIterator iterator(heap);
       HeapObject* obj = NULL;
       while (((obj = iterator.next()) != NULL)) {
         if (obj->IsJSFunction()) {
@@ -2192,9 +2192,7 @@ Object* Debug::FindSharedFunctionInfoInScript(Handle<Script> script,
   Handle<SharedFunctionInfo> target;
   Heap* heap = isolate_->heap();
   while (!done) {
-    { // Extra scope for iterator and no-allocation.
-      heap->EnsureHeapIsIterable();
-      DisallowHeapAllocation no_alloc_during_heap_iteration;
+    { // Extra scope for iterator.
       HeapIterator iterator(heap);
       for (HeapObject* obj = iterator.next();
            obj != NULL; obj = iterator.next()) {
@@ -2524,8 +2522,6 @@ void Debug::CreateScriptCache() {
   // scripts which are no longer referenced.  The second also sweeps precisely,
   // which saves us doing yet another GC to make the heap iterable.
   heap->CollectAllGarbage(Heap::kNoGCFlags, "Debug::CreateScriptCache");
-  heap->CollectAllGarbage(Heap::kMakeHeapIterableMask,
-                          "Debug::CreateScriptCache");
 
   ASSERT(script_cache_ == NULL);
   script_cache_ = new ScriptCache(isolate_);
@@ -2533,7 +2529,6 @@ void Debug::CreateScriptCache() {
   // Scan heap for Script objects.
   int count = 0;
   HeapIterator iterator(heap);
-  DisallowHeapAllocation no_allocation;
 
   for (HeapObject* obj = iterator.next(); obj != NULL; obj = iterator.next()) {
     if (obj->IsScript() && Script::cast(obj)->HasValidSource()) {
diff --git a/src/heap-profiler.cc b/src/heap-profiler.cc
index 6068bf4..4b7443a 100644
--- a/src/heap-profiler.cc
+++ b/src/heap-profiler.cc
@@ -173,9 +173,6 @@ void HeapProfiler::SetRetainedObjectInfo(UniqueId id,
 
 
 Handle<HeapObject> HeapProfiler::FindHeapObjectById(SnapshotObjectId id) {
-  heap()->CollectAllGarbage(Heap::kMakeHeapIterableMask,
-                            "HeapProfiler::FindHeapObjectById");
-  DisallowHeapAllocation no_allocation;
   HeapObject* object = NULL;
   HeapIterator iterator(heap(), HeapIterator::kFilterUnreachable);
   // Make sure that object with the given id is still reachable.
diff --git a/src/heap-snapshot-generator.cc b/src/heap-snapshot-generator.cc
index cafee77..ef752ff 100644
--- a/src/heap-snapshot-generator.cc
+++ b/src/heap-snapshot-generator.cc
@@ -2569,10 +2569,6 @@ bool HeapSnapshotGenerator::GenerateSnapshot() {
   CHECK(!debug_heap->map_space()->was_swept_conservatively());
 #endif
 
-  // The following code uses heap iterators, so we want the heap to be
-  // stable. It should follow TagGlobalObjects as that can allocate.
-  DisallowHeapAllocation no_alloc;
-
 #ifdef VERIFY_HEAP
   debug_heap->Verify();
 #endif
diff --git a/src/heap.cc b/src/heap.cc
index b672bbf..3679886 100644
--- a/src/heap.cc
+++ b/src/heap.cc
@@ -4321,14 +4321,15 @@ STRUCT_LIST(MAKE_CASE)
 
 bool Heap::IsHeapIterable() {
   return (!old_pointer_space()->was_swept_conservatively() &&
-          !old_data_space()->was_swept_conservatively());
+          !old_data_space()->was_swept_conservatively() &&
+          new_space()->bottom() == new_space()->top());
 }
 
 
-void Heap::EnsureHeapIsIterable() {
+void Heap::MakeHeapIterable() {
   ASSERT(AllowHeapAllocation::IsAllowed());
   if (!IsHeapIterable()) {
-    CollectAllGarbage(kMakeHeapIterableMask, "Heap::EnsureHeapIsIterable");
+    CollectAllGarbage(kMakeHeapIterableMask, "Heap::MakeHeapIterable");
   }
   ASSERT(IsHeapIterable());
 }
@@ -5732,7 +5733,9 @@ class UnreachableObjectsFilter : public HeapObjectsFilter {
 
 
 HeapIterator::HeapIterator(Heap* heap)
-    : heap_(heap),
+    : make_heap_iterable_helper_(heap),
+      no_heap_allocation_(),
+      heap_(heap),
       filtering_(HeapIterator::kNoFiltering),
       filter_(NULL) {
   Init();
@@ -5741,7 +5744,9 @@ HeapIterator::HeapIterator(Heap* heap)
 
 HeapIterator::HeapIterator(Heap* heap,
                            HeapIterator::HeapObjectsFiltering filtering)
-    : heap_(heap),
+    : make_heap_iterable_helper_(heap),
+      no_heap_allocation_(),
+      heap_(heap),
       filtering_(filtering),
       filter_(NULL) {
   Init();
diff --git a/src/heap.h b/src/heap.h
index 9ccbe66..e0ff0c7 100644
--- a/src/heap.h
+++ b/src/heap.h
@@ -767,7 +767,7 @@ class Heap {
 
   // Ensure that we have swept all spaces in such a way that we can iterate
   // over all objects.  May cause a GC.
-  void EnsureHeapIsIterable();
+  void MakeHeapIterable();
 
   // Notify the heap that a context has been disposed.
   int NotifyContextDisposed();
@@ -2391,6 +2391,9 @@ class SpaceIterator : public Malloced {
 // aggregates the specific iterators for the different spaces as
 // these can only iterate over one space only.
 //
+// HeapIterator ensures there is no allocation during its lifetime
+// (using an embedded DisallowHeapAllocation instance).
+//
 // HeapIterator can skip free list nodes (that is, de-allocated heap
 // objects that still remain in the heap). As implementation of free
 // nodes filtering uses GC marks, it can't be used during MS/MC GC
@@ -2413,12 +2416,18 @@ class HeapIterator BASE_EMBEDDED {
   void reset();
 
  private:
+  struct MakeHeapIterableHelper {
+    explicit MakeHeapIterableHelper(Heap* heap) { heap->MakeHeapIterable(); }
+  };
+
   // Perform the initialization.
   void Init();
   // Perform all necessary shutdown (destruction) work.
   void Shutdown();
   HeapObject* NextObject();
 
+  MakeHeapIterableHelper make_heap_iterable_helper_;
+  DisallowHeapAllocation no_heap_allocation_;
   Heap* heap_;
   HeapObjectsFiltering filtering_;
   HeapObjectsFilter* filter_;
diff --git a/src/liveedit.cc b/src/liveedit.cc
index e6bb4b2..a538c0c 100644
--- a/src/liveedit.cc
+++ b/src/liveedit.cc
@@ -939,13 +939,10 @@ static void ReplaceCodeObject(Handle<Code> original,
   // to code objects (that are never in new space) without worrying about
   // write barriers.
   Heap* heap = original->GetHeap();
-  heap->CollectAllGarbage(Heap::kMakeHeapIterableMask,
-                          "liveedit.cc ReplaceCodeObject");
+  HeapIterator iterator(heap);
 
   ASSERT(!heap->InNewSpace(*substitution));
 
-  DisallowHeapAllocation no_allocation;
-
   ReplacingVisitor visitor(*original, *substitution);
 
   // Iterate over all roots. Stack frames may have pointer into original code,
@@ -955,7 +952,6 @@ static void ReplaceCodeObject(Handle<Code> original,
 
   // Now iterate over all pointers of all objects, including code_target
   // implicit pointers.
-  HeapIterator iterator(heap);
   for (HeapObject* obj = iterator.next(); obj != NULL; obj = iterator.next()) {
     obj->Iterate(&visitor);
   }
@@ -1019,8 +1015,6 @@ class LiteralFixer {
   template<typename Visitor>
   static void IterateJSFunctions(SharedFunctionInfo* shared_info,
                                  Visitor* visitor) {
-    DisallowHeapAllocation no_allocation;
-
     HeapIterator iterator(shared_info->GetHeap());
     for (HeapObject* obj = iterator.next(); obj != NULL;
         obj = iterator.next()) {
@@ -1165,7 +1159,7 @@ void LiveEdit::ReplaceFunctionCode(
 
   Handle<SharedFunctionInfo> shared_info = shared_info_wrapper.GetInfo();
 
-  isolate->heap()->EnsureHeapIsIterable();
+  isolate->heap()->MakeHeapIterable();
 
   if (IsJSFunctionCode(shared_info->code())) {
     Handle<Code> code = compile_info_wrapper.GetFunctionCode();
@@ -1402,7 +1396,7 @@ void LiveEdit::PatchFunctionPositions(Handle<JSArray> shared_info_array,
   info->set_end_position(new_function_end);
   info->set_function_token_position(new_function_token_pos);
 
-  info->GetIsolate()->heap()->EnsureHeapIsIterable();
+  info->GetIsolate()->heap()->MakeHeapIterable();
 
   if (IsJSFunctionCode(info->code())) {
     // Patch relocation info section of the code.
diff --git a/src/runtime.cc b/src/runtime.cc
index 99dc6bf..3522609 100644
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -13129,14 +13129,6 @@ RUNTIME_FUNCTION(Runtime_DebugReferencedBy) {
   HandleScope scope(isolate);
   ASSERT(args.length() == 3);
 
-  // First perform a full GC in order to avoid references from dead objects.
-  Heap* heap = isolate->heap();
-  heap->CollectAllGarbage(Heap::kMakeHeapIterableMask, "%DebugReferencedBy");
-  // The heap iterator reserves the right to do a GC to make the heap iterable.
-  // Due to the GC above we know it won't need to do that, but it seems cleaner
-  // to get the heap iterator constructed before we start having unprotected
-  // Object* locals that are not protected by handles.
-
   // Check parameters.
   CONVERT_ARG_HANDLE_CHECKED(JSObject, target, 0);
   CONVERT_ARG_HANDLE_CHECKED(Object, instance_filter, 1);
@@ -13154,21 +13146,27 @@ RUNTIME_FUNCTION(Runtime_DebugReferencedBy) {
 
   // Get the number of referencing objects.
   int count;
-  HeapIterator heap_iterator(heap);
-  count = DebugReferencedBy(&heap_iterator,
-                            *target, *instance_filter, max_references,
-                            NULL, 0, *arguments_function);
+  // First perform a full GC in order to avoid dead objects and to make the heap
+  // iterable.
+  Heap* heap = isolate->heap();
+  heap->CollectAllGarbage(Heap::kMakeHeapIterableMask, "%DebugConstructedBy");
+  {
+    HeapIterator heap_iterator(heap);
+    count = DebugReferencedBy(&heap_iterator,
+                              *target, *instance_filter, max_references,
+                              NULL, 0, *arguments_function);
+  }
 
   // Allocate an array to hold the result.
   Handle<FixedArray> instances = isolate->factory()->NewFixedArray(count);
 
   // Fill the referencing objects.
-  // AllocateFixedArray above does not make the heap non-iterable.
-  ASSERT(heap->IsHeapIterable());
-  HeapIterator heap_iterator2(heap);
-  count = DebugReferencedBy(&heap_iterator2,
-                            *target, *instance_filter, max_references,
-                            *instances, count, *arguments_function);
+  {
+    HeapIterator heap_iterator(heap);
+    count = DebugReferencedBy(&heap_iterator,
+                              *target, *instance_filter, max_references,
+                              *instances, count, *arguments_function);
+  }
 
   // Return result as JS array.
   Handle<JSFunction> constructor(
@@ -13219,9 +13217,6 @@ RUNTIME_FUNCTION(Runtime_DebugConstructedBy) {
   HandleScope scope(isolate);
   ASSERT(args.length() == 2);
 
-  // First perform a full GC in order to avoid dead objects.
-  Heap* heap = isolate->heap();
-  heap->CollectAllGarbage(Heap::kMakeHeapIterableMask, "%DebugConstructedBy");
 
   // Check parameters.
   CONVERT_ARG_HANDLE_CHECKED(JSFunction, constructor, 0);
@@ -13230,24 +13225,31 @@ RUNTIME_FUNCTION(Runtime_DebugConstructedBy) {
 
   // Get the number of referencing objects.
   int count;
-  HeapIterator heap_iterator(heap);
-  count = DebugConstructedBy(&heap_iterator,
-                             *constructor,
-                             max_references,
-                             NULL,
-                             0);
+  // First perform a full GC in order to avoid dead objects and to make the heap
+  // iterable.
+  Heap* heap = isolate->heap();
+  heap->CollectAllGarbage(Heap::kMakeHeapIterableMask, "%DebugConstructedBy");
+  {
+    HeapIterator heap_iterator(heap);
+    count = DebugConstructedBy(&heap_iterator,
+                               *constructor,
+                               max_references,
+                               NULL,
+                               0);
+  }
 
   // Allocate an array to hold the result.
   Handle<FixedArray> instances = isolate->factory()->NewFixedArray(count);
 
-  ASSERT(heap->IsHeapIterable());
   // Fill the referencing objects.
-  HeapIterator heap_iterator2(heap);
-  count = DebugConstructedBy(&heap_iterator2,
-                             *constructor,
-                             max_references,
-                             *instances,
-                             count);
+  {
+    HeapIterator heap_iterator2(heap);
+    count = DebugConstructedBy(&heap_iterator2,
+                               *constructor,
+                               max_references,
+                               *instances,
+                               count);
+  }
 
   // Return result as JS array.
   Handle<JSFunction> array_function(
@@ -13379,8 +13381,6 @@ RUNTIME_FUNCTION(Runtime_LiveEditFindSharedFunctionInfosForScript) {
   int number;
   Heap* heap = isolate->heap();
   {
-    heap->EnsureHeapIsIterable();
-    DisallowHeapAllocation no_allocation;
     HeapIterator heap_iterator(heap);
     Script* scr = *script;
     FixedArray* arr = *array;
@@ -13388,8 +13388,6 @@ RUNTIME_FUNCTION(Runtime_LiveEditFindSharedFunctionInfosForScript) {
   }
   if (number > kBufferSize) {
     array = isolate->factory()->NewFixedArray(number);
-    heap->EnsureHeapIsIterable();
-    DisallowHeapAllocation no_allocation;
     HeapIterator heap_iterator(heap);
     Script* scr = *script;
     FixedArray* arr = *array;
@@ -14472,8 +14470,6 @@ static Handle<Object> Runtime_GetScriptFromScriptName(
   Handle<Script> script;
   Factory* factory = script_name->GetIsolate()->factory();
   Heap* heap = script_name->GetHeap();
-  heap->EnsureHeapIsIterable();
-  DisallowHeapAllocation no_allocation_during_heap_iteration;
   HeapIterator iterator(heap);
   HeapObject* obj = NULL;
   while (script.is_null() && ((obj = iterator.next()) != NULL)) {
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index d7f6602..e8c3b0d 100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -13514,7 +13514,6 @@ THREADED_TEST(LockUnlockLock) {
 
 
 static int GetGlobalObjectsCount() {
-  CcTest::heap()->EnsureHeapIsIterable();
   int count = 0;
   i::HeapIterator it(CcTest::heap());
   for (i::HeapObject* object = it.next(); object != NULL; object = it.next())
diff --git a/test/cctest/test-heap.cc b/test/cctest/test-heap.cc
index 45ec1fc..88d0ee0 100644
--- a/test/cctest/test-heap.cc
+++ b/test/cctest/test-heap.cc
@@ -890,7 +890,6 @@ TEST(StringAllocation) {
 static int ObjectsFoundInHeap(Heap* heap, Handle<Object> objs[], int size) {
   // Count the number of objects found in the heap.
   int found_count = 0;
-  heap->EnsureHeapIsIterable();
   HeapIterator iterator(heap);
   for (HeapObject* obj = iterator.next(); obj != NULL; obj = iterator.next()) {
     for (int i = 0; i < size; i++) {
@@ -1629,9 +1628,8 @@ TEST(TestSizeOfObjects) {
 
 TEST(TestSizeOfObjectsVsHeapIteratorPrecision) {
   CcTest::InitializeVM();
-  CcTest::heap()->EnsureHeapIsIterable();
-  intptr_t size_of_objects_1 = CcTest::heap()->SizeOfObjects();
   HeapIterator iterator(CcTest::heap());
+  intptr_t size_of_objects_1 = CcTest::heap()->SizeOfObjects();
   intptr_t size_of_objects_2 = 0;
   for (HeapObject* obj = iterator.next();
        obj != NULL;
diff --git a/test/cctest/test-object-observe.cc b/test/cctest/test-object-observe.cc
index a7b346f..7c78932 100644
--- a/test/cctest/test-object-observe.cc
+++ b/test/cctest/test-object-observe.cc
@@ -616,7 +616,6 @@ TEST(GetNotifierFromSameOrigin) {
 
 
 static int GetGlobalObjectsCount() {
-  CcTest::heap()->EnsureHeapIsIterable();
   int count = 0;
   i::HeapIterator it(CcTest::heap());
   for (i::HeapObject* object = it.next(); object != NULL; object = it.next())
diff --git a/test/mjsunit/regress/regress-373283.js b/test/mjsunit/regress/regress-373283.js
new file mode 100644
index 0000000..20cee4d
--- /dev/null
+++ b/test/mjsunit/regress/regress-373283.js
@@ -0,0 +1,18 @@
+// Copyright 2014 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags:  --allow-natives-syntax --deopt-every-n-times=1
+
+function __f_0() {
+  var x = [];
+  x[21] = 1;
+  x[21] + 0;
+}
+
+for (var i = 0; i < 3; i++) __f_0();
+%OptimizeFunctionOnNextCall(__f_0);
+for (var i = 0; i < 10; i++) __f_0();
+%OptimizeFunctionOnNextCall(__f_0);
+__f_0();
+%GetScript("foo");
-- 
2.7.4