Don't re-read referent in ReferenceProcessor::GetReferent

Re-reading has the issue that it may read a null value after already
having done the null check. Using a cached value prevents this from
happening and causing DCHECK failures.

Added a related stress test.

Bug: 33569625
Bug: 33389022

Test: test-art-host

Change-Id: Ic42d540e035d41ac6e5b01762f9510cd6632b28c

diff --git a/runtime/gc/reference_processor.cc b/runtime/gc/reference_processor.cc
index 2cde7d5..641a919 100644 (file)
--- a/runtime/gc/reference_processor.cc
+++ b/runtime/gc/reference_processor.cc
@@ -75,11 +75,10 @@ ObjPtr<mirror::Object> ReferenceProcessor::GetReferent(Thread* self,
   MutexLock mu(self, *Locks::reference_processor_lock_);
   while ((!kUseReadBarrier && SlowPathEnabled()) ||
          (kUseReadBarrier && !self->GetWeakRefAccessEnabled())) {
-    mirror::HeapReference<mirror::Object>* const referent_addr =
-        reference->GetReferentReferenceAddr();
+    ObjPtr<mirror::Object> referent = reference->GetReferent<kWithoutReadBarrier>();
     // If the referent became cleared, return it. Don't need barrier since thread roots can't get
     // updated until after we leave the function due to holding the mutator lock.
-    if (referent_addr->AsMirrorPtr() == nullptr) {
+    if (referent == nullptr) {
       return nullptr;
     }
     // Try to see if the referent is already marked by using the is_marked_callback. We can return
@@ -91,10 +90,15 @@ ObjPtr<mirror::Object> ReferenceProcessor::GetReferent(Thread* self,
       // case only black nodes can be safely returned. If the GC is preserving references, the
       // mutator could take a white field from a grey or white node and move it somewhere else
       // in the heap causing corruption since this field would get swept.
-      if (collector_->IsMarkedHeapReference(referent_addr)) {
+      // Use the cached referent instead of calling GetReferent since other threads could call
+      // Reference.clear() after we did the null check resulting in a null pointer being
+      // incorrectly passed to IsMarked. b/33569625
+      ObjPtr<mirror::Object> forwarded_ref = collector_->IsMarked(referent.Ptr());
+      if (forwarded_ref != nullptr) {
+        // Non null means that it is marked.
         if (!preserving_references_ ||
            (LIKELY(!reference->IsFinalizerReferenceInstance()) && reference->IsUnprocessed())) {
-          return referent_addr->AsMirrorPtr();
+          return forwarded_ref;
         }
       }
     }

diff --git a/test/153-reference-stress/expected.txt b/test/153-reference-stress/expected.txt
new file mode 100644 (file)
index 0000000..7ef22e9
--- /dev/null
+++ b/test/153-reference-stress/expected.txt
@@ -0,0 +1 @@
+PASS

diff --git a/test/153-reference-stress/info.txt b/test/153-reference-stress/info.txt
new file mode 100644 (file)
index 0000000..6bc0040
--- /dev/null
+++ b/test/153-reference-stress/info.txt
@@ -0,0 +1 @@
+Tests java.lang.ref.Reference.get() and GC running in parallel.

diff --git a/test/153-reference-stress/src/Main.java b/test/153-reference-stress/src/Main.java
new file mode 100644 (file)
index 0000000..fc6f9cc
--- /dev/null
+++ b/test/153-reference-stress/src/Main.java
@@ -0,0 +1,73 @@
+/*
+ * Copyright (C) 2016 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.lang.ref.WeakReference;
+
+public class Main {
+    static final int numWeakReferences = 16 * 1024;
+    static WeakReference[] weakReferences = new WeakReference[numWeakReferences];
+    static volatile boolean done = false;
+    static Object keepAlive;
+
+    public static void main(String[] args) throws Exception {
+        // Try to call Reference.get repeatedly while the GC is running.
+        Thread gcThread = new GcThread();
+        Thread[] readerThread = new ReaderThread[4];
+        for (int i = 0; i < readerThread.length; ++i) {
+            readerThread[i] = new ReaderThread();
+        }
+        gcThread.start();
+        for (int i = 0; i < readerThread.length; ++i) {
+            readerThread[i].start();
+        }
+        gcThread.join();
+        for (int i = 0; i < readerThread.length; ++i) {
+            readerThread[i].join();
+        }
+        System.out.println("PASS");
+    }
+
+    static class GcThread extends Thread {
+        GcThread() {
+            Object temp = new Object();
+            for (int j = 0; j < weakReferences.length; ++j) {
+                weakReferences[j] = new WeakReference(temp);
+            }
+        }
+        public void run() {
+            for (int i = 0; i < 1000; ++i) {
+                Object o = new Object();
+                for (int j = 0; j < weakReferences.length; ++j) {
+                    weakReferences[j] = new WeakReference(o);
+                }
+            }
+            done = true;
+        }
+    }
+
+    static class ReaderThread extends Thread {
+        public void run() {
+            while (!done) {
+                for (int j = 0; j < weakReferences.length; ++j) {
+                    keepAlive = weakReferences[j].get();
+                }
+                for (int j = 0; j < weakReferences.length; ++j) {
+                    weakReferences[j].clear();
+                }
+            }
+        }
+    }
+}