RFR: 8309761: Leak class loader constraints

[Johan Sjölen]

On Sat, 10 Jun 2023 02:29:55 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

> Please review this patch to avoid leaking class loader constraints.

> > Hi,
> > Yes, this fixes the bug, but I'm confused.
> > If `ConstraintSet` determines the lifetime of the `LoaderConstraint`s, then why isn't the type of `_constraints` `GrowableArray<LoaderConstraint>*`? `loaderConstraints.cpp:161` is the only place where we create `LoaderConstraint`s on the heap and also the only place where we call `add_constraint()`. I'd rather see that change, than adding code onto the destructor. Is there some reason that this can't be done?
> > Cheers, Johan
> 
> If we make it `GrowableArray<LoaderConstraint>*`, the management of `LoaderConstraint::_loaders` would be messy. E.g., if you do
> 
> ```
> {
>    LoaderConstraint lc = _constraints->at(i);
>    // lc.~LoaderConstraint() is called here
> }
> ```
> 
> How do we know if `lc._loaders` should be freed or not?

`E& at(int i)` returns a reference, so your code has an implicit pointer dereference. Change the code to the following and we're fine:

```c++
{
  LoaderConstraint& lc = _constraints->at(i);
}


This is probably the pattern that we should utilize.


> > Hi,
> > Yes, this fixes the bug, but I'm confused.
> > If `ConstraintSet` determines the lifetime of the `LoaderConstraint`s, then why isn't the type of `_constraints` `GrowableArray<LoaderConstraint>*`? `loaderConstraints.cpp:161` is the only place where we create `LoaderConstraint`s on the heap and also the only place where we call `add_constraint()`. I'd rather see that change, than adding code onto the destructor. Is there some reason that this can't be done?
> > Cheers, Johan
> 
> I have not followed this part of code for a while, @coleenp probably is better person to ask. One potential issue I see by changing `GrowableArray<LoaderConstraint*>*` to `GrowableArray<LoaderConstraint>*`, is that, if you have a dangling `LoaderConstraint*` pointer somewhere, then you call `remove_constraint()'`, that potential overwrites the content of the pointer points to.

Thanks for the reply. That's true, but I don't see how we know whether we have a dangling pointer by the end of your destructor or not either. References aren't a 100% guarantee for that either, but it is an improvement.

I don't want to delay this PR further. It does solve an issue and Coleen approves of it. I do think that we should reconsider the choices we've made in a future RFE, however.

Cheers,
Johan

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

Marked as reviewed by jsjolen (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14407#pullrequestreview-1473647888