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

Bug 288596

Summary: [Patch] CompareEditor should dispose previous input
Product: [Eclipse Project] Platform Reporter: Alex Panchenko <alex.panchenko>
Component: CompareAssignee: Platform-Compare-Inbox <platform-compare-inbox>
Status: RESOLVED DUPLICATE QA Contact: Tomasz Zarna <tomasz.zarna>
Severity: normal    
Priority: P3 CC: daniel_megert, stephan.herrmann
Version: 3.5   
Target Milestone: ---   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
CompareEditor-disconnectFromInput-patch none

Description Alex Panchenko CLA 2009-09-04 00:50:21 EDT 
Created attachment 146471 [details]
CompareEditor-disconnectFromInput-patch

Without this patch there are two issues:
- listeneres registered in ModelCompareEditorInput.contentsCreated() are not
unregistered, so there is a memory leak
- they are called during shutdown and NPE occurs in
SaveableCompareEditorInput.closeEditor(boolean):
IWorkbenchPage page = getPage();
if (page == null)
  return false;
>>> Display display = page.getWorkbenchWindow().getShell().getDisplay();
display.asyncExec(runnable);
since shell is null at that time.
Comment 1 Stephan Herrmann CLA 2010-12-07 15:36:26 EST 
Could this bug also be the cause for another symptom I frequently see 
these days (using 3.7M3):

After I did some java comparison from the svn synchronize view
and after closing the compare editor every save in a java editor
triggers recomputing diffs!

I could observe jobs like "Updating Comparison ..." and I think
"Finding differences ..." (quoted from memory. As the messages are
truncated in the progress indicator I've never seen the full wording).

Need I say that this is a performance problem?

FWIW I use subversive.
Comment 2 Tomasz Zarna CLA 2011-08-12 05:59:50 EDT 
This has been fixed with bug 297950. org.eclipse.compare.CompareEditorInput.handleDispose() is called, which is essentially what CompareEditorInput.dispose()[1] from Alex's patch did, when disposing Editor's widgets. I've verified this works fine for ModelCompareEditorInputs: it unregisters the listener and I haven't seen any signs of the NPE.

Given the above, the symptoms from comment 1 don't seem to be related to this particular bug.

If the solution from bug 297950, doesn't suit you, feel free to reopen. However, you will have to present an explanation of why this doesn't work for you.

[1] now removed

*** This bug has been marked as a duplicate of bug 297950 ***
Comment 3 Stephan Herrmann CLA 2011-08-12 13:00:39 EDT 
(In reply to comment #2)
> Given the above, the symptoms from comment 1 don't seem to be related to this
> particular bug.
> 
> If the solution from bug 297950, doesn't suit you, feel free to reopen.
> However, you will have to present an explanation of why this doesn't work for
> you.

I agree that the original report is resolved. Unfortunately the issue I
keep observing is still relevant even in 3.7 release and beyond.

I can't say *what* is leaking, but frequently "updating diff" and also
NPE from the compare editor occur long after I closed all compare editors.

If it's not bug 297950, are there other leak related bugs against the
compare framework?

Or should we assume such symptoms to be caused by some concurrency issues
(I read that a general overhaul is planned in that area?).
Comment 4 Tomasz Zarna CLA 2011-08-16 04:37:52 EDT 
(In reply to comment #3)
> I agree that the original report is resolved. Unfortunately the issue I
> keep observing is still relevant even in 3.7 release and beyond.
> 
> I can't say *what* is leaking, but frequently "updating diff" and also
> NPE from the compare editor occur long after I closed all compare editors.

Stephan any chance for a set of reproducible steps? They would be extremely helpful.
 
> If it's not bug 297950, are there other leak related bugs against the
> compare framework?

One I've been working on recently is bug 353692, but it doesn't seem to be related even though it's about a leak in Compare Editor. If you think we're missing a bug for your case feel free to file one.

> Or should we assume such symptoms to be caused by some concurrency issues
> (I read that a general overhaul is planned in that area?).

It's bug 304693.