RT crashes in METM when a lot of Editors are opened (Windows)

[heckflosse]

When I open a lot of editors in METM, RT crashes. On my machine, opening 10 editors works fine. Opening the 11th editor, RT crashes. My machine has 32 GB RAM. The crash is independent on the size of the files I open. Opening 11 12MP files crashes at 11th file as well as opening 11 36MP files.

Example console output at crash:

$ Release/rawtherapee
_create_dc_and_bitmap: Falscher Parameter.

(rawtherapee.exe:5784): Gtk-WARNING **: infinite surface size not supported
_create_dc_and_bitmap: Falscher Parameter.

(rawtherapee.exe:5784): Gtk-WARNING **: infinite surface size not supported
_create_dc_and_bitmap: Falscher Parameter.

(rawtherapee.exe:5784): Gtk-WARNING **: infinite surface size not supported
_create_dc_and_bitmap: Falscher Parameter.

(rawtherapee.exe:5784): Gtk-WARNING **: infinite surface size not supported

(rawtherapee.exe:5784): Gtk-WARNING **: infinite surface size not supported
_create_dc_and_bitmap: Falscher Parameter.

(rawtherapee.exe:5784): Gtk-WARNING **: infinite surface size not supported

(rawtherapee.exe:5784): Gtk-WARNING **: infinite surface size not supported
_create_dc_and_bitmap: Falscher Parameter.

[Beep6581]

Some relevant warnings here: https://developer.gnome.org/gdk3/stable/gdk3-Cairo-Interaction.html#gdk-cairo-create https://github.com/GNOME/gtk/blob/master/gtk/gtkiconhelper.c#L297 https://mail.gnome.org/archives/gtk-list/2013-March/msg00034.html

[gaaned92]

W10 16GB : 9 files ok, 10th file it hangs at end of the decoding ( blue bar on upper right). Edit windows never opens. Quite a lot of Gtk-WARNING **: infinite surface size not supported on console.

All files were already processed without problem. total memory used 6,5GB.

[Desmis]

I think, it is exactly the same bug as in #3920 Except for my test it is in "Multiple Editors Tabs in Own Window Mode"

In other places, I have found that the number of files that could be open depends of branch; 1) 6 files open with "dev" 2) 7 files open with "dev" if I disabled "gradient" (suppress gradient in cmakelist, toolpanelcoord, batchtoolpanelcoord...) 3) 5 files open with "locallab_dev" 4) 4 files open with "localrgb"

No problem with amount of memory, - crash append with very small files and 1.6Mb consume on 8Mb., but I think it is due to usage of Cairo function in GTK3 as "Circle, Line, etc." With Gtk2 no problem, I can open more than 18 files... :)

[heckflosse]

It's a leak somewhere. I viewed at gdi-handles in taskmanager I did the following test: Opened a folder with 246 files => gdi handles 3397 Opened a folder with 11 files => gdi handles 3419 Opened the folder with 246 files again => gdi handles 3911 Opened the folder with 11 files again => gdi handles 3933 Opened the folder with 246 files again => gdi handles 4452

=> each thumb leaks exactly two gdi handles

[heckflosse]

I did another test. Opened amsterdam.pef in METM closed editor Opened amsterdam.pef in METM closed editor ... taskmanager shows that we leak 155 gdi-handles each time.

From here:

–Limits–

There is a limit of 65,536 (64k) GDI handles per session.
Note though that, particularly on 32-bit (x86) systems, the effective maximum is usually lower than this, due to memory limitations.
For example, you generally won’t fit anywhere near 65,536 large bitmaps on a 32-bit system, no matter how many handles you’re allowed to have.

That 65,536 handles needs to accommodate all processes in the session (your app doesn’t get to use them all).
In fact, to prevent a single process from bringing the entire system to its knees (and making the OS unable even to draw a dialog saying “I…CAN’T…DRAW…ANYMORE”),
there is a per-process limit, which by default is 10,000 GDI handles.

According to the above tests with file browser and METM we reach that limit of 10000 GDI handles easily...

[heckflosse]

Some findings:

1. each standard adjuster (like this one) needs 4 GDI handles
2. standard adjusters do not leak the GDI handles
3. Checkboxes also do not leak GDI handles

Independent on the current leaks we have to limit the number of opened editors in METM (at least on Windows) to avoid the 10000 GDI handles limit

[heckflosse]

To verify that the crash is caused by reaching the gdi handle limit of windows,

HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Windows\GDIProcessHandleQuota

I increased the value from 10000 to 20000. With that setting I can open more editors.

Edit: A restart of the pc may be needed after changing the setting

[heckflosse]

I used GDIView to check for GDI handle leaks. It seems we leak handles for thumbnails (in file browser) and handles for icons (in editor)...

[agriggio]

thanks Ingo for all your investigation! I wish I could offer some help but if Windows-specific all I can do is cheerleading... :-/ so, "go @heckflosse go!" ;-)

[heckflosse]

