Bug 283586

Comment 1 Teodor Madan 2009-10-12 04:52:25 EDT

There's multiple scenarios when memory rendering should be disposed:
- Closing "Memory Browser" view;
- Closing a CTabItem
- A launch is terminated and removed from debug view

Not releasing memory rendering could lead to significant degradation. e.g. traditional memory rendering will leak a viewport cache thread, colors and pop-ul menus

Comment 2 Teodor Madan 2009-10-12 04:53:51 EDT

Created attachment 149346 [details]
Fix for rendering leaks

Fix disposing renderings

Comment 3 Ted Williams 2009-10-16 16:51:21 EDT

Randy, please review and commit.

Comment 4 Randy Rohrbach 2009-10-18 00:54:10 EDT

Ted

   I committed these changes to HEAD after reworking them a little.
   Please review.

Randy

Comment 5 Teodor Madan 2010-01-19 08:58:58 EST

Created attachment 156501 [details]
Fix to work for both CDT and DSF

The previous fix has several issues:
1. Does not work for non standard debug models (e.g. DSF-MI) that do not expose IDebugTarget debug elements through ILaunch
2. It was subject to SWT illegal thread access errors doe to launchRemoved called in non-UI thread. Use-case: starting a new debug session with previose terminated launch removed automatically.

This fix will rely on debug model sending DebugEvent.TERMINATE event for IMemoryBlockRetrieval when the retrieval is disposed (e.g. target is terminated). 

Additional minor fixes:
1. Use a helper runOnUIThread method to run immediately a UI runnable when the current thread is UI thread
2. Fix a potential race condition when debug context change event is broadcasted and the launch is already terminated. For this case handle the selection as an unsupported selection.

The change will require change in debug models to send  DebugEvent.TERMINATE event for IMemoryBlockRetrieval.

Comment 6 Teodor Madan 2010-01-19 09:04:54 EST

Created attachment 156503 [details]
Fix in CMemoryBlockRetrievalExtension to fire terminated event

Required by https://bugs.eclipse.org/bugs/attachment.cgi?id=156501&action=diff

Comment 7 Teodor Madan 2010-01-19 09:10:13 EST

DSF debug model should be updated to fire DebugEvent.TERMINATE on disposal of DsfMemoryBlockRetrieval.

Comment 8 Teodor Madan 2010-01-19 09:13:23 EST

Steps to reproduce:
1. Set a BP in releaseTabFolder
2. Start a debug session with CDI-MI session.
3. Open Memory Browser
4. Terminate debug session. Expecting the memory browser to switch to unsupported selection pane and for BP from releaseTabFolder to be hit

Repeat above steps for DSF-MI session.

Comment 9 John Cortell 2010-01-20 12:32:31 EST

Teo, are you implying that the original fix (which was adjusted and committed to HEAD) should be backed out in its entirety? Or is some part of your new fix dependent on some of the changes from the original one?

Comment 10 Teodor Madan 2010-01-20 12:38:28 EST

(In reply to comment #9)
> Teo, are you implying that the original fix (which was adjusted and committed
> to HEAD) should be backed out in its entirety? Or is some part of your new fix
> dependent on some of the changes from the original one?

The original fix is not sufficient. This is an on top of what has been committed to HEAD and depends on the original patch. 

Maybe to eliminate confusion would be better to create a new bugzilla entry?

Comment 11 John Cortell 2010-01-20 13:33:53 EST

(In reply to comment #10)
> (In reply to comment #9)
> > Teo, are you implying that the original fix (which was adjusted and committed
> > to HEAD) should be backed out in its entirety? Or is some part of your new fix
> > dependent on some of the changes from the original one?
> 
> The original fix is not sufficient. This is an on top of what has been
> committed to HEAD and depends on the original patch. 
> 
> Maybe to eliminate confusion would be better to create a new bugzilla entry?

OK. That's all I needed to know. I don't think we need a new bugzilla entry. I'll take a look at the patch.

Comment 12 John Cortell 2010-01-20 14:54:35 EST

Applied Teo's latest two patches to HEAD with minor adjustments.

Comment 13 John Cortell 2010-01-20 15:39:29 EST

Created attachment 156707 [details]
Get DSF to fire TERMINATE event for its memory retrieval object

Just as Teo updated the CDI implementation to fire a TERMINATE event for the memory retrieval object (so the memory views can clean up), I've updated the DSF implementation to do the same.

Comment 14 John Cortell 2010-01-20 15:41:02 EST

Pawel, please review my attached fix for DSF. Without it, the Memory view and Memory Browser views leak with every debug session.

Comment 15 John Cortell 2010-01-21 14:44:59 EST

Created attachment 156851 [details]
adds logging to platform debug plugin to show problem

I've attached the logging code that I used to reproduce and validate the problem. It's a lot easier than using breakpoints. The patch is for platform debug, so you need those two plugins in your workspace (checked out from cvs) to use this patch.

Comment 16 Pawel Piech 2010-01-22 19:16:48 EST

(In reply to comment #14)
> Pawel, please review my attached fix for DSF. Without it, the Memory view and
> Memory Browser views leak with every debug session.

Thanks John.  I committed the fix with a minor adjustment.

Comment 17 Pawel Piech 2010-01-25 11:19:48 EST

John Cortell wrote:
> Pawel,
>
> For my own education, why launch a thread to fire the event? I don't see the potential for a deadlock. The only lock that's obtained in firing the event is the platform debug's event queue, which given how it's used, seems deadlock proof.
>
> John

You're right John.  I was looking at example of DsfLaunch.fireTerminate() which uses LaunchNotifier(), and I didn't realize that DebugPlugin.fireEventSet() already uses a job.  I removed the redundant job and committed.