Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Mon, 18 Dec 2023 17:49:04 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Event enabled on profile.jfc but disabled on default.jfc.
src/jdk.jfr/share/classes/jdk/jfr/events/SerializationMisdeclarationEvent.java
line 41:
> 39: @Description("Methods and fields misdeclarations")
> 40: @MirrorEvent(className =
> "jdk.internal.event.SerializationMisdeclarationEvent")
> 41: @RemoveFields({"duration", "stackTrace", "thread"})
The field should be "eventThread" instead of "thread"
src/jdk.jfr/share/classes/jdk/jfr/events/SerializationMisdeclarationEvent.java
line 45:
> 43:
> 44: @Label("Class")
> 45: public Class cls;
We have often used a prefix, i.e. misdeclaredClass, to avoid using a reserved
word. We try to stay out of abbreviations.
src/jdk.jfr/share/classes/jdk/jfr/events/SerializationMisdeclarationEvent.java
line 48:
> 46:
> 47: @Label("Kind")
> 48: public int kind;
What is the use case for error codes? Are they public or an implementation
detail?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431226200
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431227843
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431229900
Re: RFR: 8313367: SunMSCAPI cannot read Local Computer certs w/o Windows elevation
On Thu, 16 Nov 2023 12:06:26 GMT, rebarbora-mckvak wrote: > This fixes the defect described at https://bugs.openjdk.org/browse/JDK-8313367 > > If the process does not have write permissions, the store is opened as > read-only (instead of failing). > > Please note that permissions to use a certificate in a local machine store > must be granted - in a management console, select a certificate, right-click > -> All tasks... -> Manage Private Keys... -> add Full control to user. This is a very trivial change fixing rather annoying bug. Can someone review it and let it merge? - PR Comment: https://git.openjdk.org/jdk/pull/16687#issuecomment-1862618406
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
> Adds serialization misdeclaration events to JFR. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Better name for a label, corrected name of removed field. - Changes: - all: https://git.openjdk.org/jdk/pull/17129/files - new: https://git.openjdk.org/jdk/pull/17129/files/8534d7af..51507e4e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17129&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17129&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17129.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17129/head:pull/17129 PR: https://git.openjdk.org/jdk/pull/17129
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Tue, 19 Dec 2023 10:43:57 GMT, Erik Gahlin wrote:
>> Raffaello Giulietti has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Event enabled on profile.jfc but disabled on default.jfc.
>
> src/jdk.jfr/share/classes/jdk/jfr/events/SerializationMisdeclarationEvent.java
> line 48:
>
>> 46:
>> 47: @Label("Kind")
>> 48: public int kind;
>
> What is the use case for error codes? Are they public or an implementation
> detail?
The intent is that they are stable and for programmatic usage, whereas the
message is more for human consumption. The codes are used in the test, for
example, and are declared as public static in the event classes.
Alternatively, one could parse the message, but that's less robust in face of
changes, I think.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431329983
Re: RFR: 8313367: SunMSCAPI cannot read Local Computer certs w/o Windows elevation
On Thu, 16 Nov 2023 12:06:26 GMT, rebarbora-mckvak wrote: > This fixes the defect described at https://bugs.openjdk.org/browse/JDK-8313367 > > If the process does not have write permissions, the store is opened as > read-only (instead of failing). > > Please note that permissions to use a certificate in a local machine store > must be granted - in a management console, select a certificate, right-click > -> All tasks... -> Manage Private Keys... -> add Full control to user. @macarte, can you take a look at this fix? Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/16687#issuecomment-1862784812
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 GMT, Raffaello Giulietti wrote: >> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Better name for a label, corrected name of removed field. Hello Raffaello, > On the other hand, the checks and the events generation are done for > serializable classes that actually actively participate in serialization, > lazily, and just once per class Is it per class for each classloader that loads it? Or is it per class per JVM? It's more out of curiosity than anything else because I don't think it makes a big difference (I don't expect too many classloaders that would lead to the case of extremely large streams of events). - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1862882260
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Better name for a label, corrected name of removed field.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 94:
> 92: if (!isPrivate(f)) {
> 93: commitEvent(SUID_PRIVATE,
> 94: SUID_NAME + " should be declared private");
Rest of the event messages use "... to be effective", should this one too use a
similar text?
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 108:
> 106: SUID_NAME + " should be declared of type long");
> 107: }
> 108: if (!isStatic(f)) {
Nit - perhaps save the return value from the previous call to `isStatic(f)` a
few lines above and reuse it here?
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 112:
> 110: }
> 111: f.setAccessible(true);
> 112: if (getLong(f) == null) {
Is this check and the `setAccessible()` needed if a few lines above, the call
to `f.getType()` returns `Long.TYPE` (i.e. primitive type)? Perhaps we can
conditionally do these additional checks and calls, only if `f.getType()` isn't
a primitive?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431512491
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431506705
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431511005
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 GMT, Raffaello Giulietti wrote: >> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Better name for a label, corrected name of removed field. src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 185: > 183: commitEvent(PRIV_METH_NON_STATIC, > 184: m + " must be non-static to be effective"); > 185: } Should there also be a check to see if this `private` method is (misdeclared) as a `native` method? - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431524703
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Better name for a label, corrected name of removed field.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 276:
> 274: }
> 275:
> 276: private static Object getObject(Field f) {
Should we call this method `getStaticFieldValue(...)`, because that's what the
implementation is.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431535049
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 GMT, Raffaello Giulietti wrote: >> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Better name for a label, corrected name of removed field. src/java.base/share/classes/jdk/internal/event/SerializationMisdeclarationEvent.java line 94: > 92: > 93: /* > 94: * These constants are not final on purpose. Just curious - what's the purpose? I don't see them being changed/updated anywhere (not even in tests). - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431551183
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Better name for a label, corrected name of removed field.
test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 112:
> 110: @MethodSource
> 111: public void testGoodClass(Class cls) {
> 112: assertEquals(0, getEventsFor(cls).size());
Can this and other assertions in this test be updated to include the class name
which failed? You can still use `assertEquals(...)`, it takes an optional
message as a third parameter which you could use to include the failing class
name. It becomes a bit more easier to debug (unexpected) failures when the
assertion includes these additional details.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431563775
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Better name for a label, corrected name of removed field.
test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 144:
> 142: }
> 143:
> 144: static {
It's a bit odd for a test case to be doing this in a static block. Could this
instead be done in a `org.junit.jupiter.api.BeforeAll` method:
import org.junit.jupiter.api.BeforeAll;
...
@BeforeAll
static void recordEvents() {
// that static block's code here
}
Any (unexpected) failures in that method will then be reported in a better way
by the testing framework instead of an `ExceptionInInitializer` that would be
reported from a failure in static block and the test class itself failing to
load.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431571145
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 GMT, Raffaello Giulietti wrote: >> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Better name for a label, corrected name of removed field. test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 50: > 48: * @requires vm.hasJFR > 49: * @library /test/lib > 50: * @run junit/othervm > jdk.jfr.event.io.TestSerializationMisdeclarationEvent Is the use of "othervm" needed here because of the use of `jdk.jfr.consumer.RecordingStream`? Does `RecordingStream` do JVM wide state changes? I did a quick look at that class but couldn't come to a conclusion. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431578942
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 14:39:47 GMT, Jaikiran Pai wrote:
> Is it per class for each classloader that loads it? Or is it per class per
> JVM? It's more out of curiosity than anything else because I don't think it
> makes a big difference (I don't expect too many classloaders that would lead
> to the case of extremely large streams of events).
The checks are done on the `Class` instance, that is, per each defined (as
per JVMS) and _used_ serializable class, on first usage in serialization. If
enabled at all, they are invoked by the private `ObjectStreamClass` constructor.
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 94:
>
>> 92: if (!isPrivate(f)) {
>> 93: commitEvent(SUID_PRIVATE,
>> 94: SUID_NAME + " should be declared private");
>
> Rest of the event messages use "... to be effective", should this one too use
> a similar text?
The field needs not be declared `private` to be effective, although it is
recommended to do so. In this sense, "should" is just a recommendation, while
"must" is really needed to make something effective.
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 108:
>
>> 106: SUID_NAME + " should be declared of type long");
>> 107: }
>> 108: if (!isStatic(f)) {
>
> Nit - perhaps save the return value from the previous call to `isStatic(f)` a
> few lines above and reuse it here?
Assuming that most serializable classes are declared correctly, I don't think
that caching the value of `isStatic()` makes a measurable difference.
If at all, then I'd prefer to cache the modifiers and "inline" the masking
logic of the individual is*() methods.
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 112:
>
>> 110: }
>> 111: f.setAccessible(true);
>> 112: if (getLong(f) == null) {
>
> Is this check and the `setAccessible()` needed if a few lines above, the call
> to `f.getType()` returns `Long.TYPE` (i.e. primitive type)? Perhaps we can
> conditionally do these additional checks and calls, only if `f.getType()`
> isn't a primitive?
Right, will be addressed in the next commit.
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 185:
>
>> 183: commitEvent(PRIV_METH_NON_STATIC,
>> 184: m + " must be non-static to be effective");
>> 185: }
>
> Should there also be a check to see if this `private` method is (misdeclared)
> as a `native` method?
I'm not sure that a `native` method is not considered effective by
serialization. I have to check.
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 276:
>
>> 274: }
>> 275:
>> 276: private static Object getObject(Field f) {
>
> Should we call this method `getStaticFieldValue(...)`, because that's what
> the implementation is.
Since the only parameter is `Field`, it has to be a static field almost for
sure.
Further, there's a `getLong()` method down below that operates on a static
field as well and that one would also need a renaming.
If I can come up with better names they will be in the next commit.
> src/java.base/share/classes/jdk/internal/event/SerializationMisdeclarationEvent.java
> line 94:
>
>> 92:
>> 93: /*
>> 94: * These constants are not final on purpose.
>
> Just curious - what's the purpose? I don't see them being changed/updated
> anywhere (not even in tests).
Declaring a `public static int` field `final` means that the value is then
stored in the client class. If a value is changed, then the client needs to be
recompiled as well, but this is not enforced by the language, so it might lead
to inconsistencies.
The clean solution would be to use an enum class, but that's not supported by
JFR.
> test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 50:
>
>> 48: * @requires vm.hasJFR
>> 49: * @library /test/lib
>> 50: * @run junit/othervm
>> jdk.jfr.event.io.TestSerializationMisdeclarationEvent
>
> Is the use of "othervm" needed here because of the use of
> `jdk.jfr.consumer.RecordingStream`? Does `RecordingStream` do JVM wide state
> changes? I did a quick look at that class but couldn't come to a conclusion.
Not sure, I have to check.
> test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 112:
>
>> 110: @MethodSource
>> 111: public void testGoodClass(Class cls) {
>> 112: assertEquals(0, getEventsFor(cls).size());
>
> Can this and other assertions in this test be updated to include the class
> name which failed? You can still use `assertEquals(...)`, it takes an
> optional message as a third parameter which you could use to include the
> failing class name. It becomes a bit more easier to debug (unexpected)
> failures when the assertion includes these additional details.
Thanks for the suggestion, will ta
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 15:56:39 GMT, Raffaello Giulietti wrote: > > Is it per class for each classloader that loads it? Or is it per class per > > JVM? It's more out of curiosity than anything else because I don't think it > > makes a big difference (I don't expect too many classloaders that would > > lead to the case of extremely large streams of events). > > The checks are done on the `Class` instance, that is, per each defined (as > per JVMS) and _used_ serializable class, on first usage in serialization. If > enabled at all, they are invoked by the private `ObjectStreamClass` > constructor. Well, in fact `ObjectStreamClass` maintains a cache of `Class` -> `ObjectStreamClass` to avoid creating a new instance of `ObjectStreamClass` each time a `Class` is looked up. It memoizes the association the first time. However, the cache can be emptied under high memory pressure, so the `ObjectStreamClass` instance might be recreated later, thus re-invoking the serialization checker once again. - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1863090984
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
> Adds serialization misdeclaration events to JFR. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: Changes according to reviewer's comments. - Changes: - all: https://git.openjdk.org/jdk/pull/17129/files - new: https://git.openjdk.org/jdk/pull/17129/files/51507e4e..fa04bf48 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17129&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17129&range=03-04 Stats: 44 lines in 2 files changed: 15 ins; 14 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/17129.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17129/head:pull/17129 PR: https://git.openjdk.org/jdk/pull/17129
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Tue, 19 Dec 2023 12:17:38 GMT, Raffaello Giulietti
wrote:
>> src/jdk.jfr/share/classes/jdk/jfr/events/SerializationMisdeclarationEvent.java
>> line 48:
>>
>>> 46:
>>> 47: @Label("Kind")
>>> 48: public int kind;
>>
>> What is the use case for error codes? Are they public or an implementation
>> detail?
>
> The intent is that they are stable and for programmatic usage, whereas the
> message is more for human consumption. The codes are used in the test, for
> example, and are declared as public static in the event classes.
>
> Alternatively, one could parse the message, but that's less robust in face of
> changes, I think.
Users (not OpenJDK developers) don't know what the error code means. I think
it's better to not have them. This is how other events work. If you want to
guard against changes, I would export the package to the test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431716132
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Tue, 19 Dec 2023 16:45:04 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Changes according to reviewer's comments.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 39:
> 37: import static java.lang.reflect.Modifier.*;
> 38:
> 39: final class SerializationMisdeclarationChecker {
Is there a reason this event is placed in java.io package and not
jdk.internal.event like other events?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431717967
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 16:00:59 GMT, Raffaello Giulietti wrote: >> test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 50: >> >>> 48: * @requires vm.hasJFR >>> 49: * @library /test/lib >>> 50: * @run junit/othervm >>> jdk.jfr.event.io.TestSerializationMisdeclarationEvent >> >> Is the use of "othervm" needed here because of the use of >> `jdk.jfr.consumer.RecordingStream`? Does `RecordingStream` do JVM wide state >> changes? I did a quick look at that class but couldn't come to a conclusion. > > Not sure, I have to check. All the other events run in othervm. While it may be possible in some case to not do that, it's much easier to analyse issues, if we are sure the JVM is fresh. For example, JFR may not generate bytecode if an event is disabled. The JFR tests have been hardened to be able to run in parallel, so they run pretty fast. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431727571
Re: RFR: 8313367: SunMSCAPI cannot read Local Computer certs w/o Windows elevation
On Thu, 16 Nov 2023 12:06:26 GMT, rebarbora-mckvak wrote: > This fixes the defect described at https://bugs.openjdk.org/browse/JDK-8313367 > > If the process does not have write permissions, the store is opened as > read-only (instead of failing). > > Please note that permissions to use a certificate in a local machine store > must be granted - in a management console, select a certificate, right-click > -> All tasks... -> Manage Private Keys... -> add Full control to user. Apologies I wasn't aware of the JBS issue until I saw this github notification. At a glance the fix seems trivial, but I'll need to test it. We were planning on looking at supporting the ability to open a keystore with READONLY access and I emailed security-dev to this effect in May Considering this change, I'd suggest that when the store is opened with read-only permissions that some warning is output (if this can't be detected then we may have to attempt to open with write-priviledges and the fall back to read-only (CERT_STORE_READONLY_FLAG). @rebarbora-mckvak - what testing was done with an elevated user opening a keystore with (CERT_STORE_MAXIMUM_ALLOWED_FLAG) and then attempting write-operations on the keystore? - PR Comment: https://git.openjdk.org/jdk/pull/16687#issuecomment-1863211291
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 16:28:03 GMT, Raffaello Giulietti wrote: > However, the cache can be emptied under high memory pressure, so the > `ObjectStreamClass` instance might be recreated later, thus re-invoking the > serialization checker once again. I think it would be good to state in the description when checks occur, something like: Only the first time a class is serialized are the misdeclaration checks carried out, but may happen happen more frequently under high memory pressure. (Try to avoid using the word "event") - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1863216232
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Tue, 19 Dec 2023 17:15:40 GMT, Erik Gahlin wrote:
>> Raffaello Giulietti has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Changes according to reviewer's comments.
>
> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
> line 39:
>
>> 37: import static java.lang.reflect.Modifier.*;
>> 38:
>> 39: final class SerializationMisdeclarationChecker {
>
> Is there a reason this event is placed in java.io package and not
> jdk.internal.event like other events?
The mirror event itself and the JFR event are placed in their standard
locations.
Only the checker is in `java.io`, so I'm not sure what you mean here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431740613
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Tue, 19 Dec 2023 17:13:58 GMT, Erik Gahlin wrote: >> The intent is that they are stable and for programmatic usage, whereas the >> message is more for human consumption. The codes are used in the test, for >> example, and are declared as public static in the event classes. >> >> Alternatively, one could parse the message, but that's less robust in face >> of changes, I think. > > Users (not OpenJDK developers) don't know what the error code means. I think > it's better to not have them. This is how other events work. If you want to > guard against changes, I would export the package to the test. What about fixed `String`s rather than `int`s for the kind of error? Something like `"SUID_INEFFECTIVE_ON_ENUM"`, and so on? It would be nice to be able to use enums, but AFAIK that's not supported in JFR. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431744479
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Tue, 19 Dec 2023 16:45:04 GMT, Raffaello Giulietti wrote: >> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Changes according to reviewer's comments. You mean, in the @Description annotation? - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1863223947
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Tue, 19 Dec 2023 16:45:04 GMT, Raffaello Giulietti wrote: >> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Changes according to reviewer's comments. > You mean, in the @description annotation? Yes. - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1863235723
Re: RFR: 8275338: Add JFR events for notable serialization situations [v5]
On Tue, 19 Dec 2023 17:37:50 GMT, Raffaello Giulietti
wrote:
>> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
>> line 39:
>>
>>> 37: import static java.lang.reflect.Modifier.*;
>>> 38:
>>> 39: final class SerializationMisdeclarationChecker {
>>
>> Is there a reason this event is placed in java.io package and not
>> jdk.internal.event like other events?
>
> The mirror event itself and the JFR event are placed in their standard
> locations.
> Only the checker is in `java.io`, so I'm not sure what you mean here.
Sorry, my fault. Github showed "SerializationMisdeclar..." and I thought it was
the event.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431762931
Re: RFR: 8313367: SunMSCAPI cannot read Local Computer certs w/o Windows elevation
On Thu, 16 Nov 2023 12:06:26 GMT, rebarbora-mckvak wrote: > This fixes the defect described at https://bugs.openjdk.org/browse/JDK-8313367 > > If the process does not have write permissions, the store is opened as > read-only (instead of failing). > > Please note that permissions to use a certificate in a local machine store > must be granted - in a management console, select a certificate, right-click > -> All tasks... -> Manage Private Keys... -> add Full control to user. Also, when a keystore is read-only. What happens when one tries to write into it? Ideally a `KeyStoreException` should be thrown with a clear and precise message. - PR Comment: https://git.openjdk.org/jdk/pull/16687#issuecomment-1863285096
Re: RFR: 8275338: Add JFR events for notable serialization situations [v3]
On Tue, 19 Dec 2023 17:41:57 GMT, Raffaello Giulietti wrote: >> Users (not OpenJDK developers) don't know what the error code means. I think >> it's better to not have them. This is how other events work. If you want to >> guard against changes, I would export the package to the test. > > What about fixed `String`s rather than `int`s for the kind of error? > Something like `"SUID_INEFFECTIVE_ON_ENUM"`, and so on? > It would be nice to be able to use enums, but AFAIK that's not supported in > JFR. You could define them with an Enum but use the ordinal as the value for JFR. - PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431864329
Re: RFR: 8275338: Add JFR events for notable serialization situations [v4]
On Tue, 19 Dec 2023 12:21:05 GMT, Raffaello Giulietti
wrote:
>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Better name for a label, corrected name of removed field.
src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java
line 108:
> 106: SUID_NAME + " should be declared of type long");
> 107: }
> 108: if (!isStatic(f)) {
The two calls to isStatic could be reordered closer together to be a single if
(isstatic()) { ... } else {... }.
src/java.base/share/classes/jdk/internal/event/SerializationMisdeclarationEvent.java
line 126:
> 124: public static int ACC_METH_PARAM_TYPES= 404;
> 125: public static int ACC_METH_NON_ACCESSIBLE = 405;
> 126:
I'd rather see few (fewer, just one) kinds of events, with so many different
kinds of events there needs to be a convenient method to look for any kind of
serialization mis-declaration.
Perhaps a single event with flags for the different kinds of errors.
The event classes could be responsible for turning the flags back into useful
messages on display.
test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 157:
> 155: }
> 156:
> 157: private static class A implements Serializable {
The test classes should document the good or badness of the class either in the
class/field/method names or in comments. What's wrong with this XXX.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431846034
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431850091
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431856800
