From 36994ba006c18c1933815cc0c4c036df086e6814 Mon Sep 17 00:00:00 2001 From: Mathieu Chartier Date: Tue, 13 Dec 2016 11:46:28 -0800 Subject: [PATCH] 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 --- runtime/gc/reference_processor.cc | 14 ++++--- test/153-reference-stress/expected.txt | 1 + test/153-reference-stress/info.txt | 1 + test/153-reference-stress/src/Main.java | 73 +++++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 test/153-reference-stress/expected.txt create mode 100644 test/153-reference-stress/info.txt create mode 100644 test/153-reference-stress/src/Main.java diff --git a/runtime/gc/reference_processor.cc b/runtime/gc/reference_processor.cc index 2cde7d573..641a91950 100644 --- a/runtime/gc/reference_processor.cc +++ b/runtime/gc/reference_processor.cc @@ -75,11 +75,10 @@ ObjPtr ReferenceProcessor::GetReferent(Thread* self, MutexLock mu(self, *Locks::reference_processor_lock_); while ((!kUseReadBarrier && SlowPathEnabled()) || (kUseReadBarrier && !self->GetWeakRefAccessEnabled())) { - mirror::HeapReference* const referent_addr = - reference->GetReferentReferenceAddr(); + ObjPtr referent = reference->GetReferent(); // 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 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 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 index 000000000..7ef22e9a4 --- /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 index 000000000..6bc00404a --- /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 index 000000000..fc6f9ccb3 --- /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(); + } + } + } + } +} -- 2.11.0