Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 319053

Summary:

Utility code may leak Image handles

Product:

[WebTools] WTP Source Editing

Reporter:

Nick Sandonato <nsand.dev>

Component:

wst.xml

Assignee:

Nick Sandonato <nsand.dev>

Status:

RESOLVED FIXED

QA Contact:

Nitin Dahyabhai <thatnitind>

Severity:

major

Priority:

CC:

david_williams

Version:

3.0.5

Flags:

nsand.dev: pmc_approved? (david_williams)
nsand.dev: pmc_approved? (raghunathan.srinivasan)
nsand.dev: pmc_approved? (naci.dai)
deboer: pmc_approved+
nsand.dev: pmc_approved? (neil.hauge)
nsand.dev: pmc_approved? (kaloyan)
thatnitind: review+
nsand.dev: review+

Target Milestone:

3.2.1

Hardware:

OS:

Windows XP

Whiteboard:

PMC_approved WI51593

Attachments:

Description	Flags
patch	none
counter-proposal patch leveraging the ImageRegistry for both ImageDescriptors and Images	none

Description Nick Sandonato

2010-07-06 16:27:44 EDT

+++ This bug was initially created as a clone of Bug #319048 for Helios +++

The CMImageUtility class has the potential to leak Image handles.

Calls to CMImageUtility#getImage() will retrieve a cached image descriptor, but will create a new Image from the descriptor every time. Each call with then result in one leaked Image handle.

Comment 1 Nick Sandonato

2010-07-06 16:35:15 EDT

Created attachment 173598 [details]
patch

Comment 2 Nitin Dahyabhai

2010-07-07 00:38:01 EDT

Created attachment 173615 [details]
counter-proposal patch leveraging the ImageRegistry for both ImageDescriptors and Images

I'm attaching a different patch I'd like you to review.  The use of a HashMap for caching the Images overlooks the need to dispose of the Images on plug-in stopping, and the proposed division of storing images in the XMLUI image registry and the image descriptors locally is confusing.  Storing just images in that registry also creates a placeholder descriptor to go along with them--more confusing.

Comment 3 Nick Sandonato

2010-07-07 10:29:18 EDT

Ah! This is much better. Thanks, Nitin. I tried using the ImageRegistry solely, but my issue was that I was trying to put both the ImageDescriptor and its Image in the registry causing an exception. I didn't know that putting just the ImageDescriptor in the registry was enough to cache the Image as well.

Comment 4 Nick Sandonato

2010-07-07 10:54:49 EDT

* Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such.

This issue causes one Image handle to be leaked every time the method is invoked. Over a period of time, this can cause enough handles to be leaked resulting in an SWTError for no more handles.

* Is there a work-around? If so, why do you believe the work-around is insufficient?

No.

* How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added?

The fix has been manually tested including running all unit test suites.

* Give a brief technical overview. Who has reviewed this fix?

Nitin's patch is based on mine, which he reviewed. I have subsequently reviewed Nitin's patch, the one we will be using for the fix.

Essentially, we are taking advantage of an ImageRegistry to reuse the Image handle as it's needed and so that it will be properly disposed of when shut down.

* What is the risk associated with this fix?

There is little risk.

Comment 5 Nick Sandonato

2010-07-07 14:02:28 EDT

Code released for Maintenance and HEAD.