8213231: ThreadSnapshot::_threadObj can become stale

Reviewed-by: dcubed, dholmes, rehn

src/hotspot/share/runtime/vmOperations.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -333,8 +333,7 @@ void VM_ThreadDump::doit() {
       if (_with_locked_synchronizers) {
         tcl = concurrent_locks.thread_concurrent_locks(jt);
       }
-      ThreadSnapshot* ts = snapshot_thread(jt, tcl);
+      snapshot_thread(jt, tcl);
-      _result->add_thread_snapshot(ts);
     }
   } else {
     // Snapshot threads in the given _threads array
@@ -345,7 +344,7 @@ void VM_ThreadDump::doit() {
       if (th() == NULL) {
         // skip if the thread doesn't exist
         // Add a dummy snapshot
-        _result->add_thread_snapshot(new ThreadSnapshot());
+        _result->add_thread_snapshot();
         continue;
       }
 
@@ -362,24 +361,22 @@ void VM_ThreadDump::doit() {
           jt->is_exiting() ||
           jt->is_hidden_from_external_view())  {
         // add a NULL snapshot if skipped
-        _result->add_thread_snapshot(new ThreadSnapshot());
+        _result->add_thread_snapshot();
         continue;
       }
       ThreadConcurrentLocks* tcl = NULL;
       if (_with_locked_synchronizers) {
         tcl = concurrent_locks.thread_concurrent_locks(jt);
       }
-      ThreadSnapshot* ts = snapshot_thread(jt, tcl);
+      snapshot_thread(jt, tcl);
-      _result->add_thread_snapshot(ts);
     }
   }
 }
 
