<Swing Dev> [9] Review Request: 8161664: Memory leak in com.apple.laf.AquaProgressBarUI: removed progress bar still referenced
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Mon Aug 8 07:41:45 UTC 2016
The fix looks good to me.
Thanks,
Alexandr.
On 03/08/16 14:05, Robin Stevens wrote:
> Gentle reminder: can I have a second person reviewing the latest webrev:
>
> http://cr.openjdk.java.net/~rstevens/8161664/webrev.02/
> <http://cr.openjdk.java.net/%7Erstevens/8161664/webrev.02/>
>
> Thanks,
>
> Robin
>
> On Thu, Jul 28, 2016 at 10:55 AM, Sergey Bylokhov
> <Sergey.Bylokhov at oracle.com <mailto: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/
> <http://cr.openjdk.java.net/%7Erstevens/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>
> <mailto: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/
> <http://cr.openjdk.java.net/%7Erstevens/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>>
> <mailto: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/>
>
>
> <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/20160808/8c236a76/attachment.html>
More information about the swing-dev
mailing list