[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 @@
     /**
      * 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 @@
             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

@@ -257,6 +257,12 @@ THE SOFTWARE.
       <version>343.vdcf37b_a_c81d5</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,80 @@
+package hudson;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.hasSize;
+import static org.junit.Assert.assertNotNull;
+
+import java.util.Collection;
+import jenkins.plugins.dependee.Dependee;
+import jenkins.plugins.dependee.DependeeExtensionPoint;
+import jenkins.plugins.dynamic_extension_loading.CustomExtensionLoadedViaConstructor;
+import jenkins.plugins.dynamic_extension_loading.CustomExtensionLoadedViaListener;
+import jenkins.plugins.dynamic_extension_loading.CustomPeriodicWork;
+import jenkins.plugins.optional_depender.OptionalDepender;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+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
+    @Issue("JENKINS-75232")
+    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
+    @Issue({ "JENKINS-50336", "JENKINS-60449" })
+    public void installDependedOptionalPluginWithoutRestart() throws Throwable {
+        var optionalDependerJpi = rjr.createSyntheticPlugin(new SyntheticPlugin(OptionalDepender.class.getPackage())
+                .header("Plugin-Dependencies", "variant:0,dependee:0;resolution:=optional"));
+        var dependeeJpi = rjr.createSyntheticPlugin(new SyntheticPlugin(Dependee.class.getPackage()).shortName("dependee"));
+        var fqcn1 = OptionalDepender.class.getName();
+        var fqcn2 = DependeeExtensionPoint.class.getName();
+        rjr.then(r -> {
+            // Load optional-depender.
+            r.jenkins.pluginManager.dynamicLoad(optionalDependerJpi);
+            // JENKINS-60449: Extension depending on dependee class isn't loaded
+            assertThat((Collection<?>) r.jenkins.getExtensionList(fqcn1), empty());
+            // Load dependee.
+            r.jenkins.pluginManager.dynamicLoad(dependeeJpi);
+            // JENKINS-60449: Classes in depender are loaded prior to OptionalDepender being loaded.
+            assertThat((Collection<?>) r.jenkins.getExtensionList(fqcn1), hasSize(1));
+            // JENKINS-50336: Extension point from dependee now includes optional extension from optional-depender
+            assertThat((Collection<?>) r.jenkins.getExtensionList(fqcn2), hasSize(1));
+        });
+    }
+}

test/src/test/java/hudson/PluginManagerTest.java

@@ -86,7 +86,6 @@
 import java.util.stream.Collectors;
 import jenkins.ClassLoaderReflectionToolkit;
 import jenkins.RestartRequiredException;
-import jenkins.model.GlobalConfiguration;
 import jenkins.model.Jenkins;
 import net.sf.json.JSONArray;
 import net.sf.json.JSONObject;
@@ -389,28 +388,6 @@ private String callDependerValue() throws Exception {
         assertEquals(1, r.jenkins.getExtensionList("org.jenkinsci.plugins.dependencytest.depender.DependerExtension").size());
     }
 
-    /**
-     * Load "optional-depender" and then load "dependee".
-     * Asserts that "depender" can access to "dependee".
-     */
-    @Issue("JENKINS-60449")
-    @WithPlugin("variant.hpi")
-    @Test public void installDependedOptionalPluginWithoutRestart() throws Exception {
-        // Load optional-depender.
-        {
-            dynamicLoad("optional-depender-0.0.2.hpi");
-        }
-        // Extension depending on dependee class isn't loaded
-        assertTrue(r.jenkins.getExtensionList("org.jenkinsci.plugins.dependencytest.optionaldepender.OptionalDependerExtension").isEmpty());
-        // Load dependee.
-        {
-            dynamicLoad("dependee-0.0.2.hpi");
-        }
-
-        // Extensions in depender are loaded.
-        assertEquals(1, r.jenkins.getExtensionList("org.jenkinsci.plugins.dependencytest.optionaldepender.OptionalDependerExtension").size());
-    }
-
     @Issue("JENKINS-21486")
     @Test public void installPluginWithObsoleteDependencyFails() throws Exception {
         // Load dependee 0.0.1.
@@ -611,22 +588,6 @@ public void findResourceForPluginFirstClassLoader() {
         assertEquals(fromPlugin, fromToolkit);
     }
 
-    // Sources for jenkins-50336.hpi are available at https://github.com/Vlatombe/jenkins-50336
-    //
-    // package io.jenkins.plugins;
-    // import org.jenkinsci.plugins.variant.OptionalExtension;
-    // import jenkins.model.GlobalConfiguration;
-    // @OptionalExtension public class MyGlobalConfiguration extends GlobalConfiguration {}
-    //
-    @Issue("JENKINS-50336")
-    @Test
-    public void optionalExtensionCanBeFoundAfterDynamicLoadOfVariant() throws Exception {
-        dynamicLoad("variant.hpi");
-        assertNotNull(r.jenkins.getPluginManager().getPlugin("variant"));
-        dynamicLoad("jenkins-50336.hpi");
-        assertTrue(ExtensionList.lookup(GlobalConfiguration.class).stream().anyMatch(gc -> "io.jenkins.plugins.MyGlobalConfiguration".equals(gc.getClass().getName())));
-    }
-
     @Test @Issue("JENKINS-64840")
     @WithPlugin({"mandatory-depender-0.0.2.hpi", "dependee-0.0.2.hpi", "depender-0.0.2.hpi"})
     public void getPluginsSortedByTitle() throws Exception {

test/src/test/java/hudson/cli/DisablePluginCommandTest.java

@@ -190,11 +190,10 @@ public void disableAlreadyDisabledPluginNotRestart() throws Exception {
     @Ignore("TODO calling restart seems to break Surefire")
     @Test
     @Issue("JENKINS-27177")
-    @WithPlugin({"variant.hpi", "depender-0.0.2.hpi", "mandatory-depender-0.0.2.hpi", "plugin-first.hpi", "dependee-0.0.2.hpi"})
+    @WithPlugin({"depender-0.0.2.hpi", "mandatory-depender-0.0.2.hpi", "plugin-first.hpi", "dependee-0.0.2.hpi"})
     public void restartAfterDisablePluginsAndErrors() {
         assumeNotWindows();
-        assertThat(disablePluginsCLiCommand("-restart", "variant", "dependee", "depender", "plugin-first", "mandatory-depender"), failedWith(RETURN_CODE_NOT_DISABLED_DEPENDANTS));
-        assertPluginDisabled("variant");
+        assertThat(disablePluginsCLiCommand("-restart", "dependee", "depender", "plugin-first", "mandatory-depender"), failedWith(RETURN_CODE_NOT_DISABLED_DEPENDANTS));
         assertPluginEnabled("dependee");
         assertPluginDisabled("depender");
         assertPluginDisabled("plugin-first");
@@ -207,13 +206,12 @@ public void restartAfterDisablePluginsAndErrors() {
      */
     @Test
     @Issue("JENKINS-27177")
-    @WithPlugin({"variant.hpi", "depender-0.0.2.hpi", "mandatory-depender-0.0.2.hpi", "plugin-first.hpi", "dependee-0.0.2.hpi"})
+    @WithPlugin({"depender-0.0.2.hpi", "mandatory-depender-0.0.2.hpi", "plugin-first.hpi", "dependee-0.0.2.hpi"})
     public void disablePluginsStrategyAll() {
         assertPluginEnabled("dependee");
         assertPluginEnabled("depender");
         assertPluginEnabled("mandatory-depender");
-        assertThat(disablePluginsCLiCommand("-strategy", "all", "variant", "dependee", "plugin-first"), succeeded());
-        assertPluginDisabled("variant");
+        assertThat(disablePluginsCLiCommand("-strategy", "all", "dependee", "plugin-first"), succeeded());
         assertPluginDisabled("dependee");
         assertPluginDisabled("depender");
         assertPluginDisabled("plugin-first");
@@ -225,10 +223,9 @@ public void disablePluginsStrategyAll() {
      */
     @Test
     @Issue("JENKINS-27177")
-    @WithPlugin({"variant.hpi", "depender-0.0.2.hpi", "mandatory-depender-0.0.2.hpi", "plugin-first.hpi", "dependee-0.0.2.hpi"})
+    @WithPlugin({"depender-0.0.2.hpi", "mandatory-depender-0.0.2.hpi", "plugin-first.hpi", "dependee-0.0.2.hpi"})
     public void disablePluginsStrategyMandatory() {
-        assertThat(disablePluginsCLiCommand("-strategy", "mandatory", "variant", "dependee", "plugin-first"), succeeded());
-        assertPluginDisabled("variant");
+        assertThat(disablePluginsCLiCommand("-strategy", "mandatory", "dependee", "plugin-first"), succeeded());
         assertPluginDisabled("dependee");
         assertPluginEnabled("depender");
         assertPluginDisabled("plugin-first");

test/src/test/java/hudson/cli/EnablePluginCommandTest.java

@@ -139,7 +139,7 @@ public void enablePluginWithRestart() throws IOException {
     @Issue("JENKINS-52950")
     public void enableNoPluginsWithRestartIsNoOp() {
         assumeNotWindows();
-        String name = "variant";
+        String name = "icon-shim";
         assertThat(installTestPlugin(name), succeeded());
         assertThat(enablePlugins("-restart", name), succeeded());
         assertJenkinsNotInQuietMode();

test/src/test/java/jenkins/plugins/dependee/Dependee.java

@@ -0,0 +1,7 @@
+package jenkins.plugins.dependee;
+
+public class Dependee {
+    public static String getValue() {
+        return "dependee";
+    }
+}

test/src/test/java/jenkins/plugins/dependee/DependeeExtensionPoint.java

@@ -0,0 +1,6 @@
+package jenkins.plugins.dependee;
+
+import hudson.ExtensionPoint;
+
+public class DependeeExtensionPoint implements ExtensionPoint {
+}

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

@@ -0,0 +1,14 @@
+package jenkins.plugins.dynamic_extension_loading;
+
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import org.jenkinsci.plugins.variant.OptionalExtension;
+
+@OptionalExtension
+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 java.util.logging.Level;
+import java.util.logging.Logger;
+import org.jenkinsci.plugins.variant.OptionalExtension;
+
+@OptionalExtension
+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.ExtensionList;
+import hudson.model.PeriodicWork;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import org.jenkinsci.plugins.variant.OptionalExtension;
+
+@OptionalExtension
+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;