@agriggio Alberto, on Linux it leaks also. Though address sanitizer does not show leaks, system monitor is clearly showing that there are leaks. For example: open 5 copies of amsterdam.pef in METM, close them, open again and so on you can clearly see increased memory usage of rawtherapee. On my linux machine it's about 100 MB leak per 5 METM editors

[agriggio]

@heckflosse ok, that I can try to debug then... (not before next week though)

[heckflosse]

This patch fixes two possible leaks detected by valgrind (though these are not the culprits). Each of these possible leaks occurred about 50 times per editor.

diff --git a/rtgui/coordinateadjuster.cc b/rtgui/coordinateadjuster.cc
index ac8e5ea4..c91ced05 100644
--- a/rtgui/coordinateadjuster.cc
+++ b/rtgui/coordinateadjuster.cc
@@ -42,13 +42,9 @@ void Axis::setValues(Glib::ustring label, unsigned int decimal, double increment

 CoordinateAdjuster::AxisAdjuster::AxisAdjuster(CoordinateAdjuster *parent, const Axis *axis, char index) : idx(index), parent(parent), rangeLowerBound(0.f), rangeUpperBound(0.f)
 {
-    label = Gtk::manage( new Gtk::Label(axis->label) );
-    spinButton = Gtk::manage( new Gtk::SpinButton() );

-    label = Gtk::manage (new Gtk::Label(axis->label));
-    //label->set_alignment(Gtk::ALIGN_MIDDLE, Gtk::ALIGN_MIDDLE);
-
-    spinButton = Gtk::manage (new Gtk::SpinButton());
+    label = Gtk::manage(new Gtk::Label(axis->label));
+    spinButton = Gtk::manage(new Gtk::SpinButton());
     spinButton->set_name("AxisAdjuster");
     spinButton->set_digits(axis->decimal);
     spinButton->set_increments(axis->increment, axis->pageIncrement);

[heckflosse]

I created a new branch valgrind_fixes to cover all the issues reported by valgrind. Currently only a few are fixed.

[Desmis]

@heckflosse tests in "Multiple Editors Tabs in Own Window Mode"

I have tested with Gdiview an old GTK2 "locallab" version (there are some differences, eg Locallab in scrollbar instead of "exposure panel", but minor differences, except gtk2 ==> gtk3

With 3 files open in the editor and GTK2 Brush = 1, Bimap = 6, Font = 3, Region=2, DC=10, All GDI = 280

With the same files and GTK3 Brush=1, Bitmap = 2219, Font =0, Region = 0, DC = 2244, All GDI = 7113

jacques

[heckflosse]

@Desmis Thanks for the comparison. That clearly shows there is something wrong with the Editor in gtk3 builds.

@Hombre57 Do you have an idea about this? Maybe it's something similar to #3908 ?

[heckflosse]

@Hombre57 I tested some old gtk3 commits. d26eaa1 already leaks. Maybe even elder ones leak...

[heckflosse]

@Hombre57 Maybe a hint to find the culprit: When I make a build without wavelets tab, the number of leaked gdi handles is reduced by a large amount. That absolutely does not mean that wavelet is the culprit! But maybe this way we can find the real culprit:

number of gdi handles     before opening editor   after opening editor  after closing editor
with wavelets                       2401                   3178                   2574
without wavelets                    1948                   2581                   1969

[Desmis]

@heckflosse

I think the number of GDI is link to number of events. Wavelet have a big number of events... so.

I think it is the same reasoning, when I open "locallab" instead of "dev", and more with "localrgb" But all that does not give the bad culprit :)

[Hombre57]

I don't think that any tool in particular is the culprit, but rather a GUI class that is broken. Despite all my researches, I haven't found a clear description on how to create custom widgets in gtk3, i.e. the instantiation process. I hope that what I've done is good, but I wouldn't bet on it.

[heckflosse]

@Desmis

Jacques, I don't think that the events are related to this issue. I tried by defining PIXELSHIFTDEV for rt gui, which adds a lot of adjusters and nothing changed :(

[Desmis]

@heckflosse

I think, but it's obvious, I am not sure. that it depends on events, but those that uses some Cairos function, as "arc (". You can found them in

• spot (graduated filter, locallab, labrgb)
• curves....
• lockable color picker *...

[heckflosse]

This patch reduces the number of leaked gdi handles per editor to 23 (was about 165 before patch)

diff --git a/rtgui/guiutils.cc b/rtgui/guiutils.cc
index 4d9dd3c5..fa4d23b2 100644
--- a/rtgui/guiutils.cc
+++ b/rtgui/guiutils.cc
@@ -849,13 +849,15 @@ bool MyExpander::get_expanded()
     return expBox ? expBox->get_visible() : false;
 }

-void MyExpander::add  (Gtk::Container& widget)
+void MyExpander::add  (Gtk::Container& widget, bool setChild)
 {
-    child = &widget;
+    if(setChild) {
+        child = &widget;
+    }
     expBox = Gtk::manage (new ExpanderBox (child));
-    expBox->add (*child);
+    expBox->add (widget);
     pack_start(*expBox, Gtk::PACK_SHRINK, 0);
-    child->show();
+    widget.show();
     expBox->hideBox();
 }

diff --git a/rtgui/guiutils.h b/rtgui/guiutils.h
index 08502757..4b015c21 100644
--- a/rtgui/guiutils.h
+++ b/rtgui/guiutils.h
@@ -276,7 +276,7 @@ public:

     /// Add a Gtk::Container for the content of the expander
     /// Warning: do not manually Show/Hide the widget, because this parameter is handled by the click on the Expander's title
-    void add  (Gtk::Container& widget);
+    void add  (Gtk::Container& widget, bool setChild = true);

     void updateVScrollbars(bool hide);
 };
