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

[Robin Stevens]

Gentle reminder: can I have a second person reviewing the latest webrev:

http://cr.openjdk.java.net/~rstevens/8161664/webrev.02/

Thanks,

Robin

On Thu, Jul 28, 2016 at 10:55 AM, Sergey Bylokhov <
Sergey.Bylokhov at oracle.com> wrote:

> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160803/dbf32f38/attachment.html>