Fix 2042-reference-processing synchronization

The phantomRefs HashMap was not consistently lock-protected.
Change it to a ConcurrentHashMap instead and remove the lock.

We conjecture this caused the first bug listed below.
We initially thought this might also fix the second one and
tried to remove the TODO and re-enable that part of the test.
Unfortunately that again resulted in a very intermittent local
failure on host.

Add comments to the CC collector to explain a particularly
surprising use of memory_order_relaxed, which I keep having
to rejustify to myself.

Bug: 285914076
Bug: 216481630
Test: Treehugger
Change-Id: I2e5616c5ad0eab59f4a0e89d451e7e34a94448f3

2 files changed, 17 insertions, 15 deletions

diff --git a/runtime/gc/collector/concurrent_copying.cc b/runtime/gc/collector/concurrent_copying.cc
index 3e958719fe..18a4edcbef 100644
--- a/runtime/gc/collector/concurrent_copying.cc
+++ b/runtime/gc/collector/concurrent_copying.cc
@@ -205,6 +205,12 @@ void ConcurrentCopying::MarkHeapReference(mirror::HeapReference<mirror::Object>*
           break;
         }
       } while (!field->CasWeakRelaxed(from_ref, to_ref));
+      // "Relaxed" is not technically sufficient by C++ rules. However, we use a "release"
+      // operation to originally store the forwarding pointer, or a constructor fence if we
+      // directly obtained to_ref from Copy(). We then count on the fact that all later accesses
+      // to the to_ref object are data/address-dependent on the forwarding pointer, and there is
+      // no reasonable way for the compiler to eliminate that depenency. This is very similar to
+      // the reasoning we must use for final fields in any case.
     }
   } else {
     // Used for preserving soft references, should be OK to not have a CAS here since there should be
@@ -3843,6 +3849,7 @@ bool ConcurrentCopying::IsNullOrMarkedHeapReference(mirror::HeapReference<mirror
           break;
         }
       } while (!field->CasWeakRelaxed(from_ref, to_ref));
+      // See comment in MarkHeapReference() for memory ordering.
     } else {
       field->Assign(to_ref);
     }
diff --git a/test/2042-reference-processing/src/Main.java b/test/2042-reference-processing/src/Main.java
index ed670523ea..c7f547fe15 100644
--- a/test/2042-reference-processing/src/Main.java
+++ b/test/2042-reference-processing/src/Main.java
@@ -21,7 +21,7 @@ import java.lang.ref.WeakReference;
 import java.math.BigInteger;
 import java.util.concurrent.ArrayBlockingQueue;
 import java.util.concurrent.atomic.AtomicInteger;
-import java.util.HashMap;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.TreeMap;
 
 /**
@@ -48,7 +48,6 @@ public class Main {
     static volatile boolean pleaseStop;
 
     AtomicInteger totalFinalized = new AtomicInteger(0);
-    Object phantomRefsLock = new Object();
     int maxDropped = 0;
     int liveObjects = 0;
 
@@ -66,7 +65,7 @@ public class Main {
     // Maps from object number to Reference; Cleared references are deleted when queues are
     // processed.
     TreeMap<Integer, MyWeakReference> weakRefs = new TreeMap<>();
-    HashMap<Integer, MyPhantomReference> phantomRefs = new HashMap<>();
+    ConcurrentHashMap<Integer, MyPhantomReference> phantomRefs = new ConcurrentHashMap<>();
 
     class FinalizableObject {
         int n;
@@ -101,16 +100,14 @@ public class Main {
         }
     }
     boolean inPhantomRefs(int n) {
-        synchronized(phantomRefsLock) {
-            MyPhantomReference ref = phantomRefs.get(n);
-            if (ref == null) {
-                return false;
-            }
-            if (ref.n != n) {
-                System.out.println("phantomRef retrieval failed");
-            }
-            return true;
+        MyPhantomReference ref = phantomRefs.get(n);
+        if (ref == null) {
+            return false;
         }
+        if (ref.n != n) {
+            System.out.println("phantomRef retrieval failed");
+        }
+        return true;
     }
 
     void CheckOKToClearWeak(int num) {
@@ -197,9 +194,7 @@ public class Main {
             int me = nextAllocated++;
             listHead = new FinalizableObject(me, listHead);
             weakRefs.put(me, new MyWeakReference(listHead));
-            synchronized(phantomRefsLock) {
-                phantomRefs.put(me, new MyPhantomReference(listHead));
-            }
+            phantomRefs.put(me, new MyPhantomReference(listHead));
         }
         liveObjects += n;
     }