Re: [EGIT PATCH 3/8] Dispose of allocated colors on finalize()

["Shawn O. Pearce" <spearce@spearce.org>]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> ---

Missing SBO.

>  .../egit/ui/internal/history/SWTPlotRenderer.java  |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java
> index 23ec255..a58b3bf 100644
> --- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java
> +++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java
> @@ -45,6 +45,17 @@ SWTPlotRenderer(final Display d) {
>  		sys_darkblue = d.getSystemColor(SWT.COLOR_DARK_BLUE);
>  	}
>  
> +	@Override
> +	protected void finalize() throws Throwable {
> +		sys_black.dispose();
> +		sys_blue.dispose();
> +		sys_gray.dispose();
> +		sys_darkblue.dispose();
> +		sys_yellow.dispose();
> +		sys_green.dispose();
> +		sys_white.dispose();
> +	}

I think this is wrong.  Any color that we get from
Display.getSystemColor() must not be disposed of by the application,
its in use by the Display and/or other parts of the application.

>From the Javadocs:

  This color should not be free'd because it was allocated by the
  system, not the application.

I take that to mean that the color should not be disposed of.

Though reading the Gtk SWT sources it seems that the system color
objects aren't pooled, each call to getSystemColor causes a new
Color instace to be allocated.  So it may be harmless on Gtk.

What was the rationale for disposing of these resources?  Did you
identify that this is a resource leak somewhere?  Because I'd like
to make sure I actually understand the SWT resource model better
so I don't commit mistakes in the future.

-- 
Shawn.