From 1cc692dd7de54bfffa05813a90b14670b4eb50d1 Mon Sep 17 00:00:00 2001
From: almson <almson@users.noreply.github.com>
Date: Wed, 24 Oct 2018 16:15:13 -0400
Subject: [PATCH] Fix incorrect reachability assumption in ResourceLeakDetector
 (#8410)

Motivation:

trackedObject != null gives no guarantee that trackedObject remains reachable. This may cause problems related to premature finalization: false leak detector warnings.

Modifications:

Add private method reachabilityFence0 that works on JDK 8 and can be factored out into PlatformDependent. Later, it can be swapped for the real Reference.reachabilityFence.

Result:

No false leak detector warnings in future versions of JDK.
---
 .../io/netty/util/ResourceLeakDetector.java   | 40 ++++++++++++++++---
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/common/src/main/java/io/netty/util/ResourceLeakDetector.java b/common/src/main/java/io/netty/util/ResourceLeakDetector.java
index da022b01b7..5f26926cf7 100644
--- a/common/src/main/java/io/netty/util/ResourceLeakDetector.java
+++ b/common/src/main/java/io/netty/util/ResourceLeakDetector.java
@@ -461,11 +461,41 @@ public class ResourceLeakDetector<T> {
             // Ensure that the object that was tracked is the same as the one that was passed to close(...).
             assert trackedHash == System.identityHashCode(trackedObject);
 
-            // We need to actually do the null check of the trackedObject after we close the leak because otherwise
-            // we may get false-positives reported by the ResourceLeakDetector. This can happen as the JIT / GC may
-            // be able to figure out that we do not need the trackedObject anymore and so already enqueue it for
-            // collection before we actually get a chance to close the enclosing ResourceLeak.
-            return close() && trackedObject != null;
+            try {
+                return close();
+            } finally {
+                // This method will do `synchronized(trackedObject)` and we should be sure this will not cause deadlock.
+                // It should not, because somewhere up the callstack should be a (successful) `trackedObject.release`,
+                // therefore it is unreasonable that anyone else, anywhere, is holding a lock on the trackedObject.
+                // (Unreasonable but possible, unfortunately.)
+                reachabilityFence0(trackedObject);
+            }
+        }
+
+         /**
+         * Ensures that the object referenced by the given reference remains
+         * <a href="package-summary.html#reachability"><em>strongly reachable</em></a>,
+         * regardless of any prior actions of the program that might otherwise cause
+         * the object to become unreachable; thus, the referenced object is not
+         * reclaimable by garbage collection at least until after the invocation of
+         * this method.
+         *
+         * <p> Recent versions of the JDK have a nasty habit of prematurely deciding objects are unreachable.
+         * see: https://stackoverflow.com/questions/26642153/finalize-called-on-strongly-reachable-object-in-java-8
+         * The Java 9 method Reference.reachabilityFence offers a solution to this problem.
+         *
+         * <p> This method is always implemented as a synchronization on {@code ref}, not as
+         * {@code Reference.reachabilityFence} for consistency across platforms and to allow building on JDK 6-8.
+         * <b>It is the caller's responsibility to ensure that this synchronization will not cause deadlock.</b>
+         *
+         * @param ref the reference. If {@code null}, this method has no effect.
+         * @see java.lang.ref.Reference#reachabilityFence
+         */
+        private static void reachabilityFence0(Object ref) {
+            if (ref != null) {
+                // Empty synchronized is ok: https://stackoverflow.com/a/31933260/1151521
+                synchronized (ref) { }
+            }
         }
 
         @Override