8256517: (ref) Reference.clear during reference processing may lose notification

8240696: (ref) Reference.clear may extend the lifetime of the referent

Use private native helper to implement Reference.clear.

Reviewed-by: pliden, rkennke, mchung

Author: Kim Barrett

make/hotspot/symbols/symbols-unix

@@ -177,6 +177,7 @@ JVM_RawMonitorCreate
 JVM_RawMonitorDestroy
 JVM_RawMonitorEnter
 JVM_RawMonitorExit
+JVM_ReferenceClear
 JVM_ReferenceRefersTo
 JVM_RegisterLambdaProxyClassForArchiving
 JVM_RegisterSignal

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

@@ -57,3 +57,7 @@ void ZBreakpoint::at_after_marking_started() {
 void ZBreakpoint::at_before_marking_completed() {
   ConcurrentGCBreakpoints::at("BEFORE MARKING COMPLETED");
 }
+
+void ZBreakpoint::at_after_reference_processing_started() {
+  ConcurrentGCBreakpoints::at("AFTER CONCURRENT REFERENCE PROCESSING STARTED");
+}

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

@@ -37,6 +37,7 @@ class ZBreakpoint : public AllStatic {
   static void at_after_gc();
   static void at_after_marking_started();
   static void at_before_marking_completed();
+  static void at_after_reference_processing_started();
 };
 
 #endif // SHARE_GC_Z_ZBREAKPOINT_HPP

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

@@ -322,6 +322,7 @@ void ZDriver::concurrent_mark_continue() {
 
 void ZDriver::concurrent_process_non_strong_references() {
   ZStatTimer timer(ZPhaseConcurrentProcessNonStrongReferences);
+  ZBreakpoint::at_after_reference_processing_started();
   ZHeap::heap()->process_non_strong_references();
 }
 

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

@@ -183,12 +183,6 @@ bool ZReferenceProcessor::should_discover(oop reference, ReferenceType type) con
 }
 
 bool ZReferenceProcessor::should_drop(oop reference, ReferenceType type) const {
-  // This check is racing with a call to Reference.clear() from the application.
-  // If the application clears the reference after this check it will still end
-  // up on the pending list, and there's nothing we can do about that without
-  // changing the Reference.clear() API. This check is also racing with a call
-  // to Reference.enqueue() from the application, which is unproblematic, since
-  // the application wants the reference to be enqueued anyway.
   const oop referent = reference_referent(reference);
   if (referent == NULL) {
     // Reference has been cleared, by a call to Reference.enqueue()

src/hotspot/share/include/jvm.h

@@ -333,6 +333,9 @@ JVM_WaitForReferencePendingList(JNIEnv *env);
 JNIEXPORT jboolean JNICALL
 JVM_ReferenceRefersTo(JNIEnv *env, jobject ref, jobject o);
 
+JNIEXPORT void JNICALL
+JVM_ReferenceClear(JNIEnv *env, jobject ref);
+
 /*
  * java.lang.ref.PhantomReference
  */

src/hotspot/share/prims/jvm.cpp

@@ -3446,6 +3446,25 @@ JVM_ENTRY(jboolean, JVM_ReferenceRefersTo(JNIEnv* env, jobject ref, jobject o))
   return referent == JNIHandles::resolve(o);
 JVM_END
 
+JVM_ENTRY(void, JVM_ReferenceClear(JNIEnv* env, jobject ref))
+  JVMWrapper("JVM_ReferenceClear");
+  oop ref_oop = JNIHandles::resolve_non_null(ref);
+  // FinalReference has it's own implementation of clear().
+  assert(!java_lang_ref_Reference::is_final(ref_oop), "precondition");
+  if (java_lang_ref_Reference::unknown_referent_no_keepalive(ref_oop) == NULL) {
+    // If the referent has already been cleared then done.
+    // However, if the referent is dead but has not yet been cleared by
+    // concurrent reference processing, it should NOT be cleared here.
+    // Instead, clearing should be left to the GC.  Clearing it here could
+    // detectably lose an expected notification, which is impossible with
+    // STW reference processing.  The clearing in enqueue() doesn't have
+    // this problem, since the enqueue covers the notification, but it's not
+    // worth the effort to handle that case specially.
+    return;
+  }
+  java_lang_ref_Reference::clear_referent(ref_oop);
+JVM_END
+
 
 // java.lang.ref.PhantomReference //////////////////////////////////////////////////
 

src/java.base/share/classes/java/lang/ref/FinalReference.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2020, 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
@@ -34,6 +34,22 @@ public FinalReference(T referent, ReferenceQueue<? super T> q) {
         super(referent, q);
     }
 
+    /* May only be called when the reference is inactive, so no longer weak. */
+    @Override
+    public T get() {
+        // Cannot call super.get() when active, as the GC could
+        // deactivate immediately after the test.
+        return getFromInactiveFinalReference();
+    }
+
+    /* May only be called when the reference is inactive, so no longer weak.
+     * Clearing while active would discard the finalization request.
+     */
+    @Override
+    public void clear() {
+        clearInactiveFinalReference();
+    }
+
     @Override
     public boolean enqueue() {
         throw new InternalError("should never reach here");

src/java.base/share/classes/java/lang/ref/Finalizer.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2020, 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
@@ -82,7 +82,7 @@ private void runFinalizer(JavaLangAccess jla) {
         }
 
         try {
-            Object finalizee = this.getInactive();
+            Object finalizee = this.get();
             assert finalizee != null;
             if (!(finalizee instanceof java.lang.Enum)) {
                 jla.invokeFinalize(finalizee);

src/java.base/share/classes/java/lang/ref/Reference.java

@@ -100,10 +100,10 @@ public abstract class Reference<T> {
      *   [active/unregistered] [1]
      *
      * Transitions:
-     *                            clear
+     *                            clear [2]
      *   [active/registered]     ------->   [inactive/registered]
      *          |                                 |
-     *          |                                 | enqueue [2]
+     *          |                                 | enqueue
      *          | GC              enqueue [2]     |
      *          |                -----------------|
      *          |                                 |
@@ -114,7 +114,7 @@ public abstract class Reference<T> {
      *          v                   |             |
      *   [pending/enqueued]      ---              |
      *          |                                 | poll/remove
-     *          | poll/remove                     |
+     *          | poll/remove                     | + clear [4]
      *          |                                 |
      *          v            ReferenceHandler     v
      *   [pending/dequeued]      ------>    [inactive/dequeued]
@@ -140,12 +140,14 @@ public abstract class Reference<T> {
      * [1] Unregistered is not permitted for FinalReferences.
      *
      * [2] These transitions are not possible for FinalReferences, making
-     * [pending/enqueued] and [pending/dequeued] unreachable, and
-     * [inactive/registered] terminal.
+     * [pending/enqueued], [pending/dequeued], and [inactive/registered]
+     * unreachable.
      *
      * [3] The garbage collector may directly transition a Reference
      * from [active/unregistered] to [inactive/unregistered],
      * bypassing the pending-Reference list.
+     *
+     * [4] The queue handler for FinalReferences also clears the reference.
      */
 
     private T referent;         /* Treated specially by GC */
@@ -342,22 +344,6 @@ public T get() {
         return this.referent;
     }
 
-    /**
-     * Load referent with strong semantics. Treating the referent
-     * as strong referent is ok when the Reference is inactive,
-     * because then the referent is switched to strong semantics
-     * anyway.
-     *
-     * This is only used from Finalizer to bypass the intrinsic,
-     * which might return a null referent, even though it is not
-     * null, and would subsequently not finalize the referent/finalizee.
-     */
-    T getInactive() {
-        assert this instanceof FinalReference;
-        assert next == this; // I.e. FinalReference is inactive
-        return this.referent;
-    }
-
     /**
      * Tests if the referent of this reference object is {@code obj}.
      * Using a {@code null} {@code obj} returns {@code true} if the
@@ -383,6 +369,41 @@ public final boolean refersTo(T obj) {
      * clears references it does so directly, without invoking this method.
      */
     public void clear() {
+        clear0();
+    }
+
+    /* Implementation of clear(), also used by enqueue().  A simple
+     * assignment of the referent field won't do for some garbage
+     * collectors.
+     */
+    private native void clear0();
+
+    /* -- Operations on inactive FinalReferences -- */
+
+    /* These functions are only used by FinalReference, and must only be
+     * called after the reference becomes inactive. While active, a
+     * FinalReference is considered weak but the referent is not normally
+     * accessed. Once a FinalReference becomes inactive it is considered a
+     * strong reference. These functions are used to bypass the
+     * corresponding weak implementations, directly accessing the referent
+     * field with strong semantics.
+     */
+
+    /**
+     * Load referent with strong semantics.
+     */
+    T getFromInactiveFinalReference() {
+        assert this instanceof FinalReference;
+        assert next != null; // I.e. FinalReference is inactive
+        return this.referent;
+    }
+
+    /**
+     * Clear referent with strong semantics.
+     */
+    void clearInactiveFinalReference() {
+        assert this instanceof FinalReference;
+        assert next != null; // I.e. FinalReference is inactive
         this.referent = null;
     }
 
@@ -413,7 +434,7 @@ public boolean isEnqueued() {
      *           it was not registered with a queue when it was created
      */
     public boolean enqueue() {
-        this.referent = null;
+        clear0();               // Intentionally clear0() rather than clear()
         return this.queue.enqueue(this);
     }