RFR: 8333796: Add missing serialization functionality to sun.reflect.ReflectionFactory

[Chen Liang]

On Thu, 13 Jun 2024 14:31:06 GMT, David M. Lloyd <duke at openjdk.org> wrote:

> Issue [JDK-8164908](https://bugs.openjdk.org/browse/JDK-8164908) added support for functionality required to continue to support IIOP and custom serializers in light of additional module-based restrictions on reflection. It was expected that these libraries would use `sun.misc.Unsafe` in order to access fields of serializable classes. However, with JEP 471, the methods necessary to do this are being removed.
> 
> To allow these libraries to continue to function, it is proposed to add two methods to `sun.reflect.ReflectionFactory` which will allow serialization libraries to acquire a method handle to generated `readObject`/`writeObject` methods which set or get the fields of the serializable class using the serialization `GetField`/`PutField` mechanism. These generated methods should be used by serialization libraries to serialize and deserialize classes which do not have a `readObject`/`writeObject` method or which use `ObjectInputStream.defaultReadObject`/`ObjectOutputStream.defaultWriteObject` to supplement default serialization.
> 
> It is also proposed to add methods which allow for the reading of serialization-specific private static final fields from classes which have them.
> 
> With the addition of these methods, serialization libraries no longer need to rely on `Unsafe` for serialization/deserialization activities.
> cc: @AlanBateman

Great work out there! We might need to check if this will be compatible with Valhalla.

Since this is an API addition, this will require a CSR; Valhalla folks can check if this API is future-proof in the CSR.

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 441:

> 439:         }
> 440: 
> 441:         return SerializationBytecodeGenerator.defaultReadObjectForSerialization(cl);

How likely is it for multiple libraries to request readObject for the same class?

If it happens that many clients request factories for the same class, we can consider computing and leaving the MHs in a `ClassValue` instead (so the MHs can be GC'd when `cl` is unreachable, but are otherwise cached and shared)

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 464:

> 462:             }
> 463:             field.setAccessible(true);
> 464:             return (ObjectStreamField[]) field.get(null);

Should we clone the array to prevent user modification?

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 488:

> 486:     }
> 487: 
> 488:     private static boolean isValidSerializable(Class<?> cl) {

Should we filter `Class`, `ObjectStreamClass`, and `String` (https://docs.oracle.com/en/java/javase/22/docs/specs/serialization/serial-arch.html#writing-to-an-object-stream) too?

src/java.base/share/classes/jdk/internal/reflect/SerializationBytecodeGenerator.java line 53:

> 51: 
> 52:     static MethodHandle defaultReadObjectForSerialization(Class<?> cl) {
> 53:         // build an anonymous+hidden nestmate to perform the read operation

Maybe note that we generate erased types and use MH setters/getters because the generated bytecode is defined under the loader of SerializationBytecodeGenerator, which cannot access `cl`. Otherwise, the first thing coming to my mind is that we can just use direct field access and explicit `cl` type in the bytecode, or we are supporting hidden classes for `cl` so we have to stay erased (which is not the case here)

src/java.base/share/classes/jdk/internal/reflect/SerializationBytecodeGenerator.java line 88:

> 86:                     }
> 87:                     cb.ldc(DynamicConstantDesc.ofNamed(
> 88:                         ConstantDescs.BSM_CLASS_DATA_AT,

One side remark: I think `classDataAt` is more useful for bootstrap method arguments, like `nullConstant`; in actual bytecode, `classData` + `List.get` invokeinterface should be more constant-pool friendly, as you just need one CP entry for the whole list as opposed to one CP entry for each list item.

But either way, this is correct, and we only care about correctness in legacy support code.

src/java.base/share/classes/jdk/internal/reflect/SerializationBytecodeGenerator.java line 100:

> 98:                     ClassDesc fieldDesc = fieldType.describeConstable().orElseThrow(InternalError::new);
> 99: 
> 100:                     switch (fieldDesc.descriptorString()) {

If you just switch over primitives + reference, consider something like `switch (TypeKind.from(fieldType)) {` for simplicity.

test/jdk/sun/reflect/ReflectionFactory/ReflectionFactoryTest.java line 732:

> 730:         final String final_str;
> 731: 
> 732:         Ser2(final byte final_byte, final short final_short, final char final_char, final int final_int,

I wonder if you can add a serializable user-defined class in the test; `String` is a reference but it's a primitive data type in serialization specification.

-------------

PR Review: https://git.openjdk.org/jdk/pull/19702#pullrequestreview-2150436289
PR Comment: https://git.openjdk.org/jdk/pull/19702#issuecomment-2200111311
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1660484934
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1660483713
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1660490811
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1660477343
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1660482673
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1660477372
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1660493587