[JEP-228] Unforking XStream (#4944)

* Unforking XStream

* Some overrides missing from MapperDelegate

* [XSTR-762] writeReplace & readResolve must be accessible to be used from subclasses

* DomElement.click overloads changed incompatibly

* Giving up on ToolInstallation being Serializable.
Fixing MavenInstallation accordingly.
AntInstallation was already fixed: jenkinsci/ant-plugin@c957dca

* Relaxing message assertion in RobustReflectionConverter.randomExceptionsReported

* Picking up jenkinsci/matrix-auth-plugin#89 caught via JobTest

* jenkinsci/jenkins-test-harness#243 released

* Another case forgotten in 0e5aeec

* XStream has SecurityMapper, but we have our own system already, so suppress warning

* GetMavenVersion was also serializing its outer class

* Trying to make ToolInstallation serializable compatible

* Avoid throwing IOException from PersistedList.inModified for common cases covered by jenkinsci/jenkins-test-harness#243

* Found a way to avoid looking up XStream.converterRegistry by reflection

* XStream2Test.crashXstream now throwing ConversionException, not ForbiddenClassException

* Noting which version of the parent POM has jenkinsci/jenkins-test-harness#243

* Refreshed Javadoc for XStream2

* The simplest solution to JENKINS-19561 seems to be to stop using autodetectAnnotations

* Fixing some deprecations in Robust*Converter

* Display OldDataMonitor errors from RobustReflectionConverter during functional tests

* Stray digit in test name

* Added XStream2AnnotationTest to clearly document incompatibility

* Picking up release of jenkinsci/matrix-auth-plugin#89

Co-authored-by: Basil Crow <[email protected]>

Co-authored-by: Basil Crow <[email protected]>

bom/pom.xml

@@ -332,9 +332,9 @@ THE SOFTWARE.
 
       <!--XStream-->
       <dependency>
-        <groupId>org.jvnet.hudson</groupId>
+        <groupId>com.thoughtworks.xstream</groupId>
         <artifactId>xstream</artifactId>
-        <version>1.4.7-jenkins-1</version>
+        <version>1.4.13</version>
       </dependency>
       <dependency>
         <groupId>net.sf.kxml</groupId>

core/pom.xml

@@ -230,7 +230,7 @@ THE SOFTWARE.
       <artifactId>antlr</artifactId>
     </dependency>
     <dependency>
-      <groupId>org.jvnet.hudson</groupId>
+      <groupId>com.thoughtworks.xstream</groupId>
       <artifactId>xstream</artifactId>
       <exclusions>
         <exclusion>

core/src/main/java/hudson/diagnosis/OldDataMonitor.java

@@ -27,6 +27,7 @@
 import com.thoughtworks.xstream.converters.UnmarshallingContext;
 import hudson.Extension;
 import hudson.ExtensionList;
+import hudson.Main;
 import hudson.XmlFile;
 import hudson.model.AdministrativeMonitor;
 import hudson.model.Item;
@@ -207,6 +208,9 @@ public static void report(Saveable obj, Collection<Throwable> errors) {
             if (e instanceof ReportException) {
                 report(obj, ((ReportException)e).version);
             } else {
+                if (Main.isUnitTest) {
+                    LOGGER.log(Level.INFO, "Trouble loading " + obj, e);
+                }
                 if (++i > 1) buf.append(", ");
                 buf.append(e.getClass().getSimpleName()).append(": ").append(e.getMessage());
             }

core/src/main/java/hudson/model/AbstractItem.java

@@ -614,7 +614,7 @@ public final XmlFile getConfigFile() {
         return Items.getConfigFile(this);
     }
 
-    private Object writeReplace() {
+    protected Object writeReplace() {
         return XmlFile.replaceIfNotAtTopLevel(this, () -> new Replacer(this));
     }
     private static class Replacer {

core/src/main/java/hudson/model/ListView.java

@@ -132,7 +132,7 @@ public void setJobFilters(List<ViewJobFilter> jobFilters) throws IOException {
         this.jobFilters.replaceBy(jobFilters);
     }
 
-    private Object readResolve() {
+    protected Object readResolve() {
         if(includeRegex!=null) {
             try {
                 includePattern = Pattern.compile(includeRegex);

core/src/main/java/hudson/model/Queue.java

@@ -2383,7 +2383,7 @@ public Api getApi() throws AccessDeniedException {
             }
         }
 
-        private Object readResolve() {
+        protected Object readResolve() {
             this.future = new FutureImpl(task);
             return this;
         }

core/src/main/java/hudson/model/Run.java

@@ -2082,7 +2082,7 @@ public synchronized void save() throws IOException {
         return new XmlFile(XSTREAM,new File(getRootDir(),"build.xml"));
     }
 
-    private Object writeReplace() {
+    protected Object writeReplace() {
         return XmlFile.replaceIfNotAtTopLevel(this, () -> new Replacer(this));
     }
     private static class Replacer {

core/src/main/java/hudson/tasks/Maven.java

@@ -553,7 +553,7 @@ public void buildEnvVars(EnvVars env) {
         public boolean meetsMavenReqVersion(Launcher launcher, int mavenReqVersion) throws IOException, InterruptedException {
             // FIXME using similar stuff as in the maven plugin could be better 
             // olamy : but will add a dependency on maven in core -> so not so good 
-            String mavenVersion = launcher.getChannel().call(new GetMavenVersion());
+            String mavenVersion = launcher.getChannel().call(new GetMavenVersion(getHome()));
 
             if (!mavenVersion.equals("")) {
                 if (mavenReqVersion == MAVEN_20) {
@@ -572,11 +572,14 @@ else if (mavenReqVersion == MAVEN_30) {
             return false;
 
         }
-        private class GetMavenVersion extends MasterToSlaveCallable<String, IOException> {
-            private static final long serialVersionUID = -4143159957567745621L;
+        private static class GetMavenVersion extends MasterToSlaveCallable<String, IOException> {
+            private final String home;
+            GetMavenVersion(String home) {
+                this.home = home;
+            }
             @Override
             public String call() throws IOException {
-                File[] jars = new File(getHomeDir(), "lib").listFiles();
+                File[] jars = new File(home, "lib").listFiles();
                 if (jars != null) { // be defensive
                     for (File jar : jars) {
                         if (jar.getName().startsWith("maven-")) {
@@ -608,24 +611,29 @@ public boolean isMaven2_1(Launcher launcher) throws IOException, InterruptedExce
          * Gets the executable path of this maven on the given target system.
          */
         public String getExecutable(Launcher launcher) throws IOException, InterruptedException {
-            return launcher.getChannel().call(new GetExecutable());
-        }
-        private class GetExecutable extends MasterToSlaveCallable<String, IOException> {
-                private static final long serialVersionUID = 2373163112639943768L;
-                @Override
-                public String call() throws IOException {
-                    File exe = getExeFile("mvn");
-                    if(exe.exists())
-                        return exe.getPath();
-                    exe = getExeFile("maven");
-                    if(exe.exists())
-                        return exe.getPath();
-                    return null;
+            return launcher.getChannel().call(new GetExecutable(getHome()));
+        }
+        private static class GetExecutable extends MasterToSlaveCallable<String, IOException> {
+            private final String rawHome;
+            GetExecutable(String rawHome) {
+                this.rawHome = rawHome;
+            }
+            @Override
+            public String call() throws IOException {
+                File exe = getExeFile("mvn", rawHome);
+                if (exe.exists()) {
+                    return exe.getPath();
+                }
+                exe = getExeFile("maven", rawHome);
+                if (exe.exists()) {
+                    return exe.getPath();
                 }
+                return null;
+            }
         }
 
-        private File getExeFile(String execName) {
-            String m2Home = Util.replaceMacro(getHome(),EnvVars.masterEnvVars);
+        private static File getExeFile(String execName, String home) {
+            String m2Home = Util.replaceMacro(home, EnvVars.masterEnvVars);
 
             if(Functions.isWindows()) {
                 File exeFile = new File(m2Home, "bin/" + execName + ".bat");

core/src/main/java/hudson/tools/ToolInstallation.java

@@ -30,19 +30,26 @@
 import hudson.ExtensionPoint;
 import hudson.diagnosis.OldDataMonitor;
 import hudson.model.*;
+import hudson.remoting.Channel;
 import hudson.slaves.NodeSpecific;
 import hudson.util.DescribableList;
 import hudson.util.XStream2;
 
 import java.io.Serializable;
+import java.io.StringReader;
 import java.io.IOException;
 import java.util.List;
 
-import com.thoughtworks.xstream.annotations.XStreamSerializable;
 import com.thoughtworks.xstream.converters.UnmarshallingContext;
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import jenkins.model.Jenkins;
+import jenkins.util.Timer;
+import org.dom4j.Document;
+import org.dom4j.Element;
+import org.dom4j.io.SAXReader;
 
 /**
  * Formalization of a tool installed in nodes used for builds.
@@ -76,14 +83,16 @@
  * @since 1.286
  */
 public abstract class ToolInstallation extends AbstractDescribableImpl<ToolInstallation> implements Serializable, ExtensionPoint {
+
+    private static final Logger LOGGER = Logger.getLogger(ToolInstallation.class.getName());
+
     private final String name;
     private /*almost final*/ String home;
 
     /**
      * {@link ToolProperty}s that are associated with this tool.
      */
-    @XStreamSerializable
-    private transient /*almost final*/ DescribableList<ToolProperty<?>,ToolPropertyDescriptor> properties
+    private /*almost final*/ DescribableList<ToolProperty<?>,ToolPropertyDescriptor> properties
             = new DescribableList<>(Saveable.NOOP);
 
     /**
@@ -145,8 +154,10 @@ public String getName() {
     public void buildEnvVars(EnvVars env) {
     }
 
-    public DescribableList<ToolProperty<?>,ToolPropertyDescriptor> getProperties() {
-        assert properties!=null;
+    public synchronized DescribableList<ToolProperty<?>,ToolPropertyDescriptor> getProperties() {
+        if (properties == null) {
+            properties = new DescribableList<>(Saveable.NOOP);
+        }
         return properties;
     }
 
@@ -210,13 +221,32 @@ protected String translateFor(Node node, TaskListener log) throws IOException, I
      * Invoked by XStream when this object is read into memory.
      */
     protected Object readResolve() {
-        if(properties==null)
-            properties = new DescribableList<>(Saveable.NOOP);
-        for (ToolProperty<?> p : properties)
-            _setTool(p, this);
+        if (properties != null) {
+            for (ToolProperty<?> p : properties) {
+                _setTool(p, this);
+            }
+        }
         return this;
     }
 
+    protected Object writeReplace() throws Exception {
+        if (Channel.current() == null) { // XStream
+            return this;
+        } else { // Remoting
+            LOGGER.log(Level.WARNING, "Serialization of " + getClass().getSimpleName() + " extends ToolInstallation over Remoting is deprecated", new Throwable());
+            // Hack: properties is not serializable, so try to serialize as XML (in another thread); delete <properties/>; deserialize; then load a clone
+            String xml1 = Timer.get().submit(() -> Jenkins.XSTREAM2.toXML(this)).get();
+            Document dom = new SAXReader().read(new StringReader(xml1));
+            Element properties = dom.getRootElement().element("properties");
+            if (properties != null) {
+                dom.getRootElement().remove(properties);
+            }
+            String xml2 = dom.asXML();
+            ToolInstallation clone = (ToolInstallation) Timer.get().submit(() -> Jenkins.XSTREAM2.fromXML(xml2)).get();
+            return clone;
+        }
+    }
+
     @Override public String toString() {
         return getClass().getSimpleName() + "[" + name + "]";
     }
@@ -244,5 +274,4 @@ public static DescriptorExtensionList<ToolInstallation,ToolDescriptor<?>> all()
         return Jenkins.get().getDescriptorList(ToolInstallation.class);
     }
 
-    private static final long serialVersionUID = 1L;
 }

core/src/main/java/hudson/util/PersistedList.java

@@ -23,6 +23,7 @@
  */
 package hudson.util;
 
+import com.google.common.collect.ImmutableSet;
 import com.infradna.tool.bridge_method_injector.WithBridgeMethods;
 import com.thoughtworks.xstream.converters.Converter;
 import com.thoughtworks.xstream.converters.MarshallingContext;
@@ -40,6 +41,10 @@
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 /**
  * Collection whose change is notified to the parent object for persistence.
@@ -48,6 +53,9 @@
  * @since 1.333
  */
 public class PersistedList<T> extends AbstractList<T> {
+
+    private static final Logger LOGGER = Logger.getLogger(PersistedList.class.getName());
+
     protected final CopyOnWriteList<T> data = new CopyOnWriteList<>();
     protected Saveable owner = Saveable.NOOP;
 
@@ -170,7 +178,30 @@ public Iterator<T> iterator() {
      * Called when a list is mutated.
      */
     protected void onModified() throws IOException {
-        owner.save();
+        try {
+            owner.save();
+        } catch (IOException x) {
+            Optional<T> ignored = stream().filter(PersistedList::ignoreSerializationErrors).findAny();
+            if (ignored.isPresent()) {
+                LOGGER.log(Level.WARNING, "Ignoring serialization errors in " + ignored.get() + "; update your parent POM to 4.8 or newer", x);
+            } else {
+                throw x;
+            }
+        }
+    }
+
+    // TODO until https://github.com/jenkinsci/jenkins-test-harness/pull/243 is widely adopted:
+    private static final Set<String> IGNORED_CLASSES = ImmutableSet.of("org.jvnet.hudson.test.TestBuilder", "org.jvnet.hudson.test.TestNotifier");
+    // (SingleFileSCM & ExtractResourceWithChangesSCM would also be nice to suppress, but they are not kept in a PersistedList.)
+    private static boolean ignoreSerializationErrors(Object o) {
+        if (o != null) {
+            for (Class<?> c = o.getClass(); c != Object.class; c = c.getSuperclass()) {
+                if (IGNORED_CLASSES.contains(c.getName())) {
+                    return true;
+                }
+            }
+        }
+        return false;
     }
 
     /**

core/src/main/java/hudson/util/RobustCollectionConverter.java

@@ -31,6 +31,7 @@
 import com.thoughtworks.xstream.mapper.Mapper;
 import com.thoughtworks.xstream.XStream;
 import com.thoughtworks.xstream.XStreamException;
+import com.thoughtworks.xstream.core.ClassLoaderReference;
 
 import java.util.Collection;
 import java.util.concurrent.CopyOnWriteArrayList;
@@ -48,6 +49,7 @@
  *
  * @author Kohsuke Kawaguchi
  */
+@SuppressWarnings({"rawtypes", "unchecked"})
 public class RobustCollectionConverter extends CollectionConverter {
     private final SerializableConverter sc;
 
@@ -57,7 +59,7 @@ public RobustCollectionConverter(XStream xs) {
 
     public RobustCollectionConverter(Mapper mapper, ReflectionProvider reflectionProvider) {
         super(mapper);
-        sc = new SerializableConverter(mapper,reflectionProvider);
+        sc = new SerializableConverter(mapper, reflectionProvider, new ClassLoaderReference(null));
     }
 
     @Override
@@ -82,7 +84,7 @@ protected void populateCollection(HierarchicalStreamReader reader, Unmarshalling
         while (reader.hasMoreChildren()) {
             reader.moveDown();
             try {
-                Object item = readItem(reader, context, collection);
+                Object item = readBareItem(reader, context, collection);
                 collection.add(item);
             } catch (CriticalXStreamException e) {
                 throw e;

core/src/main/java/hudson/util/RobustMapConverter.java

@@ -55,7 +55,7 @@ final class RobustMapConverter extends MapConverter {
     private Object read(HierarchicalStreamReader reader, UnmarshallingContext context, Map map) {
         reader.moveDown();
         try {
-            return readItem(reader, context, map);
+            return readBareItem(reader, context, map);
         } catch (CriticalXStreamException x) {
             throw x;
         } catch (XStreamException | LinkageError x) {