On Mon, 8 Jan 2024 13:48:06 GMT, Raffaello Giulietti <rgiulie...@openjdk.org> wrote:
>> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request with a new target base due > to a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 15 additional > commits since the last revision: > > - Merge branch 'master' into 8275338 > - Simplified event messages. > Remove ckecker allocation. > - Corrected @Label of event and of field. > - Removed @module from test. > - Merge branch 'master' into 8275338 > - Renamed an event field. > - Minor changes. > - Removed event kind. > serialVersionUID must have type long. > Test now base on keyword search in event message. > Commented test classes about misdeclarations. > - Changes according to reviewer's comments. > - Better name for a label, corrected name of removed field. > - ... and 5 more: https://git.openjdk.org/jdk/compare/8d57faa7...9ca1f36d src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 53: > 51: private static final Class<?>[] READ_OBJECT_NO_DATA_PARAM_TYPES = {}; > 52: private static final Class<?>[] WRITE_REPLACE_PARAM_TYPES = {}; > 53: private static final Class<?>[] READ_RESOLVE_PARAM_TYPES = {}; These could share a single zero length Class<?> array. src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 70: > 68: privilegedCheckAccessibleMethod(cl, WRITE_REPLACE_NAME, > 69: WRITE_REPLACE_PARAM_TYPES, Object.class); > 70: privilegedCheckAccessibleMethod(cl, READ_RESOLVE_NAME, Thinking ahead to when the security manager is removed, can the code that needs private access to field values (SUID) be more narrowly accessed? Perhaps switch to using a VarHandle and MethodHandles.Lookup. This may be a longer term issue and beyond the scope of this PR. In the naming of the `PrivilegedXXX` methods, I would drop the "privileged" part of the name. The methods do not change the privilege level and operate correctly if when the caller has the privileges needed. And all of these methods are read-only so there is no/little risk in their implementations and avoid refitting the terminology later. src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 87: > 85: } > 86: if (cl.isEnum()) { > 87: commitEvent(cl, SUID_NAME + " in an enum class is not > effective"); Is there a best practice that can be included in the message? "SUID should not be declared"? src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 113: > 111: } else if (cl.isEnum()) { > 112: commitEvent(cl, SERIAL_PERSISTENT_FIELDS_NAME + > 113: " in an enum class is not effective"); Is there best practice to include in the message? "SPFN should not be declared"? test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 33: > 31: import org.junit.jupiter.params.provider.MethodSource; > 32: > 33: import java.io.*; Explicit imports are preferred. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1445102883 PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1445129715 PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1445107271 PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1445108891 PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1445137904
