RFR: JDK-8317920: JDWP-agent sends broken exception event with onthrow option [v2]

[Chris Plummer]

On Mon, 16 Oct 2023 16:01:39 GMT, Johannes Bechberger <jbechberger at 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