Re: [Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator

[David Grieve]

Github failed me. My 'remove' comment was applied to the System.out.println, 
not the assert. 

> -----Original Message-----
> From: openjfx-dev <openjfx-dev-boun...@openjdk.java.net> On Behalf Of
> David Grieve
> Sent: Thursday, December 19, 2019 2:29 PM
> To: openjfx-dev@openjdk.java.net
> Subject: [EXTERNAL] Re: [Rev 01] RFR: 8236259: MemoryLeak in
> ProgressIndicator
> 
> On Thu, 19 Dec 2019 19:28:34 GMT, Florian Kirmaier
> <fkirma...@openjdk.org> wrote:
> 
> >> Hi everyone,
> >>
> >> ticket:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs
> .openjdk.java.net%2Fbrowse%2FJDK-
> 8236259&amp;data=02%7C01%7CDavid.Grieve%40microsoft.com%7C856e05
> 0ee1bf484a7a7008d784b9dde4%7C72f988bf86f141af91ab2d7cd011db47%7C1
> %7C0%7C637123806135627853&amp;sdata=yMuZMaBg7UmJEWzn8AGI6K8f5
> aRmFclOi4nov%2BLas%2BM%3D&amp;reserved=0
> >>
> >> The fix itself is quite straight forward.
> >> It basically just removed the listener which causes the leak.
> >>
> >> The unit-test for the fix is a bit more complicated.
> >>
> >> I added a library JMemoryBuddy
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2FSandec%2FJMemoryBuddy&amp;data=02%7C01%7CDavid.Griev
> e%40microsoft.com%7C856e050ee1bf484a7a7008d784b9dde4%7C72f988bf8
> 6f141af91ab2d7cd011db47%7C1%7C0%7C637123806135627853&amp;sdata=6
> a1cOZEa9Ah%2F47NyvrRetz9YZyTgXhoJ1b13re8SZw8%3D&amp;reserved=0
> (written by myself), which simplifies testing for memory leaks.
> >> I think there are many places in the JavaFX-codebase that could highly
> benefit from this library.
> >> It could also simplify many of the already existing unit tests.
> >> It makes testing for memory-leaks readably and reliable.
> >> It would also be possible to just copy the code of the library into the
> JavaFX-codebase.
> >> It only contains a single class.
> >>
> >> I also had to make a method public, to write the test. I'm open to ideas,
> how I could solve it differently.
> >
> > The pull request has been updated with 1 additional commit.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/Progr
> essIndicatorSkinTest.java line 106:
> 
> > 105:             Node detIndicator = 
> > indicator.getChildrenUnmodifiable().get(0);
> > 106:             
> > System.out.println(detIndicator.getClass().getSimpleName());
> > 107:             checker.assertCollectable(detIndicator);
> 
> remove
> 
> -------------
> 
> PR:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.o
> penjdk.java.net%2Fjfx%2Fpull%2F71&amp;data=02%7C01%7CDavid.Grieve%
> 40microsoft.com%7C856e050ee1bf484a7a7008d784b9dde4%7C72f988bf86f1
> 41af91ab2d7cd011db47%7C1%7C0%7C637123806135627853&amp;sdata=ATF
> nmAXAw0x0ENGrqp%2Fde8fEuIg7mrQrYZmXn4gLnug%3D&amp;reserved=0