Bug 55905 - [EditorMgmt] WorkbenchPage#lastActiveEditor keeps reference on editor after closing all editors
Summary: [EditorMgmt] WorkbenchPage#lastActiveEditor keeps reference on editor after c...
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.0   Edit
Hardware: PC Windows XP
: P1 normal (vote)
Target Milestone: 3.0 M8   Edit
Assignee: Chris McLaren CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-03-24 10:41 EST by Christof Marti CLA
Modified: 2004-03-26 01:10 EST (History)
2 users (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Christof Marti CLA 2004-03-24 10:41:36 EST 
I200403240800

After opening/using ~6 editors and then closing all, I found
WorkbenchPage#lastActiveEditor referencing an editor.
Comment 1 Chris McLaren CLA 2004-03-24 15:29:19 EST 
requires File -> Close All to reproduce.
Comment 2 Douglas Pollock CLA 2004-03-25 10:26:26 EST 
This bug has been added to list to stop ship for M8.
Comment 3 Chris McLaren CLA 2004-03-25 16:09:35 EST 
fixed in head (no really :)

some very ancient code in WorkbenchPage has methods closeEditor and
closeEditors, which it seems over time have diverged more than you might expect.
i have rewritten these methods to use a common code path. 

NOW: open editors A and B. close individually. no leak. reopen both editors.
close all. no leak. (using yourkit 2.0.1 to check for roots of
CompilationUnitEditor)
Comment 4 Chris McLaren CLA 2004-03-25 16:34:44 EST 
kim: please review this change for me. (the code was pretty ugly in there
previously). the change is local to WorkbenchPage. thanks.
Comment 5 Kim Horne CLA 2004-03-25 16:53:53 EST 
in closeEditors2(IEditorReference[]) you have the following:

editorParts[i] = editorReferences[i].getEditor(false);

later you call closeEditors2(IEditorPart[]).  In here you do not check for the
possiblity of a null editor part.  This will cause the certifyPart call to fail
and the entire list won't be cloesd.  If you open a workspace that has open
editors and try to close all immediately, you'll see it doesn't work.  It's for
this reason... when you getEditor(false) on an editor that is not yet visible
you'll get a null part.
Comment 6 Chris McLaren CLA 2004-03-25 17:41:01 EST 
good catch. please retry with new changes..
Comment 7 Chris McLaren CLA 2004-03-25 17:41:39 EST 
also, i retested leaks in both close and close all cases with latest change.
Comment 8 Douglas Pollock CLA 2004-03-26 00:03:20 EST 
Two test failures on I200403252030.  This seems to have gotten worse.  :(
Comment 9 Nick Edgar CLA 2004-03-26 00:13:52 EST 
I've backed out of the changes to WorkbenchPage.closeEditor* since they were
causing the NPE in bug 56287 and also a test failure in
IWorkbenchPageTests.closeEditorTests: bug 56284.

I've made a more surgical fix for the leak.  In WorkbenchPage.closeEditors, I
removed the else in:
			if (part == activePart) {
				deactivated = true;
				setActivePart(null);
			}
			else if (lastActiveEditor == part) {
				lastActiveEditor = null;
				actionSwitcher.updateTopEditor(null);
			}

This is in the midnight build.
Comment 10 Nick Edgar CLA 2004-03-26 01:10:48 EST 
Verified with YourKit 2.1 on Win2K that this fixes the leak for both plain text
editors and Java editors, using both Close and Close All.

Closing.