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

[Chris Plummer]

On Mon, 16 Oct 2023 21:05:58 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:
> 
>   Update test/jdk/com/sun/jdi/JdwpOnThrowTest.java
>   
>   Co-authored-by: Chris Plummer <chris.plummer at oracle.com>

I've included a fix. I'm still running it through more thorough testing. However, I'm also wondering about this whole business of sending the ExceptionEvent in the first place. It's very "Kludgy" as already called out in a comment. The following also caught my eye:

`eventHelper_recordEvent(&info, 0, suspendPolicy, initEventBag);`

`0` is the event handlerID (sometimes known as the requestID in JDWP or JDI). At that point I realized that we may be sending an event that the debugger didn't even ask for. I don't think there is anything in the JDI spec covering delivering an event in this manner. I guess event->request() is working because JDB actually setup an EventRequest for ExceptionEvent, but what would happen if it didn't? I'm thinking of also sending a second event that I know JDB is not expecting just to see what happens.

I'm also wondering if we shouldn't just export `cbException()` and call it directly rather than mimic a lot of what it is doing, although probably what would be easier is a new API that handles the event described in the EventInfo passed to it. Maybe `cbEI()`. This would resolve the handlerID issue I point out (the event would not be sent if there is no request for it, and if there is a request it will be sent with the correct handlerID, not 0).

src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c line 730:

> 728:         if (opt_info != NULL) {
> 729:             info = *opt_info;
> 730:         }

Try the following code here. I don't think I changed anything outside this area, but since I also have your diffs present I can't reacall for sure. I also cleaned this section up a bit. It looks like it only has to deal with EI_EXCEPTION and opt_info is always set.


        /*
         * TO DO: Kludgy way of getting the triggering event to the
         * just-attached debugger. It would be nice to make this a little
         * cleaner. There is also a race condition where other events
         * can get in the queue (from other not-yet-suspended threads)
         * before this one does. (Also need to handle allocation error below?)
         */
        struct bag *initEventBag;
        LOG_MISC(("triggering_ei == EI_EXCEPTION"));
        JDI_ASSERT(triggering_ei == EI_EXCEPTION);
        JDI_ASSERT(opt_info != NULL);
        initEventBag = eventHelper_createEventBag();
        threadControl_onEventHandlerEntry(currentSessionID, opt_info, NULL);
        eventHelper_recordEvent(opt_info, 0, suspendPolicy, initEventBag);
        (void)eventHelper_reportEvents(currentSessionID, initEventBag);
        bagDestroyBag(initEventBag);

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

PR Review: https://git.openjdk.org/jdk/pull/16145#pullrequestreview-1683135388
PR Review Comment: https://git.openjdk.org/jdk/pull/16145#discussion_r1362615989