diff --git a/rtgui/retinex.cc b/rtgui/retinex.cc
index 6f05f3f7..75f09948 100644
--- a/rtgui/retinex.cc
+++ b/rtgui/retinex.cc
@@ -194,7 +194,7 @@ Retinex::Retinex () : FoldableToolPanel (this, "retinex", M ("TP_RETINEX_LABEL")

-    expsettings = new MyExpander (false, M ("TP_RETINEX_SETTINGS"));
+    expsettings = Gtk::manage(new MyExpander(false, M ("TP_RETINEX_SETTINGS")));
     setExpandAlignProperties (expsettings, true, false, Gtk::ALIGN_FILL, Gtk::ALIGN_START);
     expsettings->signal_button_release_event().connect_notify ( sigc::bind ( sigc::mem_fun (this, &Retinex::foldAllButMe), expsettings) );

@@ -474,7 +474,7 @@ Retinex::Retinex () : FoldableToolPanel (this, "retinex", M ("TP_RETINEX_LABEL")

     //--------------------------

-    expsettings->add (*settingsGrid);
+    expsettings->add (*settingsGrid, false);
     expsettings->setLevel (2);
     pack_start (*expsettings);

diff --git a/rtgui/wavelet.cc b/rtgui/wavelet.cc
index 9a692b78..1fea0cd5 100644
--- a/rtgui/wavelet.cc
+++ b/rtgui/wavelet.cc
@@ -132,15 +132,15 @@ Wavelet::Wavelet() :
     wavLabels(Gtk::manage(new Gtk::Label("---", Gtk::ALIGN_CENTER))),
     labmC(Gtk::manage(new Gtk::Label(M("TP_WAVELET_CTYPE") + ":"))),
     labmNP(Gtk::manage(new Gtk::Label(M("TP_WAVELET_NPTYPE") + ":"))),
-    expchroma(new MyExpander(true, M("TP_WAVELET_LEVCH"))),
-    expcontrast(new MyExpander(true, M("TP_WAVELET_LEVF"))),
-    expedge(new MyExpander(true, M("TP_WAVELET_EDGE"))),
-    expfinal(new MyExpander(true, M("TP_WAVELET_FINAL"))),
-    expgamut(new MyExpander(false, M("TP_WAVELET_CONTR"))),
-    expnoise(new MyExpander(true, M("TP_WAVELET_NOISE"))),
-    expresid(new MyExpander(true, M("TP_WAVELET_RESID"))),
-    expsettings(new MyExpander(false, M("TP_WAVELET_SETTINGS"))),
-    exptoning(new MyExpander(true, M("TP_WAVELET_TON"))),
+    expchroma(Gtk::manage(new MyExpander(true, M("TP_WAVELET_LEVCH")))),
+    expcontrast(Gtk::manage(new MyExpander(true, M("TP_WAVELET_LEVF")))),
+    expedge(Gtk::manage(new MyExpander(true, M("TP_WAVELET_EDGE")))),
+    expfinal(Gtk::manage(new MyExpander(true, M("TP_WAVELET_FINAL")))),
+    expgamut(Gtk::manage(new MyExpander(false, M("TP_WAVELET_CONTR")))),
+    expnoise(Gtk::manage(new MyExpander(true, M("TP_WAVELET_NOISE")))),
+    expresid(Gtk::manage(new MyExpander(true, M("TP_WAVELET_RESID")))),
+    expsettings(Gtk::manage(new MyExpander(false, M("TP_WAVELET_SETTINGS")))),
+    exptoning(Gtk::manage(new MyExpander(true, M("TP_WAVELET_TON")))),
     neutrHBox(Gtk::manage(new Gtk::HBox()))
 {
     CurveListener::setMulti(true);
@@ -838,39 +838,39 @@ Wavelet::Wavelet() :

 //-----------------------------

-    expsettings->add(*settingsBox);
+    expsettings->add(*settingsBox, false);
     expsettings->setLevel(2);
     pack_start (*expsettings);

-    expcontrast->add(*levBox);
+    expcontrast->add(*levBox, false);
     expcontrast->setLevel(2);
     pack_start (*expcontrast);

-    expchroma->add(*chBox);
+    expchroma->add(*chBox, false);
     expchroma->setLevel(2);
     pack_start (*expchroma);

-    exptoning->add(*tonBox);
+    exptoning->add(*tonBox, false);
     exptoning->setLevel(2);
     pack_start (*exptoning);

-    expnoise->add(*noiseBox);
+    expnoise->add(*noiseBox, false);
     expnoise->setLevel(2);
     pack_start (*expnoise);

-    expedge->add(*edgBox);
+    expedge->add(*edgBox, false);
     expedge->setLevel(2);
     pack_start (*expedge);

-    expgamut->add(*conBox);
+    expgamut->add(*conBox, false);
     expgamut->setLevel(2);
     pack_start (*expgamut);

-    expresid->add(*resBox);
+    expresid->add(*resBox, false);
     expresid->setLevel(2);
     pack_start(*expresid);

-    expfinal->add(*finalBox);
+    expfinal->add(*finalBox, false);
     expfinal->setLevel(2);
     pack_start(*expfinal);
 }
@@ -887,6 +887,7 @@ Wavelet::~Wavelet ()
     delete curveEditorG;
     delete opacityCurveEditorW;
     delete opacityCurveEditorWL;
+
 }

 void Wavelet::wavChanged (double nlevel)

@Desmis Jacques, please test that wavelet and retinex still work well (especially the expanders) @Hombre57 please review the patch

The patch does not solve this issue. It does not reduce the number of gdi handles per editor.

[Desmis]

@heckflosse

Ingo I just tested the patch : no problem, all seems to work well :) Jacques

