[JENKINS-75232] Prevent dynamic plugin installation from registering the same extension twice in some cases

core/src/main/java/hudson/ExtensionList.java

@@ -36,10 +36,12 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.IdentityHashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import java.util.Vector;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArrayList;
@@ -48,6 +50,8 @@
 import jenkins.ExtensionComponentSet;
 import jenkins.model.Jenkins;
 import jenkins.util.io.OnMaster;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 
 /**
  * Retains the known extension instances for the given type 'T'.
@@ -335,27 +339,44 @@ protected Object getLoadLock() {
     /**
      * Used during {@link Jenkins#refreshExtensions()} to add new components into existing {@link ExtensionList}s.
      * Do not call from anywhere else.
+     * @return true if {@link #fireOnChangeListeners} should be called on {@code this} after all lists have been refreshed.
      */
-    public void refresh(ExtensionComponentSet delta) {
-        boolean fireOnChangeListeners = false;
+    @Restricted(NoExternalUse.class)
+    public boolean refresh(ExtensionComponentSet delta) {
         synchronized (getLoadLock()) {
             if (extensions == null)
-                return;     // not yet loaded. when we load it, we'll load everything visible by then, so no work needed
-
-            Collection<ExtensionComponent<T>> found = load(delta);
-            if (!found.isEmpty()) {
-                List<ExtensionComponent<T>> l = new ArrayList<>(extensions);
-                l.addAll(found);
-                extensions = sort(l);
-                fireOnChangeListeners = true;
+                return false;     // not yet loaded. when we load it, we'll load everything visible by then, so no work needed
+
+            Collection<ExtensionComponent<T>> newComponents = load(delta);
+            if (!newComponents.isEmpty()) {
+                // We check to ensure that we do not insert duplicate instances of already-loaded extensions into the list.
+                // This can happen when dynamically loading a plugin with an extension A that itself loads another
+                // extension B from the same plugin in some contexts, such as in A's constructor or via a method in A called
+                // by an ExtensionListListener. In those cases, ExtensionList.refresh may be called on a list that already
+                // includes the new extensions. Note that ExtensionComponent objects are always unique, even when
+                // ExtensionComponent.getInstance is identical, so we have to track the components and instances separately
+                // to handle ordinal sorting and check for dupes.
+                List<ExtensionComponent<T>> components = new ArrayList<>(extensions);
+                Set<T> instances = Collections.newSetFromMap(new IdentityHashMap<>());
+                for (ExtensionComponent<T> component : components) {
+                    instances.add(component.getInstance());
+                }
+                boolean fireListeners = false;
+                for (ExtensionComponent<T> newComponent : newComponents) {
+                    if (instances.add(newComponent.getInstance())) {
+                        fireListeners = true;
+                        components.add(newComponent);
+                    }
+                }
+                extensions = sort(new ArrayList<>(components));
+                return fireListeners;
             }
         }
-        if (fireOnChangeListeners) {
-            fireOnChangeListeners();
-        }
+        return false;
     }
 
-    private void fireOnChangeListeners() {
+    @Restricted(NoExternalUse.class)
+    public void fireOnChangeListeners() {
         for (ExtensionListListener listener : listeners) {
             try {
                 listener.onChange();

core/src/main/java/jenkins/model/Jenkins.java

@@ -2867,11 +2867,21 @@ public void refreshExtensions() throws ExtensionRefreshException {
             delta = ExtensionComponentSet.union(delta, ef.refresh().filtered());
         }
 
+        List<ExtensionList> listsToFireOnChangeListeners = new ArrayList<>();
         for (ExtensionList el : extensionLists.values()) {
-            el.refresh(delta);
+            if (el.refresh(delta)) {
+                listsToFireOnChangeListeners.add(el);
+            }
         }
         for (ExtensionList el : descriptorLists.values()) {
-            el.refresh(delta);
+            if (el.refresh(delta)) {
+                listsToFireOnChangeListeners.add(el);
+            }
+        }
+        // Refresh all extension lists before firing any listeners in case a listener would cause any new extension
+        // lists to be forcibly loaded, which may lead to duplicate entries for the same extension object in a list.
+        for (var el : listsToFireOnChangeListeners) {
+            el.fireOnChangeListeners();
         }
 
         // TODO: we need some generalization here so that extension points can be notified when a refresh happens?

test/pom.xml

@@ -92,6 +92,12 @@ THE SOFTWARE.
         <artifactId>plugin-util-api</artifactId>
         <version>6.0.0</version>
       </dependency>
+      <dependency>
+        <groupId>org.jenkins-ci.main</groupId>
+        <artifactId>jenkins-test-harness</artifactId>
+        <!-- TODO: https://github.com/jenkinsci/jenkins-test-harness/pull/920 -->
+        <version>2399.v070266dc43f6</version>
+      </dependency>
       <dependency>
         <groupId>org.jenkins-ci.plugins</groupId>
         <artifactId>ant</artifactId>
@@ -178,7 +184,6 @@ THE SOFTWARE.
     <dependency>
       <groupId>org.jenkins-ci.main</groupId>
       <artifactId>jenkins-test-harness</artifactId>
-      <version>2395.v8256a_2157798</version>
       <scope>test</scope>
       <exclusions>
         <exclusion>
@@ -257,6 +262,12 @@ THE SOFTWARE.
       <version>338.v848422169819</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.jenkins-ci.plugins</groupId>
+      <artifactId>variant</artifactId>
+      <version>70.va_d9f17f859e0</version>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>org.junit.jupiter</groupId>
       <artifactId>junit-jupiter</artifactId>

test/src/test/java/hudson/ExtensionListRjrTest.java

@@ -0,0 +1,49 @@
+package hudson;
+
+import static org.junit.Assert.assertNotNull;
+
+import jenkins.plugins.dynamic_extension_loading.CustomExtensionLoadedViaConstructor;
+import jenkins.plugins.dynamic_extension_loading.CustomExtensionLoadedViaListener;
+import jenkins.plugins.dynamic_extension_loading.CustomPeriodicWork;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.RealJenkinsRule;
+import org.jvnet.hudson.test.RealJenkinsRule.SyntheticPlugin;
+
+public class ExtensionListRjrTest {
+    @Rule
+    public RealJenkinsRule rjr = new RealJenkinsRule();
+
+    /**
+     * Check that dynamically loading a plugin does not lead to extension lists with duplicate entries.
+     * In particular we test extensions that load other extensions in their constructors and extensions that have
+     * methods that load other extensions and which are called by an {@link ExtensionListListener}.
+     */
+    @Test
+    public void checkDynamicLoad_singleRegistration() throws Throwable {
+        var pluginJpi = rjr.createSyntheticPlugin(new SyntheticPlugin(CustomPeriodicWork.class.getPackage())
+                .shortName("dynamic-extension-loading")
+                .header("Plugin-Dependencies", "variant:0"));
+        var fqcn1 = CustomExtensionLoadedViaListener.class.getName();
+        var fqcn2 = CustomExtensionLoadedViaConstructor.class.getName();
+        rjr.then(r -> {
+            r.jenkins.pluginManager.dynamicLoad(pluginJpi);
+            assertSingleton(r, fqcn1);
+            assertSingleton(r, fqcn2);
+        });
+    }
+
+    private static void assertSingleton(JenkinsRule r, String fqcn) throws Exception {
+        var clazz = r.jenkins.pluginManager.uberClassLoader.loadClass(fqcn);
+        try {
+            assertNotNull(ExtensionList.lookupSingleton(clazz));
+        } catch (Throwable t) {
+            var list = ExtensionList.lookup(clazz).stream().map(e ->
+                    // TODO: Objects.toIdentityString in Java 19+
+                    e.getClass().getName() + "@" + Integer.toHexString(System.identityHashCode(e))).toList();
+            System.out.println("Duplicates are: " + list);
+            throw t;
+        }
+    }
+}

test/src/test/java/jenkins/plugins/dynamic_extension_loading/CustomExtensionLoadedViaConstructor.java

@@ -0,0 +1,14 @@
+package jenkins.plugins.dynamic_extension_loading;
+
+import hudson.Extension;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+@Extension
+public class CustomExtensionLoadedViaConstructor {
+    private static final Logger LOGGER = Logger.getLogger(CustomExtensionLoadedViaConstructor.class.getName());
+
+    public CustomExtensionLoadedViaConstructor() {
+        LOGGER.log(Level.INFO, null, new Exception("Instantiating CustomExtensionLoadedViaConstructor"));
+    }
+}

test/src/test/java/jenkins/plugins/dynamic_extension_loading/CustomExtensionLoadedViaListener.java

@@ -0,0 +1,16 @@
+package jenkins.plugins.dynamic_extension_loading;
+
+import hudson.Extension;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+@Extension
+public class CustomExtensionLoadedViaListener {
+    private static final Logger LOGGER = Logger.getLogger(CustomExtensionLoadedViaListener.class.getName());
+
+    public long recurrencePeriod = 120;
+
+    public CustomExtensionLoadedViaListener() {
+        LOGGER.log(Level.INFO, null, new Exception("Instantiating CustomExtensionLoadedViaListener"));
+    }
+}

test/src/test/java/jenkins/plugins/dynamic_extension_loading/CustomPeriodicWork.java

@@ -0,0 +1,27 @@
+package jenkins.plugins.dynamic_extension_loading;
+
+import hudson.Extension;
+import hudson.ExtensionList;
+import hudson.model.PeriodicWork;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+@Extension
+public class CustomPeriodicWork extends PeriodicWork {
+
+    private static final Logger LOGGER = Logger.getLogger(CustomPeriodicWork.class.getName());
+
+    public CustomPeriodicWork() {
+        LOGGER.log(Level.INFO, null, new Exception("Instantiating CustomPeriodicWork"));
+        ExtensionList.lookupSingleton(CustomExtensionLoadedViaConstructor.class);
+    }
+
+    @Override
+    protected void doRun() {}
+
+    @Override
+    public long getRecurrencePeriod() {
+        LOGGER.log(Level.INFO, null, new Exception("Loading CustomExtensionLoadedViaListener"));
+        return ExtensionList.lookupSingleton(CustomExtensionLoadedViaListener.class).recurrencePeriod;
+    }
+}

test/src/test/java/jenkins/plugins/dynamic_extension_loading/package-info.java

@@ -0,0 +1,4 @@
+@OptionalPackage(requirePlugins = "dynamic-extension-loading")
+package jenkins.plugins.dynamic_extension_loading;
+
+import org.jenkinsci.plugins.variant.OptionalPackage;