php-gtk3 icon indicating copy to clipboard operation
php-gtk3 copied to clipboard

Method "connect" leaks memory OR method "destroy" doesn't destroy all children

Open ntfs1984 opened this issue 1 year ago • 22 comments

Hello. Found that "connect" method leaks memory if applied to previously declared and even destroyed object.

Example.

<?php
Gtk::init();
function GtkWindowDestroy($widget=NULL, $event=NULL)
 {
 Gtk::main_quit();
 }

function refresh() 
 {
 global $win, $box, $button;
 $box->destroy();
 $button->destroy();	
 $box = new GtkBox(GtkOrientation::HORIZONTAL);
 $button = GtkButton::new_with_label(rand(1,100));
 $button->connect("clicked",function($button) {}); // Comment this to stop memory leak
 $box->add($button);
 $win->add($box);	
 $button->show();
 $box->show();
 echo "\n".round(memory_get_usage()/1024,2) . " Kb\n";
 } 

$win = new GtkWindow();
$win->set_default_size(300, 200);
$win->connect("destroy", "GtkWindowDestroy");
$box = new GtkBox(GtkOrientation::HORIZONTAL);
$button = GtkButton::new_with_label(rand(1,100));
$box->add($button);
$win->add($box);
Gtk::timeout_add(2000, function () {refresh();return true;});
$win->show_all();
Gtk::main();

This code leaks memory upon every iteration of timeout_add. If we comment line "$button->connect("clicked",function($button) {});" - memory stopped to leak, this means that g_signal_connect doesn't replace function within previously declared object, but add it.

Is there a way to fix such situation according example? Thanks.

ntfs1984 avatar Jan 26 '24 16:01 ntfs1984

Hey Hello!

As you can see on doc, destroy not free memory

widget will still point to valid memory, allowing you to release the references you hold, but you may not query the widget’s own state.

Look a nice reference too

gdk_destroy() just sends the signal to politely ask everyone to remove their references, but doesn't force any references to disappear. The issue was that I somehow misunderstood the effect of ref_sink(), which does NOT need to paired with a ref() call

Manage memory on GTK is not a simple task, as well the problem of clone objects

If you want a deep understand of Gtk memory, this a famous article

In any case, I dont see any use of recreate all stack of widgets every 2 seconds, as we can use global variables or so! This cause not only memory problem, but use of cpu and disk also

scorninpc avatar Jan 26 '24 17:01 scorninpc

I'm still checking all possible solutions to create system tray for your dock.

I found way to do this over d-bus.

Any change of system tray state (for example incoming message to messenger) requires re-draw of system tray (like it used in X system tray apps) with new properties of applications.

So, recreation of stack performing not at every 2 sec, but at every changed state. Received message to Telegram - icon recreated, +2Kb of memory used. Have read message of Telegram - icon recreated, +2Kb of memory used.

But thanks for links, will dig how to resolve these memory leaks.

ntfs1984 avatar Jan 26 '24 17:01 ntfs1984

if you do successfully icon tray, i'd like to take a look and add this to cookbook. I know this is a UX features of all OS (windows, linux, mac) but I love systray too

scorninpc avatar Jan 26 '24 17:01 scorninpc

Well, if shortly:

  • There is exists utility called "snixembed". It's proxying old xembed calls to modern SNI to d-bus
  • After run this utility in background, all new apps who using system tray - will create new D-bus service StatusNotifierItem.
  • In this service are methods and properties for: a) get current icon to display it; b) title and tooltip; c) method which we might simply call and application will activate or display context menu.

I'm not professional programmer, so not able to see all best methods, but you can use this snixembed app and d-feet utility to understand how it works.

ntfs1984 avatar Jan 26 '24 17:01 ntfs1984

ahhh very nice workaround!

scorninpc avatar Jan 26 '24 17:01 scorninpc

Anyway want to re-open ticket just for your info. Step by step I found root of memory leak, and it's not GTK.

Function "Php::Value GObject_::connect_internal" of GObject.cpp contains some struct called "callback_object". This struct seems is not freeing before end of function.

It's not a critical bug, but you will 100% meet it once will develop some software with content refresh and big uptime, like dockStupido.

ntfs1984 avatar Mar 08 '24 00:03 ntfs1984

Oh! nice to know! I'll investigate if we can free this to prevent it. Thank you for report

scorninpc avatar Mar 08 '24 01:03 scorninpc

Added callback_object to public field of class to make it visible outside. Not sure how to explain this correctly.

GObject.h:

        public:
struct st_callback;
st_callback *callback_object;

Then added deletion of this var into handler_disconnect function.

GObject.cpp:

void GObject_::handler_disconnect(Php::Parameters &parameters)
{
    Php::Value callback_handle = parameters[0];
    delete callback_object;
    g_signal_handler_disconnect(instance, (int)callback_handle);
}

So now we can run handler_disconnect when necessary and it will also release memory used by struct. Tested, this works, so you can get this idea.

ntfs1984 avatar Mar 09 '24 05:03 ntfs1984

Ok, i got, but if you disconnect before reconnect your self with PHP, without delete, still leaking memory?

Because what are telling make sense, but all structures on callback_object are primitive like or small classes. it's free some bytes only

I'll update with this code, because make sense free the object, but it's not will leak so many memory