[heckflosse]

@Desmis

Jacques, thanks for testing. I will push some more fixes tomorrow

[Hombre57]

@heckflosse : My I suggest this patch ? :

diff --git a/rtgui/guiutils.cc b/rtgui/guiutils.cc
index 4d9dd3c..9e40358 100644
--- a/rtgui/guiutils.cc
+++ b/rtgui/guiutils.cc
@@ -513,7 +513,7 @@
 }
 */

-ExpanderBox::ExpanderBox( Gtk::Container *p): pC(p)
+ExpanderBox::ExpanderBox()
 {
     set_name ("ExpanderBox");
 //GTK318
@@ -561,7 +561,7 @@

 MyExpander::MyExpander(bool useEnabled, Gtk::Widget* titleWidget) :
     enabled(false), inconsistent(false), flushEvent(false), expBox(nullptr),
-    child(nullptr), headerWidget(nullptr), statusImage(nullptr),
+    headerWidget(nullptr), statusImage(nullptr),
     label(nullptr), useEnabled(useEnabled)
 {
     set_spacing(0);
@@ -613,7 +613,7 @@

 MyExpander::MyExpander(bool useEnabled, Glib::ustring titleLabel) :
     enabled(false), inconsistent(false), flushEvent(false), expBox(nullptr),
-    child(nullptr), headerWidget(nullptr), statusImage(nullptr),
+    headerWidget(nullptr), statusImage(nullptr),
     label(nullptr), useEnabled(useEnabled)
 {
     set_spacing(0);
@@ -851,11 +851,10 @@

 void MyExpander::add  (Gtk::Container& widget)
 {
-    child = &widget;
-    expBox = Gtk::manage (new ExpanderBox (child));
-    expBox->add (*child);
+    expBox = Gtk::manage (new ExpanderBox ());
+    expBox->add (widget);
     pack_start(*expBox, Gtk::PACK_SHRINK, 0);
-    child->show();
+    widget.show();
     expBox->hideBox();
 }

@@ -887,11 +886,6 @@
     }

     return false;
-}
-
-Gtk::Container* MyExpander::getChild()
-{
-    return child;
 }

 // used to connect a function to the enabled_toggled signal
diff --git a/rtgui/guiutils.h b/rtgui/guiutils.h
index 0850275..b7a1e16 100644
--- a/rtgui/guiutils.h
+++ b/rtgui/guiutils.h
@@ -144,15 +144,8 @@
  */
 class ExpanderBox: public Gtk::EventBox
 {
-private:
-    Gtk::Container *pC;
-
 public:
-    explicit ExpanderBox( Gtk::Container *p);
-    ~ExpanderBox( )
-    {
-        delete pC;
-    }
+    explicit ExpanderBox();

     void setLevel(int level);

@@ -206,7 +199,6 @@
     void updateStyle();

 protected:
-    Gtk::Container* child;      /// Gtk::Contained to display below the expander's title
     Gtk::Widget* headerWidget;  /// Widget to display in the header, next to the arrow image ; can be NULL if the "string" version of the ctor has been used
     Gtk::Image* statusImage;    /// Image to display the opened/closed status (if useEnabled is false) of the enabled/disabled status (if useEnabled is true)
     Gtk::Label* label;          /// Text to display in the header, next to the arrow image ; can be NULL if the "widget" version of the ctor has been used
@@ -265,9 +257,6 @@
         return headerWidget ? headerWidget : label;
     }

-    /// Get the widget shown/hidden by the expander
-    Gtk::Container* getChild();
-
     /// Set the collapsed/expanded state of the expander
     void set_expanded( bool expanded );

@@ -276,6 +265,7 @@

     /// Add a Gtk::Container for the content of the expander
     /// Warning: do not manually Show/Hide the widget, because this parameter is handled by the click on the Expander's title
+    /// 'widget' has to be created with Gtk::manage
     void add  (Gtk::Container& widget);

     void updateVScrollbars(bool hide);
diff --git a/rtgui/retinex.cc b/rtgui/retinex.cc
index 6f05f3f..0423f65 100644
--- a/rtgui/retinex.cc
+++ b/rtgui/retinex.cc
@@ -194,7 +194,7 @@

-    expsettings = new MyExpander (false, M ("TP_RETINEX_SETTINGS"));
+    expsettings = Gtk::manage(new MyExpander(false, M ("TP_RETINEX_SETTINGS")));
     setExpandAlignProperties (expsettings, true, false, Gtk::ALIGN_FILL, Gtk::ALIGN_START);
     expsettings->signal_button_release_event().connect_notify ( sigc::bind ( sigc::mem_fun (this, &Retinex::foldAllButMe), expsettings) );

diff --git a/rtgui/wavelet.cc b/rtgui/wavelet.cc
index 9a692b7..32bb470 100644
--- a/rtgui/wavelet.cc
+++ b/rtgui/wavelet.cc
@@ -132,15 +132,15 @@
     wavLabels(Gtk::manage(new Gtk::Label("---", Gtk::ALIGN_CENTER))),
     labmC(Gtk::manage(new Gtk::Label(M("TP_WAVELET_CTYPE") + ":"))),
     labmNP(Gtk::manage(new Gtk::Label(M("TP_WAVELET_NPTYPE") + ":"))),
-    expchroma(new MyExpander(true, M("TP_WAVELET_LEVCH"))),
-    expcontrast(new MyExpander(true, M("TP_WAVELET_LEVF"))),
-    expedge(new MyExpander(true, M("TP_WAVELET_EDGE"))),
-    expfinal(new MyExpander(true, M("TP_WAVELET_FINAL"))),
-    expgamut(new MyExpander(false, M("TP_WAVELET_CONTR"))),
-    expnoise(new MyExpander(true, M("TP_WAVELET_NOISE"))),
-    expresid(new MyExpander(true, M("TP_WAVELET_RESID"))),
-    expsettings(new MyExpander(false, M("TP_WAVELET_SETTINGS"))),
-    exptoning(new MyExpander(true, M("TP_WAVELET_TON"))),
+    expchroma(Gtk::manage(new MyExpander(true, M("TP_WAVELET_LEVCH")))),
+    expcontrast(Gtk::manage(new MyExpander(true, M("TP_WAVELET_LEVF")))),
+    expedge(Gtk::manage(new MyExpander(true, M("TP_WAVELET_EDGE")))),
+    expfinal(Gtk::manage(new MyExpander(true, M("TP_WAVELET_FINAL")))),
+    expgamut(Gtk::manage(new MyExpander(false, M("TP_WAVELET_CONTR")))),
+    expnoise(Gtk::manage(new MyExpander(true, M("TP_WAVELET_NOISE")))),
+    expresid(Gtk::manage(new MyExpander(true, M("TP_WAVELET_RESID")))),
+    expsettings(Gtk::manage(new MyExpander(false, M("TP_WAVELET_SETTINGS")))),
+    exptoning(Gtk::manage(new MyExpander(true, M("TP_WAVELET_TON")))),
     neutrHBox(Gtk::manage(new Gtk::HBox()))
 {
     CurveListener::setMulti(true);

I've removed the child property since it's not used (anymore) anyway, but kept your changes about Gtk::manage. I don't know if there's a way to enforce that only Gtk::managed Container have to be used, but the code should be correct as is (to be confirmed).

[heckflosse]

@Hombre57

Isn't the child property needed to delete the object when using expanders created by toolpanelcoordinator?

[Hombre57]

Let me see...

[heckflosse]

@Hombre57 I tested your patch. It leaks more gdi handles than my patch

[Hombre57]

Correct, my patch sucks ! Maybe I'm just not into it anymore... Yours is fine.

[heckflosse]

@Hombre57 Though you are right about removing Gtk::Container* getChild()

I will do that.

[Hombre57]

@heckflosse Do you think that this patch make sense ?

diff --git a/rtgui/adjuster.cc b/rtgui/adjuster.cc
index a4f5749..69cdb20 100644
--- a/rtgui/adjuster.cc
+++ b/rtgui/adjuster.cc
@@ -184,7 +184,7 @@
 void Adjuster::delAutoButton ()
 {
     if (automatic) {
-        removeIfThere(grid, automatic);
+        removeIfThere(grid, automatic, false);
         delete automatic;
         automatic = nullptr;
     }
diff --git a/rtgui/crop.cc b/rtgui/crop.cc
index 4b9e22f..f4e404e 100644
--- a/rtgui/crop.cc
+++ b/rtgui/crop.cc
@@ -1313,5 +1313,5 @@
     ratio->append (M("GENERAL_UNCHANGED"));
     orientation->append (M("GENERAL_UNCHANGED"));
     guide->append (M("GENERAL_UNCHANGED"));
-    removeIfThere (this, ppibox);
+    removeIfThere (this, ppibox, false);
 }
diff --git a/rtgui/filecatalog.cc b/rtgui/filecatalog.cc
index 0d10ba1..ba83b79 100644
--- a/rtgui/filecatalog.cc
+++ b/rtgui/filecatalog.cc
@@ -78,6 +78,7 @@
     emptyT->signal_pressed().connect (sigc::mem_fun(*this, &FileCatalog::emptyTrash));
     trashButtonBox->pack_start (*emptyT, Gtk::PACK_SHRINK, 4);
     emptyT->show ();
+    trashButtonBox->reference (); // will prevent deletion with removeIfThere
     trashButtonBox->show ();

     //initialize hbToolBar1
@@ -453,6 +454,9 @@
         delete igEdited[i];
         delete iRecentlySaved[i];
         delete igRecentlySaved[i];
+    }
+    if (trashButtonBox) {
+        trashButtonBox->unreference();
     }

     delete iFilterClear;
@@ -1540,7 +1544,7 @@
         _refreshProgressBar();

         //rearrange panels according to the selected filter
-        removeIfThere (hBox, trashButtonBox);
+        removeIfThere (hBox, trashButtonBox, false);

         if (bTrash->get_active ()) {
             hBox->pack_start (*trashButtonBox, Gtk::PACK_SHRINK, 4);
diff --git a/rtgui/icmpanel.cc b/rtgui/icmpanel.cc
index e80b202..4ad7d00 100644
--- a/rtgui/icmpanel.cc
+++ b/rtgui/icmpanel.cc
@@ -1053,7 +1053,7 @@
     iunchanged->set_group (opts);
     iVBox->pack_start (*iunchanged, Gtk::PACK_SHRINK, 4);
     iVBox->reorder_child (*iunchanged, 5);
-    removeIfThere (this, saveRef);
+    removeIfThere (this, saveRef, false);
     onames->append (M("GENERAL_UNCHANGED"));
     ointent->addEntry("unchanged-22.png", M("GENERAL_UNCHANGED"));
     ointent->show();
diff --git a/rtgui/lensgeom.cc b/rtgui/lensgeom.cc
index 7cae75d..6f8b7fc 100644
--- a/rtgui/lensgeom.cc
+++ b/rtgui/lensgeom.cc
@@ -60,7 +60,9 @@
     fillConn.block (true);
     fill->set_active (pp->commonTrans.autofill);
     fillConn.block (false);
-    autoCrop->set_sensitive (!pp->commonTrans.autofill);
+    if (autoCrop) {
+        autoCrop->set_sensitive (!pp->commonTrans.autofill);
+    }

     lastFill = pp->commonTrans.autofill;

@@ -100,7 +102,9 @@

         lastFill = fill->get_active ();
     } else {
-        autoCrop->set_sensitive (!fill->get_active());
+        if (autoCrop) {
+            autoCrop->set_sensitive (!fill->get_active());
+        }
     }

     if (listener) {

[heckflosse]

@Hombre57 I will test it now

[heckflosse]

@Hombre57 With your patch exactly the same number of gdi handels are leaking as without your patch. But that does not mean that it does not make sense. Reviewing ....

[Hombre57]

I think you should focus on line 697 of editorpanel.cc :

colorMgmtToolBar.reset (new ColorManagementToolbar (ipc));

PS: That's not my programing style, it's Adams' code and I had some difficulties to understand this coding level. Maybe that the ColorManagementToolbar is not freed since it doesn't use Gtk::manage !?

[Hombre57]

Preparing a patch...

Spoke too fast. There's no problem with this class it seem.

[Floessie]

@Hombre57

Maybe that the ColorManagementToolbar is not freed since it doesn't use Gtk::manage !?

I didn't have a closer look, but the .reset (new ColorManagementToolbar (ipc)) pattern looks like the correct use of a std::unique_ptr<>.

[agriggio]

I agree with @Floessie, I don't see anything (obviously) wrong with it

[heckflosse]

Here's a screen shot of gdiview. The selected entries are gdi handle leaks per editor. Maybe someone has an idea...

[heckflosse]

@Hombre57 @Floessie @agriggio

The following patch clearly shows that there is a leak here

The patch does not solve the leak, it just shows that there is a leak, because there is constr : wwwwwwwwwwwwwwwwww in console when an editor in setm is opened but there is no destr : wwwwwwwwwwwwwwwwww in console when the editor is closed

diff --git a/rtgui/curveeditor.cc b/rtgui/curveeditor.cc
index f8beff54..58cdbe4a 100644
--- a/rtgui/curveeditor.cc
+++ b/rtgui/curveeditor.cc
@@ -211,7 +211,7 @@ CurveEditor::CurveEditor (Glib::ustring text, CurveEditorGroup* ceGroup, CurveEd
     if (group && text.size()) {
         curveType = new PopUpToggleButton(text + ":");
     } else {
-        curveType = new PopUpToggleButton();
+        curveType = new PopUpToggleButton("xxx");
     }

     curveType->set_tooltip_text(M("CURVEEDITOR_TYPE"));
diff --git a/rtgui/popupbutton.h b/rtgui/popupbutton.h
index 245d29ae..090db2ad 100644
--- a/rtgui/popupbutton.h
+++ b/rtgui/popupbutton.h
@@ -28,7 +28,7 @@ class PopUpButton : public Gtk::Button, public PopUpCommon
 {

 public:
-    PopUpButton (const Glib::ustring& label = Glib::ustring (), bool nextOnClicked = false);
+    PopUpButton (const Glib::ustring& label = Glib::ustring ("wwwwwwwwwwwwwwwwww"), bool nextOnClicked = false);
     void show ();
     void set_tooltip_text (const Glib::ustring &text);
     void set_sensitive (bool isSensitive=true);
diff --git a/rtgui/popupcommon.cc b/rtgui/popupcommon.cc
index 47d9efe6..dc2d6978 100644
--- a/rtgui/popupcommon.cc
+++ b/rtgui/popupcommon.cc
@@ -24,12 +24,14 @@
 #include "popupcommon.h"
 #include "rtimage.h"
 #include "guiutils.h"
-
+#include <iostream>
 PopUpCommon::PopUpCommon (Gtk::Button* thisButton, const Glib::ustring& label)
     : buttonImage (nullptr)
     , menu (nullptr)
     , selected (-1) // -1 means that the button is invalid
 {
+    labelTemp = label;
+    std::cout << "constr : " << labelTemp << std::endl;
     button = thisButton;
     hasMenu = false;
     imageContainer = Gtk::manage( new Gtk::Grid());
@@ -51,6 +53,7 @@ PopUpCommon::PopUpCommon (Gtk::Button* thisButton, const Glib::ustring& label)

 PopUpCommon::~PopUpCommon ()
 {
+    std::cout << "destr : " << labelTemp << std::endl;
     delete menu;
     delete buttonImage;
 }
diff --git a/rtgui/popupcommon.h b/rtgui/popupcommon.h
index 8875f402..7a570cf6 100644
--- a/rtgui/popupcommon.h
+++ b/rtgui/popupcommon.h
@@ -68,7 +68,7 @@ private:
     Gtk::Button* button;
     int selected;
     bool hasMenu;
-
+    Glib::ustring labelTemp;
     void showMenu(GdkEventButton* event);

 protected:
diff --git a/rtgui/popuptogglebutton.cc b/rtgui/popuptogglebutton.cc
index 9676a2be..07359076 100644
--- a/rtgui/popuptogglebutton.cc
+++ b/rtgui/popuptogglebutton.cc
@@ -29,7 +29,7 @@
  * Parameters:
  *      label = label displayed in the button
  */
-PopUpToggleButton::PopUpToggleButton (const Glib::ustring& label) : Gtk::ToggleButton(), PopUpCommon(this, label) { }
+PopUpToggleButton::PopUpToggleButton (const Glib::ustring& label) : Gtk::ToggleButton(), PopUpCommon(this, label.empty() ? "xxxxxxxxxxxxxxxxx" : label) { }

 void PopUpToggleButton::show()
 {

Please review the code of ColorManagementToolbar. I'm lost with that coding style...

[agriggio]

Ingo, I'm taking a look

[heckflosse]

@agriggio Alberto, hold. I was wrong. It leaks here. Sorry for the confusion. Though I still don't understand the coding I mentioned in last post...

[agriggio]

I found something

[agriggio]

Ingo, can you try this?

diff --git a/rtgui/filepanel.cc b/rtgui/filepanel.cc
--- a/rtgui/filepanel.cc
+++ b/rtgui/filepanel.cc
@@ -157,6 +157,8 @@
     if (inspectorPanel) {
         delete inspectorPanel;
     }
+
+    delete tpc;
 }

 void FilePanel::on_realize ()
diff --git a/rtgui/icmpanel.cc b/rtgui/icmpanel.cc
--- a/rtgui/icmpanel.cc
+++ b/rtgui/icmpanel.cc
@@ -199,7 +199,7 @@
     Gtk::HBox *riHBox = Gtk::manage ( new Gtk::HBox());
     Gtk::Label* outputIntentLbl = Gtk::manage (new Gtk::Label(M("TP_ICM_PROFILEINTENT")+":"));
     riHBox->pack_start (*outputIntentLbl, Gtk::PACK_SHRINK);
-    ointent = Gtk::manage (new PopUpButton ());
+    ointent.reset (new PopUpButton ());
     ointent->addEntry("intent-perceptual.png", M("PREFERENCES_INTENT_PERCEPTUAL"));
     ointent->addEntry("intent-relative.png", M("PREFERENCES_INTENT_RELATIVE"));
     ointent->addEntry("intent-saturation.png", M("PREFERENCES_INTENT_SATURATION"));
diff --git a/rtgui/icmpanel.h b/rtgui/icmpanel.h
--- a/rtgui/icmpanel.h
+++ b/rtgui/icmpanel.h
@@ -83,7 +83,7 @@

     MyComboBoxText*    onames;
     sigc::connection   onamesconn;
-    PopUpButton*       ointent;
+    std::unique_ptr<PopUpButton>       ointent;
     sigc::connection   ointentconn;
     Gtk::RadioButton*  iunchanged;
     MyFileChooserButton* ipDialog;

[heckflosse]

Alberto, your patch is better than mine (which I didn't post yet). Neither your or my patch reduce the number of leaking gdi handles. Both solve the leak in icmpanel and yours also makes a clean destroy of toolpanelcoordinator when closing rt. :+1: to commit your patch

[agriggio]

Ingo, unfortunately I have no way of testing for leaking GDI handles, I'm afraid you are on your own there :disappointed: Feel free to commit the patch though

[heckflosse]

Alberto, can you confirm (by console output), that the patch at least solves one missing destructor (which means it solves at least a leak, though no leak of a gdi handle)?

[Hombre57]

@agriggio That will be a silly question for you, but why is ointent.reset (new PopUpButton ()); preferable over ointent = Gtk::manage (new PopUpButton ()); ? Does it mean that only the Gtk::Button part of the class is deleted through Gtk::manage ?

[agriggio]

@Hombre57 I'm actually not sure I fully understand how Gtk::manage works. the button is added to a grid which is also one of its members. my guess is that this might create some sort of circular reference. I haven't investigated in detail yet. since I understand unique_ptr, I was just lazy did that :-) @heckflosse yes, it does solve some leaks

[heckflosse]

Does it mean that only the Gtk::Button part of the class is deleted through Gtk::manage ?

That's how I understand that too!

[heckflosse]

When using address sanitizer I get this:

=================================================================
==7760==ERROR: AddressSanitizer: new-delete-type-mismatch on 0x607000241820 in thread T0:
  object passed to delete has wrong type:
  size of the allocated type:   72 bytes;
  size of the deallocated type: 48 bytes.
    #0 0x7ff76213c6e8 in operator delete(void*, unsigned long) /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_new_delete.cc:140
    #1 0x7ff75e117987 in Gtk::Container::foreach(sigc::slot<void, Gtk::Widget&, sigc::nil, sigc::nil, sigc::nil, sigc::nil, sigc::nil, sigc::nil> const&) (/usr/lib/libgtkmm-3.0.so.1+0x32c987)
    #2 0x7ff75e117b11 in Gtk::Container::show_all_children(bool) (/usr/lib/libgtkmm-3.0.so.1+0x32cb11)
    #3 0xf5814b in PartialPasteDlg::PartialPasteDlg(Glib::ustring const&, Gtk::Window*) /home/ingo/repo-rt/rtgui/partialpastedlg.cc:373
    #4 0x1035889 in ProfilePanel::init(Gtk::Window*) /home/ingo/repo-rt/rtgui/profilepanel.cc:34
    #5 0x10e1946 in RTWindow::RTWindow() /home/ingo/repo-rt/rtgui/rtwindow.cc:95
    #6 0xdae620 in main /home/ingo/repo-rt/rtgui/main.cc:383
    #7 0x7ff75b69b439 in __libc_start_main (/usr/lib/libc.so.6+0x20439)
    #8 0x7bc7e9 in _start (/home/ingo/repo-rt/build/debug/rawtherapee+0x7bc7e9)

0x607000241820 is located 0 bytes inside of 72-byte region [0x607000241820,0x607000241868)
allocated by thread T0 here:
    #0 0x7ff76213b168 in operator new(unsigned long) /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_new_delete.cc:80
    #1 0x7ff75e118a20 in sigc::internal::typed_slot_rep<sigc::mem_functor0<void, Gtk::Widget> >::dup(void*) (/usr/lib/libgtkmm-3.0.so.1+0x32da20)

SUMMARY: AddressSanitizer: new-delete-type-mismatch /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_new_delete.cc:140 in operator delete(void*, unsigned long)
==7760==HINT: if you don't care about these errors you may set ASAN_OPTIONS=new_delete_type_mismatch=0
==7760==ABORTING

Does that mean the mismatch is in libgtkmm?

Version: 5.1-115-g3281332b
Branch: dev
Commit: 3281332b
Commit date: 2017-06-29
Compiler: cc 7.1.1
Processor: x86_64
System: Linux
Bit depth: 64 bits
Gtkmm: V3.22.1
Build type: debug
Build flags:  -std=c++11 -march=native -Werror=unused-label -fsanitize=address -fopenmp -Werror=unknown-pragmas -Wall -Wno-unused-result -Wno-deprecated-declarations -g
Link flags:  -march=native -fsanitize=address
OpenMP support: ON
MMAP support: ON