Community
Participate
Working Groups
Build ID: I20070621-1340 Steps To Reproduce: The jface class ColumnViewerEditor has a field "cell" of type ViewerCell. This is set and never unset which can cause problems as the only way to overwrite it is to have a user click on a cell, say (or otherwise open a viewer editor). I have 3 views showing model A in my RCP app. If a user clicks on a cell in view 1 then loads model B then this troublesome viewer cell will leek a reference to model B. I f they then click on a cell in editor 2 and reload model C then I also leek a reference to model B etc. I can only fit three models in memory at one time and sometimes I need to explicitly load three simultaneously so this is quite debilitating! This is clearly not a good state of affairs. Setting the input should reset this field, or it should be made more transient so that, say, it is explicitly nulled when either no cell editor is opened or the cell editor completes editting. There's a work-around for read-only views, by overriding ColumnViewer.triggerEditorActivationEvent to do nothing you can avoid the whole problem. However, I can't see any way around this problem for views with editors. Thanks for looking into this :)
(In reply to comment #0) > Build ID: I20070621-1340 > > Steps To Reproduce: > The jface class ColumnViewerEditor has a field "cell" of type ViewerCell. This > is set and never unset which can cause problems as the only way to overwrite it > is to have a user click on a cell, say (or otherwise open a viewer editor). > > I have 3 views showing model A in my RCP app. If a user clicks on a cell in > view 1 then loads model B then this troublesome viewer cell will leek a > reference to model B. I f they then click on a cell in editor 2 and reload > model C then I also leek a reference to model B etc. I can only fit three > models in memory at one time and sometimes I need to explicitly load three > simultaneously so this is quite debilitating! > Can you please provide a snippet to reproduce this? > This is clearly not a good state of affairs. Setting the input should reset > this field, or it should be made more transient so that, say, it is explicitly > nulled when either no cell editor is opened or the cell editor completes > editting. > Good point. Nulling it out make sense. > There's a work-around for read-only views, by overriding > ColumnViewer.triggerEditorActivationEvent Hm why's that needed. If your Viewer is not editable ColumnViewerEditor.handleEditorActivationEvent() is a no-op anyways not? > to do nothing you can avoid the whole problem. However, I can't see any way > around this problem for views with editors. Well if that's really the problem you can use reflection to null out the field in the ColumnViewerEditor maybe? > > Thanks for looking into this :) > We are caching ViewerCells in other part of the system too: e.g. ColumnViewer, SWTCellFocusManager
Created attachment 76293 [details] Clearing cell after editor deactivated
(In reply to comment #1) > We are caching ViewerCells in other part of the system too: e.g. ColumnViewer, > SWTCellFocusManager Yes, I frequently see such leaks in TreeViewers (last updated cell is leaked in ColumnViewer#cell, which finally leaks the client's model elements via ViewerCell#element). This makes it hard to find other leaks. Will you fix all these leaks with this bug, or do you already have another bug for the other leaks?
no I'll only address the Editor leak here. Need to put some thought into the other leak in ColumnViewer.
Thanks, filed bug 201280 for the ColumnViewer case.
+1. Has this been applied to 3.4 yet?
No. I wanted to do this in at once
Released to R3_3_maintenance >= 20070831
3.4 integration is done in bug #201906
fixed for 3.3.1
Reopened because introduced an NPE
Created attachment 77593 [details] Moving nulling at the end
+1
Ping-pong - closing again
Verified by code introspection in M20070905-1045
Created attachment 87873 [details] Still leaking when editor activation cancled Still snippet demonstrates that we still leak
Created attachment 87874 [details] fixing leak when activation cancled
The snippet shows that we have missed two cases :-(
This is a temporary leak, right? Is there a workaround that clients could use?
well they could access the cell using reflection and null it explicitly before refreshing/setting a new input (if they are the ones who control it).
McQ this might be a 3.2.2 addition
(In reply to comment #18) > The snippet shows that we have missed two cases :-( > Does that mean "... and there are likely to be more."? McQ.
setting to 3.3.2 and moving to Boris
(In reply to comment #22) > (In reply to comment #18) > > The snippet shows that we have missed two cases :-( > > > Does that mean "... and there are likely to be more."? > > McQ. > Not as far as I can see, the problem was that we forgot to clear the cell when we cancled the cell-editor activation. The patch fixes the 2 remaining possibilities for the "leak".
+1 from me for putting the patch "fixing leak when activation cancled" into 3.3.2. A better way to deal with this in 3.4 would be to return a boolean from ColumnViewerEditor.activateCellEditor: returns true if a cell editor was activated. Then, the field "cell" could be cleared in just one place, if activateCellEditor returns false, close to where a value is assigned to that field. This would also increase the chances that we won't introduce the leak again when further changes to activateCellEditor are made in the future. For 3.3.2, I believe Tom's patch is the appropriate fix since it minimizes the number of changed lines of code.
Adding McQ back in for PMC approval for this last change
Final patch released for build >20080128
Verified on Windows XP using M20080131-1440.
