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

Bug 321658

Summary:

Debug leaks WorkbenchWindow

Product:

[Eclipse Project] Platform

Reporter:

Markus Keller <markus.kell.r>

Component:

Debug

Assignee:

Pawel Piech <pawel.1.piech>

Status:

VERIFIED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

daniel_megert, darin.eclipse, pawel.1.piech, remy.suen

Version:

3.7

Flags:

pawel.1.piech: review+

Target Milestone:

3.7 M2

Hardware:

OS:

Mac OS X

Whiteboard:

Attachments:

Description	Flags
Patch with leak fixes.	pawel.1.piech: review?
patch	none

Description Markus Keller

2010-08-03 18:50:42 EDT

I20100802-1800

Debug leaks WorkbenchWindows in multiple ways. After
- Window > New Window
- switch to Debug perspective
- close window
there are multiple paths through which a WorkbenchWindow is leaked:

- ViewContextManager#windowClosed(..) should call fWindowToService.remove(window)
- SourceLookupManager#windowClosed(..) should call fServices.remove(window)
- AbstractBreadcrumb#dispose() should remove display listeners, but it's only called by LaunchViewBreadcrumb#dispose() which in turn is never called
- maybe more paths

Comment 1 Pawel Piech

2010-08-04 13:29:10 EDT

Created attachment 175855 [details]
Patch with leak fixes.

In addition to the leaks pointed out I also found:

- ToggleStepFilter action is never disposed.
- PresentationContext doesn't reset fWindow on dispose
- ViewContextService doesn't reset fWindow on dispose
- SourceLookupService doesn't reset fWindow on dispose

The reason that the mentioned hash maps don't remove the reference upon windowClosed() call, as pointed out, is that those managers are often called by clients with a cached reference to the closed window.  When that happens the managers end up creating a new service instance for that window and thus leak the window reference anyway.  The simplest solution I can think of is to have these managers hold a weak reference to the window instead.  Any objections?

Comment 2 Pawel Piech

2010-08-05 14:29:41 EDT

Comment on attachment 175855 [details]
Patch with leak fixes.

I forgot to set the review flag.  Darin, could you take a look at these changes and let me know what you think.  I'll be away for next three weeks so if you like the changes please go ahead and apply them.

Comment 3 Darin Wright

2010-08-05 14:40:30 EDT

Sure, I'll take a look once M1 has shipped.

Comment 4 Darin Wright

2010-08-06 13:59:52 EDT

Created attachment 176056 [details]
patch

You can avoid using weak hash maps with this patch. A "null" debug context service is returned when a window has been closed. Not sure which approach is better.

Comment 5 Darin Wright

2010-08-09 15:55:32 EDT

I released the 2nd patch for now. All tests pass.

Comment 6 Pawel Piech

2010-08-11 16:50:35 EDT

I'm not crazy about the weak references either, so the null-service approach seems like a good idea to me.

Comment 7 Pawel Piech

2011-05-06 18:23:18 EDT

Reviewed.