On Mon, 16 Oct 2023 16:01:39 GMT, Johannes Bechberger <jbechber...@openjdk.org>
wrote:
>> Fix `onthrow` issue by passing the event info to the `initialize` method.
>>
>> This prevents `jdb` from receiving a broken exception event and throwing an
>> internal NullPointerException, upon attaching to the JDWP-agent.
>
> Johannes Bechberger has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add test case
test/jdk/com/sun/jdi/JdwpOnThrowTest.java line 70:
> 68: long start = System.currentTimeMillis();
> 69: while (start + 1000 > System.currentTimeMillis()) {
> 70: EventSet eventSet = queue.remove(1000);
One second is not always enough in some test situations such as with -Xcomp on
a slower machine. I'd suggest giving it 10 seconds.
test/jdk/com/sun/jdi/JdwpOnThrowTest.java line 74:
> 72: while(eventIterator.hasNext() && start + 1000 >
> System.currentTimeMillis()) {
> 73: Event event = eventIterator.next();
> 74: if (event instanceof ExceptionEvent ex) {
It would be useful for future debugging to log every event. However, logging
the invalid ExceptionEvent might itself throw an exception and you would never
get to your checks below, so perhaps log after all the checks below.
test/jdk/com/sun/jdi/JdwpOnThrowTest.java line 86:
> 84: if (ex.catchLocation() == null) {
> 85: throw new RuntimeException("Exception
> catch location is null");
> 86: }
One more thing you can check here. ExceptionEvent implements Locatable, which
has a location() API. That should return the same location as
ex.thread().frame(0).location().
test/jdk/com/sun/jdi/ThrowCaughtException.java line 1:
> 1: /* /nodynamiccopyright/ */ public class ThrowCaughtException { // hard
> coded linenumbers - DO NOT CHANGE
I think you should still do a copyright. We have plenty of cases of files with
hard coded line numbers and copyrights. Also, I don't see that you are actually
hard coding the line numbers in the test. There is no check that the exception
was thrown or caught at the proper location.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16145#discussion_r1360902294
PR Review Comment: https://git.openjdk.org/jdk/pull/16145#discussion_r1360892174
PR Review Comment: https://git.openjdk.org/jdk/pull/16145#discussion_r1360889149
PR Review Comment: https://git.openjdk.org/jdk/pull/16145#discussion_r1360898071
