RFR: 8281615: Deadlock caused by jdwp agent [v4]

[Zhengyu Gu]

On Thu, 17 Feb 2022 05:57:24 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

> processUnloads is continuously called on every event. It's purpose is to process pending CLASS_UNLOAD events that have accumulated since the last JVMTI event. This is a rather hacky solution. JVMTI does not actually send a CLASS_UNLOAD event. It doesn't support them. What it does support is OBJECT_FREE events. So when the debugger has enabled CLASS_UNLOAD events, the debug agent enables OBJECT_FEE events. It tags each j.l.Class instance so it will get the OBJECT_FREE event for them, and when the event comes in, the Class gets added to deletedSignatures so the agent can later notify the debugger about it.
> 
> As a side note, this OBJECT_FREE solution is somewhat new. The old solution was to always do an "allClasses" during initialization, and then diff that with "allClasses" every time there was a full GC to see which classes unloaded. I wonder now if there isn't a reason we just don't send the CLASS_UNLOAD when the OBJECT_FREE is received. It would probably greatly simplify classTrack.c, and will fix an outstanding bug we have where sometimes there is a lengthy delay before CLASS_UNLOAD events are sent. This is because once there is a GC, they events are not sent until the next JVMTI event. It's actually possible there would never be one if the only thing the debugger is requesting is CLASS_UNLOAD events.
> 
> Anyway, back to processUnloads synchronization. The reason for synchronization between processUnload and activate is because one thread could be handling some random JVMTI event (maybe a breakpoint) and is calling processUnloads, and at the same time another thread is handling a CLASS_UNLOAD event request from the debugger, and calling activate. But now that I think of it, this should be harmless even without synchronization. The processUnloads thread will see the allocated deletedSignatures if activate has gotten that far, otherwise it won't. It should behave correctly in either case. I'm not convince that activate even needs synchronization. The purpose of the classTrack synchronization is to prevent one thread in processUnloads from grabbing and using deletedSignatures, only to have another thread in processUnloads do the same, and delete deletedSignatures in the process, leaving the 1st thread holding onto a deallocated pointer. However, even this should not be possible because
  all callers to processUnloads have already grabbed the handlerLock.
> 
> What I'm tempted to say here is just have this PR keep all the synchronization and I'll file a CR to possibly do a lot of cleanup here. Either remove (maybe all of) the synchronization, or maybe even implement sending the CLASS_UNLOAD when the OBJECT_FREE comes in, which will make the synchronization question moot, and also fix that other bug I mentioned.
> 
> BTW, one other bit of nonsense. classTrack_activate() does:
> 
> ```
> deletedSignatures = bagCreateBag(sizeof(char*), 1000);
> ```
> 
> This will be deleted the very first time an event comes in, even if no classes were unloaded, and processUnloads replaces it with:
> 
> ```
> deletedSignatures = bagCreateBag(sizeof(char*), 10);
> ```

I believe checking `bagSize() == 0` is an optimization. I think just taking `handlerLock` lock before calling `classTrack_reset()` ensures no race can happen and seems harmless.

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

PR: https://git.openjdk.java.net/jdk/pull/7461