<Swing Dev> [9] Review Request: 8161664: Memory leak in com.apple.laf.AquaProgressBarUI: removed progress bar still referenced

[Sergey Bylokhov]

Thanks! Looks fine.

On 28.07.16 0:16, Robin Stevens wrote:
> Here is the updated webrev which includes the comma in the copyright and
> the extra tag:
>
> http://cr.openjdk.java.net/~rstevens/8161664/webrev.02/
>
> Robin
>
> On Wed, Jul 27, 2016 at 12:08 AM, Sergey Bylokhov
> <Sergey.Bylokhov at oracle.com <mailto:Sergey.Bylokhov at oracle.com>> wrote:
>
>     Sorry I missed two small issues in the previous review:
>      - The date in copyright should have a comma "Copyright (c) 2016,
>     Oracle and/or its affiliates. All rights reserved."
>      - The test should have "* @key headful" tag.
>
>     On 27.07.16 0:13, Robin Stevens wrote:
>
>         Hello,
>
>         thank you both for the review.
>
>         Updated webrev:
>         http://cr.openjdk.java.net/~rstevens/8161664/webrev.01/
>
>         I have adjusted the test as suggested by Sergey:
>
>         - I moved it to the javax/swing/JProgressBar package, as it is
>         no longer
>         specific for the Aqua look and feel
>         - The test now loops over all installed look and feels. For that, I
>         copied code from the test in
>         javax/swing/JTab;e/7124218/SelectEditTableCell.java
>         - The test now uses Util.generateOOME instead of the System.gc +
>         System.finalization call
>
>
>         Robin
>
>         On Mon, Jul 25, 2016 at 10:32 PM, Sergey Bylokhov
>         <Sergey.Bylokhov at oracle.com <mailto:Sergey.Bylokhov at oracle.com>
>         <mailto:Sergey.Bylokhov at oracle.com
>         <mailto:Sergey.Bylokhov at oracle.com>>> wrote:
>
>             Hi, Robin.
>             These calls do no guaranties that all weak references will
>         be collected.
>             56     System.runFinalization();
>             57     System.gc();
>
>             I suggest to generate OOM explicitly. see for example:
>             javax/swing/UIDefaults/6795356/bug6795356.java
>
>             Note that on other platforms it will cover Metal L&F but
>         probably
>             others can be tested as well. So it will be good to test all
>             installed L&F in the loop.
>
>
>             On 20.07.16 14:26, Alexandr Scherbatiy wrote:
>
>
>                 The fix looks good to me.
>
>                 Thanks,
>                 Alexandr.
>
>                 On 7/20/2016 11:37 AM, Robin Stevens wrote:
>
>                     Hello,
>
>                     please review this patch for issue JDK-8161664:
>
>                     Bug: https://bugs.openjdk.java.net/browse/JDK-8161664
>                     Patch:
>         http://cr.openjdk.java.net/~rstevens/8161664/webrev.00/
>
>         <http://cr.openjdk.java.net/%7Erstevens/8161664/webrev.00/>
>
>
>                     The problem is that in certain scenarios, the Timer
>         in the
>                     Animator of
>                     the AquaProgressBarUI does not get stopped when the
>         progress
>                     bar is
>                     removed from the Swing hierarchy.
>
>                     Several approaches were discussed in another thread
>
>         (http://mail.openjdk.java.net/pipermail/swing-dev/2016-July/006330.html).
>                     In the linked webrev, I opted to use the same
>         approach as
>                     what is done
>                     in the BasicProgressBarUI class: only start the
>         timer when the
>                     progress bar is displayable.
>
>                     To achieve this, I wrapped all calls to
>         startAnimationTimer
>                     with an if
>                     statement that checks the displayable state of the
>         progress bar.
>
>                     There is one call to startAnimationTimer that is not
>                     adjusted. That
>                     call is only triggered from the paint method, which
>         in turn
>                     is only
>                     triggered if the progress bar is displayable. As
>         such, the
>                     if check
>                     was not needed there (imo).
>
>                     The patch includes a test, which fails without the
>         fix and
>                     succeeds
>                     afterwards.
>
>                     Thanks,
>
>                     Robin
>
>
>
>
>             --
>             Best regards, Sergey.
>
>
>
>
>     --
>     Best regards, Sergey.
>
>


-- 
Best regards, Sergey.