[BREAKING] Use event class instead of event for timings. Fixes BUKKIT-4664

TimedRegisteredListener uses a reference to the first event fired. This
causes a memory leak in the server for any references that event has. This
changes TimedRegisteredListener to only store a reference to the class of
the event.

This change is intentionally a breaking change, as it is an obscure part
of the API. A non-breaking change would require the leak to be maintained
or an immediate update for any plugins using the method, as it would be an
indirect break.

A unit test is also included to check behavior of shared superclass
functionality.

By: Score_Under <seejay.11@gmail.com>

paper-api/src/main/java/org/bukkit/command/defaults/TimingsCommand.java

@@ -84,9 +84,9 @@ public class TimingsCommand extends BukkitCommand {
                             if (count == 0) continue;
                             long avg = time / count;
                             totalTime += time;
-                            Event event = trl.getEvent();
+                            Class<? extends Event> eventClass = trl.getEventClass();
-                            if (count > 0 && event != null) {
+                            if (count > 0 && eventClass != null) {
-                                fileTimings.println("    " + event.getClass().getSimpleName() + (trl.hasMultiple() ? " (and others)" : "") + " Time: " + time + " Count: " + count + " Avg: " + avg);
+                                fileTimings.println("    " + eventClass.getSimpleName() + (trl.hasMultiple() ? " (and sub-classes)" : "") + " Time: " + time + " Count: " + count + " Avg: " + avg);
                             }
                         }
                     }

paper-api/src/main/java/org/bukkit/plugin/TimedRegisteredListener.java

@@ -11,7 +11,7 @@ import org.bukkit.event.Listener;
 public class TimedRegisteredListener extends RegisteredListener {
     private int count;
     private long totalTime;
-    private Event event;
+    private Class<? extends Event> eventClass;
     private boolean multiple = false;
 
     public TimedRegisteredListener(final Listener pluginListener, final EventExecutor eventExecutor, final EventPriority eventPriority, final Plugin registeredPlugin, final boolean listenCancelled) {
@@ -25,17 +25,25 @@ public class TimedRegisteredListener extends RegisteredListener {
             return;
         }
         count++;
-        if (this.event == null) {
+        Class<? extends Event> newEventClass = event.getClass();
-            this.event = event;
+        if (this.eventClass == null) {
-        }
+            this.eventClass = newEventClass;
-        else if (!this.event.getClass().equals(event.getClass())) {
+        } else if (!this.eventClass.equals(newEventClass)) {
             multiple = true;
+            this.eventClass = getCommonSuperclass(newEventClass, this.eventClass).asSubclass(Event.class);
         }
         long start = System.nanoTime();
         super.callEvent(event);
         totalTime += System.nanoTime() - start;
     }
 
+    private static Class<?> getCommonSuperclass(Class<?> class1, Class<?> class2) {
+        while (!class1.isAssignableFrom(class2)) {
+            class1 = class1.getSuperclass();
+        }
+        return class1;
+    }
+
     /**
      * Resets the call count and total time for this listener
      */
@@ -63,18 +71,26 @@ public class TimedRegisteredListener extends RegisteredListener {
     }
 
     /**
-     * Gets the first event this listener handled
+     * Gets the class of the events this listener handled. If it handled
+     * multiple classes of event, the closest shared superclass will be
+     * returned, such that for any event this listener has handled,
+     * <code>this.getEventClass().isAssignableFrom(event.getClass())</code>
+     * and no class
+     * <code>this.getEventClass().isAssignableFrom(clazz)
+     * && this.getEventClass() != clazz &&
+     * event.getClass().isAssignableFrom(clazz)</code> for all handled events.
      *
-     * @return An event handled by this RegisteredListener
+     * @return the event class handled by this RegisteredListener
      */
-    public Event getEvent() {
+    public Class<? extends Event> getEventClass() {
-        return event;
+        return eventClass;
     }
 
     /**
-     * Gets whether this listener has handled multiple events
+     * Gets whether this listener has handled multiple events, such that for
+     * some two events, <code>eventA.getClass() != eventB.getClass()</code>.
      *
-     * @return True if this listener has handled multiple events
+     * @return true if this listener has handled multiple events
      */
     public boolean hasMultiple() {
         return multiple;

paper-api/src/test/java/org/bukkit/plugin/TimedRegisteredListenerTest.java

@@ -0,0 +1,56 @@
+package org.bukkit.plugin;
+
+import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.*;
+
+import org.bukkit.event.Event;
+import org.bukkit.event.EventException;
+import org.bukkit.event.EventPriority;
+import org.bukkit.event.Listener;
+import org.bukkit.event.block.BlockBreakEvent;
+import org.bukkit.event.player.PlayerEvent;
+import org.bukkit.event.player.PlayerInteractEvent;
+import org.bukkit.event.player.PlayerMoveEvent;
+import org.junit.Test;
+
+public class TimedRegisteredListenerTest {
+
+    @Test
+    public void testEventClass() throws EventException {
+        Listener listener = new Listener() {};
+        EventExecutor executor = new EventExecutor() {
+            public void execute(Listener listener, Event event) {}
+        };
+        TestPlugin plugin = new TestPlugin("Test");
+
+        PlayerInteractEvent interactEvent = new PlayerInteractEvent(null, null, null, null, null);
+        PlayerMoveEvent moveEvent = new PlayerMoveEvent(null, null, null);
+        BlockBreakEvent breakEvent = new BlockBreakEvent(null, null);
+
+        TimedRegisteredListener trl = new TimedRegisteredListener(listener, executor, EventPriority.NORMAL, plugin, false);
+
+        // Ensure that the correct event type is reported for a single event
+        trl.callEvent(interactEvent);
+        assertThat(trl.getEventClass(), is((Object) PlayerInteractEvent.class));
+        // Ensure that no superclass is used in lieu of the actual event, after two identical event types
+        trl.callEvent(interactEvent);
+        assertThat(trl.getEventClass(), is((Object) PlayerInteractEvent.class));
+        // Ensure that the closest superclass of the two events is chosen
+        trl.callEvent(moveEvent);
+        assertThat(trl.getEventClass(), is((Object) PlayerEvent.class));
+        // As above, so below
+        trl.callEvent(breakEvent);
+        assertThat(trl.getEventClass(), is((Object) Event.class));
+        // In the name of being thorough, check that it never travels down the hierarchy again.
+        trl.callEvent(breakEvent);
+        assertThat(trl.getEventClass(), is((Object) Event.class));
+
+        trl = new TimedRegisteredListener(listener, executor, EventPriority.NORMAL, plugin, false);
+
+        trl.callEvent(breakEvent);
+        assertThat(trl.getEventClass(), is((Object) BlockBreakEvent.class));
+        // Test moving up the class hierarchy by more than one class at a time
+        trl.callEvent(moveEvent);
+        assertThat(trl.getEventClass(), is((Object) Event.class));
+    }
+}