-ThreadSnapshot* VM_ThreadDump::snapshot_thread(JavaThread* java_thread, ThreadConcurrentLocks* tcl) {
+void VM_ThreadDump::snapshot_thread(JavaThread* java_thread, ThreadConcurrentLocks* tcl) {
-  ThreadSnapshot* snapshot = new ThreadSnapshot(_result->t_list(), java_thread);
+  ThreadSnapshot* snapshot = _result->add_thread_snapshot(java_thread);
   snapshot->dump_stack_at_safepoint(_max_depth, _with_locked_monitors);
   snapshot->set_concurrent_locks(tcl);
-  return snapshot;
 }
 
 volatile bool VM_Exit::_vm_exited = false;

src/hotspot/share/runtime/vmOperations.hpp

@@ -445,7 +445,7 @@ class VM_ThreadDump : public VM_Operation {
   bool                           _with_locked_monitors;
   bool                           _with_locked_synchronizers;
 
-  ThreadSnapshot* snapshot_thread(JavaThread* java_thread, ThreadConcurrentLocks* tcl);
+  void snapshot_thread(JavaThread* java_thread, ThreadConcurrentLocks* tcl);
 
  public:
   VM_ThreadDump(ThreadDumpResult* result,

src/hotspot/share/services/management.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -1097,15 +1097,13 @@ JVM_ENTRY(jint, jmm_GetThreadInfo(JNIEnv *env, jlongArray ids, jint maxDepth, jo
     for (int i = 0; i < num_threads; i++) {
       jlong tid = ids_ah->long_at(i);
       JavaThread* jt = dump_result.t_list()->find_JavaThread_from_java_tid(tid);
-      ThreadSnapshot* ts;
       if (jt == NULL) {
         // if the thread does not exist or now it is terminated,
         // create dummy snapshot
-        ts = new ThreadSnapshot();
+        dump_result.add_thread_snapshot();
       } else {
-        ts = new ThreadSnapshot(dump_result.t_list(), jt);
+        dump_result.add_thread_snapshot(jt);
       }
-      dump_result.add_thread_snapshot(ts);
     }
   } else {
     // obtain thread dump with the specific list of threads with stack trace

src/hotspot/share/services/threadService.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -503,8 +503,25 @@ ThreadDumpResult::~ThreadDumpResult() {
   }
 }
 
+ThreadSnapshot* ThreadDumpResult::add_thread_snapshot() {
+  ThreadSnapshot* ts = new ThreadSnapshot();
+  link_thread_snapshot(ts);
+  return ts;
+}
 
-void ThreadDumpResult::add_thread_snapshot(ThreadSnapshot* ts) {
+ThreadSnapshot* ThreadDumpResult::add_thread_snapshot(JavaThread* thread) {
+  // Note: it is very important that the ThreadSnapshot* gets linked before
+  // ThreadSnapshot::initialize gets called. This is to ensure that
+  // ThreadSnapshot::oops_do can get called prior to the field
+  // ThreadSnapshot::_threadObj being assigned a value (to prevent a dangling
+  // oop).
+  ThreadSnapshot* ts = new ThreadSnapshot();
+  link_thread_snapshot(ts);
+  ts->initialize(t_list(), thread);
+  return ts;
+}
+
+void ThreadDumpResult::link_thread_snapshot(ThreadSnapshot* ts) {
   assert(_num_threads == 0 || _num_snapshots < _num_threads,
          "_num_snapshots must be less than _num_threads");
   _num_snapshots++;
@@ -831,12 +848,9 @@ ThreadStatistics::ThreadStatistics() {
   memset((void*) _perf_recursion_counts, 0, sizeof(_perf_recursion_counts));
 }
 
-ThreadSnapshot::ThreadSnapshot(ThreadsList * t_list, JavaThread* thread) {
+void ThreadSnapshot::initialize(ThreadsList * t_list, JavaThread* thread) {
   _thread = thread;
   _threadObj = thread->threadObj();
-  _stack_trace = NULL;
-  _concurrent_locks = NULL;
-  _next = NULL;
 
   ThreadStatistics* stat = thread->get_thread_stat();
   _contended_enter_ticks = stat->contended_enter_ticks();
@@ -846,9 +860,6 @@ ThreadSnapshot::ThreadSnapshot(ThreadsList * t_list, JavaThread* thread) {
   _sleep_ticks = stat->sleep_ticks();
   _sleep_count = stat->sleep_count();
 
-  _blocker_object = NULL;
-  _blocker_object_owner = NULL;
-
   _thread_status = java_lang_Thread::get_thread_status(_threadObj);
   _is_ext_suspended = thread->is_being_ext_suspended();
   _is_in_native = (thread->thread_state() == _thread_in_native);

src/hotspot/share/services/threadService.hpp

@@ -213,12 +213,15 @@ private:
   ThreadConcurrentLocks* _concurrent_locks;
   ThreadSnapshot*        _next;
 
-public:
+  // ThreadSnapshot instances should only be created via
-  // Dummy snapshot
+  // ThreadDumpResult::add_thread_snapshot.
+  friend class ThreadDumpResult;
   ThreadSnapshot() : _thread(NULL), _threadObj(NULL),
                      _blocker_object(NULL), _blocker_object_owner(NULL),
                      _stack_trace(NULL), _concurrent_locks(NULL), _next(NULL) {};
-  ThreadSnapshot(ThreadsList * t_list, JavaThread* thread);
+  void        initialize(ThreadsList * t_list, JavaThread* thread);
+
+public:
   ~ThreadSnapshot();
 
   java_lang_Thread::ThreadStatus thread_status() { return _thread_status; }
@@ -367,12 +370,16 @@ class ThreadDumpResult : public StackObj {
   ThreadsListSetter    _setter;  // Helper to set hazard ptr in the originating thread
                                  // which protects the JavaThreads in _snapshots.
 
+  void                 link_thread_snapshot(ThreadSnapshot* ts);
+
  public:
   ThreadDumpResult();
   ThreadDumpResult(int num_threads);
   ~ThreadDumpResult();
 
-  void                 add_thread_snapshot(ThreadSnapshot* ts);
+  ThreadSnapshot*      add_thread_snapshot();
+  ThreadSnapshot*      add_thread_snapshot(JavaThread* thread);
+
   void                 set_next(ThreadDumpResult* next) { _next = next; }
   ThreadDumpResult*    next()                           { return _next; }
   int                  num_threads()                    { return _num_threads; }