Skip to content

Resolve "SWT Resource was not properly disposed" for "Statistics" #297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ruspl-afed wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Resolve "SWT Resource was not properly disposed" for "Statistics" #297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions .../tracecompass/internal/tmf/ui/viewers/piecharts/PieChartViewerStateNoContentSelected.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2015 Ericsson
* Copyright (c) 2015, 2025 Ericsson and others
*
* All rights reserved. This program and the accompanying materials are
* made available under the terms of the Eclipse Public License 2.0 which
Expand All @@ -10,6 +10,7 @@
*
* Contributors:
* Alexis Cabana-Loriaux - Initial API and implementation
* Alexander Fedorov (ArSysOp) - fix "SWT Resource was not properly disposed"
*
*******************************************************************************/

Expand Down Expand Up @@ -43,13 +44,7 @@ public PieChartViewerStateNoContentSelected(final TmfPieChartViewer context) {
synchronized (context) {
if (!context.isDisposed()) {
// Have to get rid of the time-range PieChart
if (context.getTimeRangePC() != null) {
if (!context.getTimeRangePC().isDisposed()) {
context.getTimeRangePC().dispose();
}
context.setTimeRangePC(null);
}

context.disposeTimeRangePC();
context.updateGlobalPieChart();
// update the global chart so it takes all the place
context.getGlobalPC().getLegend().setPosition(SWT.RIGHT);
Expand Down
35 changes: 26 additions & 9 deletions ....ui/src/org/eclipse/tracecompass/internal/tmf/ui/viewers/piecharts/TmfPieChartViewer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2015, 2020 Ericsson
* Copyright (c) 2015, 2025 Ericsson and others
*
* All rights reserved. This program and the accompanying materials are
* made available under the terms of the Eclipse Public License 2.0 which
Expand All @@ -10,6 +10,7 @@
*
* Contributors:
* Alexis Cabana-Loriaux - Initial API and implementation
* Alexander Fedorov (ArSysOp) - fix "SWT Resource was not properly disposed"
*
*******************************************************************************/

Expand Down Expand Up @@ -442,17 +443,11 @@ public synchronized void reinitializeCharts() {
if (isDisposed()) {
return;
}

if (getGlobalPC() != null && !getGlobalPC().isDisposed()) {
getGlobalPC().dispose();
}
disposeGlobalPC();
fGlobalPC = new TmfPieChart(this, SWT.NONE);
getGlobalPC().getTitle().setText(fGlobalPCname);
getGlobalPC().getAxisSet().getXAxis(0).getTitle().setText(""); //Hide the title over the legend //$NON-NLS-1$
if (getTimeRangePC() != null && !getTimeRangePC().isDisposed()) {
getTimeRangePC().dispose();
fTimeRangePC = null;
}
disposeTimeRangePC();
layout();
setCurrentState(new PieChartViewerStateNoContentSelected(this));
}
Expand Down Expand Up @@ -582,4 +577,26 @@ public void select(String id) {
pieChart.redraw();
}
}

@Override
public void dispose() {
Copy link
Contributor

@PatrickTasse PatrickTasse Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually not the intended SWT design to require dispose() to also dispose the children.

The method Widget.dispose() is not a cleanup method, it's a trigger for the application to remove a widget (and all its children). Internally it's the Widget.release() method that gets called recursively, automatically.

If a widget has cleanup to do, it should be done in a DisposeListener. This will always be called when the widget is removed, while it's not guaranteed that dispose() will ever be called, it might only be called on one of the widget's parents.

The real source of the problem is the implementation in org.eclipse.swtchart.Chart. It holds some system resources (e.g. Font in the AxisTitle) that are only released if Chart.dispose() is called, which is not normally called. Because of this implementation, we have to make sure to call Chart.dispose() on our child pie charts.

However, to avoid repeating the mistake, we should do this in a DisposeListener added in the constructor. There is already one, but it should be added to 'this', not to the parent. The disposal of the two pie charts can be moved to that listener. Then this overload of dispose() is no longer needed.

Copy link
Contributor

@PatrickTasse PatrickTasse Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a clarification, it works currently since TmfPieChartViewer is only a child of TmfStatisticsViewer, but I want to protect in case it is ever put as a child of any other Composite that might not call dispose().

Copy link
Contributor Author

@ruspl-afed ruspl-afed Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very grateful for your comments @PatrickTasse
Please give me some time to revisit SWT API and find better formulation for this change.

disposeGlobalPC();
disposeTimeRangePC();
super.dispose();
}

void disposeGlobalPC() {
if (fGlobalPC != null && !fGlobalPC.isDisposed()) {
fGlobalPC.dispose();
fGlobalPC = null;
}
}

void disposeTimeRangePC() {
if (fTimeRangePC != null && !fTimeRangePC.isDisposed()) {
fTimeRangePC.dispose();
fTimeRangePC = null;
}
}

}
5 changes: 3 additions & 2 deletions .../src/org/eclipse/tracecompass/internal/tmf/ui/viewers/statistics/TmfStatisticsViewer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2012, 2020 Ericsson
* Copyright (c) 2012, 2025 Ericsson and others
*
* All rights reserved. This program and the accompanying materials are
* made available under the terms of the Eclipse Public License 2.0 which
Expand All @@ -13,6 +13,7 @@
* Alexandre Montplaisir - Port to ITmfStatistics provider
* Patrick Tasse - Support selection range
* Bernd Hufmann - Fix range selection updates
* Alexander Fedorov (ArSysOp) - fix "SWT Resource was not properly disposed"
*******************************************************************************/

package org.eclipse.tracecompass.internal.tmf.ui.viewers.statistics;
Expand Down Expand Up @@ -186,7 +187,7 @@ private void internalDispose() {

// Clean the model for this viewer
TmfStatisticsTreeManager.removeStatTreeRoot(getTreeID());
fPieChartViewer.reinitializeCharts();
fPieChartViewer.dispose();
}

// ------------------------------------------------------------------------
Expand Down
Loading