Anyway, you cannot do this object as public, beause all widget will share the same object. You may need to register allocated memory with the same signal id, and free it after by this ID

I'll try to do something today and return here

scorninpc avatar Mar 10 '24 14:03 scorninpc

Yes, it's not leaking so many memory, just about 1Kb for iteration. But if we have many iterations - this leak is sensitive.

If we'll refresh every 250ms - our program will eat + 1Mb every 4.5 min, or + 1Gb every 3 days. It's so much. You can check with my initial example.

ntfs1984 avatar Mar 31 '24 23:03 ntfs1984

Store a list of structures of each signal of widget may be a pain, so I found a simplest way https://github.com/scorninpc/php-gtk3/commit/d176b1472bbb2757cf56fbe383bb808cbcf06369

image

Review please and close if it's OK

ps: sorry for delay, hard work days

scorninpc avatar Apr 11 '24 00:04 scorninpc

I compiled for windows, now i get a lot of those:

(php.exe :4948): GLib-GObject-CRITICAL **: 09:24:38.068: g_object_weak_unref: couldn't find weak ref 00007ffbe68f7e90(0000026b7388c0f0)

apss-pohl avatar Apr 16 '24 07:04 apss-pohl

Can you provide Gtk, GLib and Pango version?

https://bugzilla.mozilla.org/show_bug.cgi?id=503792 https://gitlab.gnome.org/GNOME/loupe/-/issues/100

After save versions, try to update your libs

scorninpc avatar Apr 16 '24 10:04 scorninpc

mingw-w64-x86_64-gtk3 3.24.39-1 mingw-w64-x86_64-glib2 2.78.3-1 mingw-w64-x86_64-pango 1.50.14-4

Versions are stable, once i revert the changes the error disappears using same environment.

apss-pohl avatar Apr 16 '24 11:04 apss-pohl

Yes, the reverted version does not have g_object_weak_unref. All i found on web tell this is a glib related

image

Can you upgrade glib to 2.80? yours is 2.78

scorninpc avatar Apr 16 '24 11:04 scorninpc

I just noticed that it is even more complicated.

We are stuck on php7.4 and there is an even more minor version in place: glib-2.53.3-vc15-x64.zip This is part of the official php build procedure which is also in place on the Tutorial we have on this repo. Updating can lead to unforeseen effort and issues. Even php 8 for windows will use outdated version: glib-2.53.3-1-vs16-x64.zip

apss-pohl avatar Apr 16 '24 13:04 apss-pohl

for real? i have 8.1 compiled here!

I'll do another VM to make a new compile env, and try to do some tests

But i'm sure that problem is part of GTK/Glib/Pando problem, not PHP-GTK it's self

Maybe try clear structure by another way to avoid this bug, but ... need tests

scorninpc avatar Apr 16 '24 17:04 scorninpc

Did you try update only on mingw?

image

scorninpc avatar Apr 16 '24 17:04 scorninpc

Yes i tried this and the version is "mingw-w64-x86_64-glib2-2.80.0-1"

I also updated the build depedencies for php itself against the lastest versions from msys and rebuild it +gtk3 on top of it. But the issue persists..

PS. Also happen on linux

apss-pohl avatar Apr 17 '24 09:04 apss-pohl

sad to hear that! i think i'll try to avoid g_object_weak_unref method

Alot issue about that https://github.com/search?q=g_object_weak_unref&type=issues https://github.com/search?q=%22couldn+t+find+weak+ref%22&type=issues

can you try to run with param GSETTINGS_BACKEND=memory please?

GSETTINGS_BACKEND=memory php.exe test.php for example

edited: merge posts

scorninpc avatar Apr 17 '24 12:04 scorninpc

Just tried it but i don't see anything changed. What to expect? IF you are online we can also chat on discord for testing

apss-pohl avatar Apr 17 '24 13:04 apss-pohl

Just tested. Works fine without any tricks, thanks.

@apss-pohl - tested with php8.1 on archlinux - running without problems.

ntfs1984 avatar Apr 30 '24 21:04 ntfs1984

Hi Bruno,

do you consider to revert that change? g_object_weak_unref still causes the issues

apss-pohl avatar Jul 10 '24 09:07 apss-pohl

Hey, how are you!

Yes, i'm waiting you confirm if I can revert

scorninpc avatar Jul 10 '24 14:07 scorninpc

Well i am using it without the "g_object_remove_weak_pointer" all the time very intensive, i don't have any issues. Here is a PR: https://github.com/scorninpc/php-gtk3/pull/132

apss-pohl avatar Jul 23 '24 12:07 apss-pohl

Ah ok! Very nice

But i think you can remove this PR, because there is more thing to remove https://github.com/scorninpc/php-gtk3/commit/d176b1472bbb2757cf56fbe383bb808cbcf06369 and revert is more easy

scorninpc avatar Jul 23 '24 12:07 scorninpc

Whatever fits best for you. I only did this one liner and it seems fine, but i also have no clue what the other stuff will do

apss-pohl avatar Jul 23 '24 12:07 apss-pohl

fixed https://github.com/scorninpc/php-gtk3/commit/09e9cdec470c1fef8682133baa013c447cc8e753

i just comment methods, becase i loved to find that to just remove it now hahah

scorninpc avatar Jul 25 '24 02:07 scorninpc