8276696: ParallelObjectIterator freed at the wrong time in VM_HeapDumper

Reviewed-by: ogillespie, phh
Backport-of: f4dc03ea6de327425ff265c3d2ec16ea7b0e1634

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

@@ -2267,7 +2267,7 @@ void G1CollectedHeap::object_iterate(ObjectClosure* cl) {
   heap_region_iterate(&blk);
 }
 
-class G1ParallelObjectIterator : public ParallelObjectIterator {
+class G1ParallelObjectIterator : public ParallelObjectIteratorImpl {
 private:
   G1CollectedHeap*  _heap;
   HeapRegionClaimer _claimer;
@@ -2282,7 +2282,7 @@ class G1ParallelObjectIterator : public ParallelObjectIterator {
   }
 };
 
-ParallelObjectIterator* G1CollectedHeap::parallel_object_iterator(uint thread_num) {
+ParallelObjectIteratorImpl* G1CollectedHeap::parallel_object_iterator(uint thread_num) {
   return new G1ParallelObjectIterator(thread_num);
 }
 

src/hotspot/share/gc/g1/g1CollectedHeap.hpp

@@ -1205,7 +1205,7 @@ class G1CollectedHeap : public CollectedHeap {
   // Iterate over all objects, calling "cl.do_object" on each.
   virtual void object_iterate(ObjectClosure* cl);
 
-  virtual ParallelObjectIterator* parallel_object_iterator(uint thread_num);
+  virtual ParallelObjectIteratorImpl* parallel_object_iterator(uint thread_num);
 
   // Keep alive an object that was loaded with AS_NO_KEEPALIVE.
   virtual void keep_alive(oop obj);

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp

@@ -588,7 +588,7 @@ void ParallelScavengeHeap::object_iterate_parallel(ObjectClosure* cl,
   }
 }
 
-class PSScavengeParallelObjectIterator : public ParallelObjectIterator {
+class PSScavengeParallelObjectIterator : public ParallelObjectIteratorImpl {
 private:
   ParallelScavengeHeap*  _heap;
   HeapBlockClaimer      _claimer;
@@ -603,7 +603,7 @@ class PSScavengeParallelObjectIterator : public ParallelObjectIterator {
   }
 };
 
-ParallelObjectIterator* ParallelScavengeHeap::parallel_object_iterator(uint thread_num) {
+ParallelObjectIteratorImpl* ParallelScavengeHeap::parallel_object_iterator(uint thread_num) {
   return new PSScavengeParallelObjectIterator();
 }
 

src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp

@@ -211,7 +211,7 @@ class ParallelScavengeHeap : public CollectedHeap {
 
   void object_iterate(ObjectClosure* cl);
   void object_iterate_parallel(ObjectClosure* cl, HeapBlockClaimer* claimer);
-  virtual ParallelObjectIterator* parallel_object_iterator(uint thread_num);
+  virtual ParallelObjectIteratorImpl* parallel_object_iterator(uint thread_num);
 
   HeapWord* block_start(const void* addr) const;
   bool block_is_obj(const HeapWord* addr) const;

src/hotspot/share/gc/shared/collectedHeap.cpp

@@ -110,6 +110,18 @@ void GCHeapLog::log_heap(CollectedHeap* heap, bool before) {
   st.print_cr("}");
 }
 
+ParallelObjectIterator::ParallelObjectIterator(uint thread_num) :
+  _impl(Universe::heap()->parallel_object_iterator(thread_num))
+{}
+
+ParallelObjectIterator::~ParallelObjectIterator() {
+  delete _impl;
+}
+
+void ParallelObjectIterator::object_iterate(ObjectClosure* cl, uint worker_id) {
+  _impl->object_iterate(cl, worker_id);
+}
+
 size_t CollectedHeap::unused() const {
   MutexLocker ml(Heap_lock);
   return capacity() - used();

src/hotspot/share/gc/shared/collectedHeap.hpp

@@ -62,10 +62,23 @@ class VirtualSpaceSummary;
 class WorkGang;
 class nmethod;
 
-class ParallelObjectIterator : public CHeapObj<mtGC> {
+class ParallelObjectIteratorImpl : public CHeapObj<mtGC> {
 public:
+  virtual ~ParallelObjectIteratorImpl() {}
   virtual void object_iterate(ObjectClosure* cl, uint worker_id) = 0;
-  virtual ~ParallelObjectIterator() {}
+};
+
+// User facing parallel object iterator. This is a StackObj, which ensures that
+// the _impl is allocated and deleted in the scope of this object. This ensures
+// the life cycle of the implementation is as required by ThreadsListHandle,
+// which is sometimes used by the root iterators.
+class ParallelObjectIterator : public StackObj {
+  ParallelObjectIteratorImpl* _impl;
+
+public:
+  ParallelObjectIterator(uint thread_num);
+  ~ParallelObjectIterator();
+  void object_iterate(ObjectClosure* cl, uint worker_id);
 };
 
 //
@@ -82,6 +95,7 @@ class CollectedHeap : public CHeapObj<mtGC> {
   friend class JVMCIVMStructs;
   friend class IsGCActiveMark; // Block structured external access to _is_gc_active
   friend class MemAllocator;
+  friend class ParallelObjectIterator;
 
  private:
   GCHeapLog* _gc_heap_log;
@@ -382,10 +396,12 @@ class CollectedHeap : public CHeapObj<mtGC> {
   // Iterate over all objects, calling "cl.do_object" on each.
   virtual void object_iterate(ObjectClosure* cl) = 0;
 
-  virtual ParallelObjectIterator* parallel_object_iterator(uint thread_num) {
+ protected:
+  virtual ParallelObjectIteratorImpl* parallel_object_iterator(uint thread_num) {
     return NULL;
   }
 
+ public:
   // Keep alive an object that was loaded with AS_NO_KEEPALIVE.
   virtual void keep_alive(oop obj) {}
 

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

@@ -1374,7 +1374,7 @@ class ShenandoahObjectIterateParScanClosure : public BasicOopIterateClosure {
 // parallel marking queues.
 // Every worker processes it's own marking queue. work-stealing is used
 // to balance workload.
-class ShenandoahParallelObjectIterator : public ParallelObjectIterator {
+class ShenandoahParallelObjectIterator : public ParallelObjectIteratorImpl {
 private:
   uint                         _num_workers;
   bool                         _init_ready;
@@ -1474,7 +1474,7 @@ class ShenandoahParallelObjectIterator : public ParallelObjectIterator {
   }
 };
 
-ParallelObjectIterator* ShenandoahHeap::parallel_object_iterator(uint workers) {
+ParallelObjectIteratorImpl* ShenandoahHeap::parallel_object_iterator(uint workers) {
   return new ShenandoahParallelObjectIterator(workers, &_aux_bit_map);
 }
 

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp

@@ -479,7 +479,7 @@ class ShenandoahHeap : public CollectedHeap {
   // Used for native heap walkers: heap dumpers, mostly
   void object_iterate(ObjectClosure* cl);
   // Parallel heap iteration support
-  virtual ParallelObjectIterator* parallel_object_iterator(uint workers);
+  virtual ParallelObjectIteratorImpl* parallel_object_iterator(uint workers);
 
   // Keep alive an object that was loaded with AS_NO_KEEPALIVE.
   void keep_alive(oop obj);

src/hotspot/share/gc/z/zCollectedHeap.cpp

@@ -243,7 +243,7 @@ void ZCollectedHeap::object_iterate(ObjectClosure* cl) {
   _heap.object_iterate(cl, true /* visit_weaks */);
 }
 
-ParallelObjectIterator* ZCollectedHeap::parallel_object_iterator(uint nworkers) {
+ParallelObjectIteratorImpl* ZCollectedHeap::parallel_object_iterator(uint nworkers) {
   return _heap.parallel_object_iterator(nworkers, true /* visit_weaks */);
 }
 

src/hotspot/share/gc/z/zCollectedHeap.hpp

@@ -95,7 +95,7 @@ class ZCollectedHeap : public CollectedHeap {
   virtual GrowableArray<MemoryPool*> memory_pools();
 
   virtual void object_iterate(ObjectClosure* cl);
-  virtual ParallelObjectIterator* parallel_object_iterator(uint nworkers);
+  virtual ParallelObjectIteratorImpl* parallel_object_iterator(uint nworkers);
 
   virtual void keep_alive(oop obj);
 

src/hotspot/share/gc/z/zHeap.cpp

@@ -439,7 +439,7 @@ void ZHeap::object_iterate(ObjectClosure* cl, bool visit_weaks) {
   iter.object_iterate(cl, 0 /* worker_id */);
 }
 
-ParallelObjectIterator* ZHeap::parallel_object_iterator(uint nworkers, bool visit_weaks) {
+ParallelObjectIteratorImpl* ZHeap::parallel_object_iterator(uint nworkers, bool visit_weaks) {
   assert(SafepointSynchronize::is_at_safepoint(), "Should be at safepoint");
   return new ZHeapIterator(nworkers, visit_weaks);
 }

src/hotspot/share/gc/z/zHeap.hpp

@@ -141,7 +141,7 @@ class ZHeap {
 
   // Iteration
   void object_iterate(ObjectClosure* cl, bool visit_weaks);
-  ParallelObjectIterator* parallel_object_iterator(uint nworkers, bool visit_weaks);
+  ParallelObjectIteratorImpl* parallel_object_iterator(uint nworkers, bool visit_weaks);
   void pages_do(ZPageClosure* cl);
 
   // Serviceability

src/hotspot/share/gc/z/zHeapIterator.hpp

@@ -42,7 +42,7 @@ using ZHeapIteratorQueues = GenericTaskQueueSet<ZHeapIteratorQueue, mtGC>;
 using ZHeapIteratorArrayQueue = OverflowTaskQueue<ObjArrayTask, mtGC>;
 using ZHeapIteratorArrayQueues = GenericTaskQueueSet<ZHeapIteratorArrayQueue, mtGC>;
 
-class ZHeapIterator : public ParallelObjectIterator {
+class ZHeapIterator : public ParallelObjectIteratorImpl {
   friend class ZHeapIteratorContext;
 
 private:

src/hotspot/share/memory/heapInspection.cpp

@@ -584,18 +584,12 @@ uintx HeapInspection::populate_table(KlassInfoTable* cit, BoolObjectClosure *fil
       // Can't run with more threads than provided by the WorkGang.
       WithUpdatedActiveWorkers update_and_restore(gang, parallel_thread_num);
 
-      ParallelObjectIterator* poi = Universe::heap()->parallel_object_iterator(gang->active_workers());
-      if (poi != NULL) {
-        // The GC supports parallel object iteration.
-
-        ParHeapInspectTask task(poi, cit, filter);
-        // Run task with the active workers.
-        gang->run_task(&task);
-
-        delete poi;
-        if (task.success()) {
-          return task.missed_count();
-        }
+      ParallelObjectIterator poi(gang->active_workers());
+      ParHeapInspectTask task(&poi, cit, filter);
+      // Run task with the active workers.
+      gang->run_task(&task);
+      if (task.success()) {
+        return task.missed_count();
       }
     }
   }