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

[Robin Stevens]

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