Memory leak when accessing AstalTray.item.menuModel

[navidmafi]

Describe the bug AstalTray.item.menuModel leaks leading to increased memory usage and later segfaults.

To Reproduce MCVE:

function TrayItemComponent({ item }: { item: AstalTray.TrayItem }) {
  let popovermenu: Gtk.PopoverMenu;
  let image: Gtk.Image;

  return (
    <Gtk.Box
      name={item.id}
      $={(widget) => {
        const handlerId = item.connect("notify", (i) => {
          widget.insert_action_group("dbusmenu", i.action_group);
          image.set_from_gicon(i.gicon);
        });

        widget.connect("destroy", () => {
          item.disconnect(handlerId);
        });
      }}
      class="BarItemContainer"
      tooltipMarkup={item.tooltipMarkup}
    >
      <Gtk.GestureClick
        button={Gdk.BUTTON_SECONDARY}
        onPressed={() => popovermenu.popup()}
      />
      <Gtk.GestureClick
        button={Gdk.BUTTON_PRIMARY}
        onPressed={(e, x, y) => item.activate(x, y)}
      />

      <Gtk.PopoverMenu
        $={(self) => (popovermenu = self)}
        // menuModel={item.menuModel}
      />
      <Gtk.Image $={(ref) => (image = ref)} />
    </Gtk.Box>
  );
}

I am unable to provide coredumps at this time, sorry.

Additional context Memory growth is both more controlled and later GCed and gjs is stable for dozens of hours.

[navidmafi]

With that line uncommented

[Aylur]

FYI the destroy signal is unreliable, it will not work. The closure increases the ref count of the item and it may (likely will) cause a leak. You have to use onCleanup

[navidmafi]

Thank you for clarifying that. Also for some reason I was inserting action group onto the container?! I'm not sure whether that was a mistake from my part or a side-effect of some quirk.

Modified to this:

let popovermenu: Gtk.PopoverMenu;
let image: Gtk.Image;
const handlerId = item.connect("notify", (i) => {
  popovermenu?.insert_action_group("dbusmenu", i.action_group);
  image?.set_from_gicon(i.gicon);
});
onCleanup(() => {
  item.disconnect(handlerId);
});

Should this work more reliably (at least in theory)?

[navidmafi]

No answer from your part.

Things have been going fine for the past month just by commenting out the menuModel supplier. Just enabling that single line causes steady increase in memory consumption. Here's the example:

function TrayItemComponent({ item }: { item: AstalTray.TrayItem }) {
  let popovermenu: Gtk.PopoverMenu;
  let image: Gtk.Image;
  const handlerId = item.connect("notify", (i) => {
    popovermenu?.insert_action_group("dbusmenu", i.action_group);
    image.set_from_gicon(i.gicon);
  });
  onCleanup(() => {
    item.disconnect(handlerId);
  });
  return (
    <Gtk.Box
      name={item.id}
      tooltipMarkup={item.tooltipMarkup}
      orientation={Gtk.Orientation.VERTICAL}
    >
      <Gtk.GestureClick
        button={Gdk.BUTTON_SECONDARY}
        onPressed={() => popovermenu.popup()}
      />
      <Gtk.GestureClick
        button={Gdk.BUTTON_PRIMARY}
        onPressed={(e, x, y) => item.activate(x, y)}
      />

      <Gtk.PopoverMenu
        $={(self) => (popovermenu = self)}
        // menuModel={item.menuModel}
      />
      <Gtk.Image vexpand $={(ref) => (image = ref)} />
    </Gtk.Box>
  );
}

[navidmafi]

Thingamabob for testing?

#!/usr/bin/env gjs

imports.gi.versions.Gtk = "3.0";
imports.gi.versions.AppIndicator3 = "0.1";
const { Gtk, GObject, Gio, GLib, GdkPixbuf } = imports.gi;

Gtk.init(null);

const randStr = () => Math.random().toString(36).substring(2, 8);

const makeMenu = () => {
  const m = new Gtk.Menu();
  for (let i = 0, n = Math.floor(Math.random() * 5) + 1; i < n; i++) {
    const label = `Action ${randStr()}`;
    const item = new Gtk.MenuItem({ label });
    item.connect("activate", () => print(`Activated: ${label}`));
    m.append(item);
  }
  m.show_all();
  return m;
};

const makeIcon = (size = 16) => {
  const data = new Uint8Array(size * size * 4).map((_, i) =>
    i % 4 === 3 ? 255 : Math.floor(Math.random() * 256)
  );
  const pixbuf = GdkPixbuf.Pixbuf.new_from_data(
    data,
    GdkPixbuf.Colorspace.RGB,
    true,
    8,
    size,
    size,
    size * 4,
    () => {}
  );
  const tmp = `/tmp/dyn-icon-${randStr()}.png`;
  pixbuf.savev(tmp, "png", [], []);
  return tmp;
};

const Doohickey = GObject.registerClass(
  class DynamicTrayApp extends Gtk.Application {
    _init() {
      super._init({ application_id: "io.doohickey" });
    }
    vfunc_startup() {
      super.vfunc_startup();
      const AppIndicator = imports.gi.AppIndicator3;
      this._indicator = AppIndicator.Indicator.new(
        "dynamic-tray",
        "system-run",
        AppIndicator.IndicatorCategory.APPLICATION_STATUS
      );
      this._indicator.set_status(AppIndicator.IndicatorStatus.ACTIVE);
      GLib.timeout_add(GLib.PRIORITY_DEFAULT, 100, () => {
        if (this._menu) this._menu.destroy();
        this._menu = makeMenu();
        this._indicator.set_menu(this._menu);
        this._indicator.set_icon_full(makeIcon(), "Random icon");
        return GLib.SOURCE_CONTINUE;
      });
    }
    vfunc_activate() {
      this.hold();
    }
  }
);

new Doohickey().run([]);

[gian-reto]

I also still see constantly increasing memory consumption with astal (hi from https://github.com/Aylur/astal/issues/262! I have since rewritten my config from scratch for ags v3 and moved it to its own repo: https://github.com/gian-reto/adw-shell. I still experience a steady increase in memory consumption, similar to v2.). This is a total shot in the dark, and I have never written a line of vala in my life, but in the following section, a new menu importer instance is created whenever the menu path changes, and a subscription is created (.connect(...)) on the menu model:

https://github.com/Aylur/astal/blob/189bf73016c26d7d32729a913d6436cd7b1a0885/lib/tray/src/tray-item.vala#L384-L397

If one already exists, the existing subscription is not disconnected, and the instance is not disposed. Note: This could be totally fine! As I said above, this is just a wild guess.

[navidmafi]

I have no idea what's happening in the vala code but I did implement some changes. Will report back and thank you for the investigation!

[kotontrion]

If one already exists, the existing subscription is not disconnected, and the instance is not disposed.

This is not true. While it would be better practice to explicitly disconnect, when a new menu_importer is created, the old one will get destroyed, which automatically disconnects the handlers. Also this part of the code is usually only run once when the item is created, i know of no implementation, which actually changes the menu path during runtime.

I checked my own bar (written in vala) with valgrind and could not find memory leaks related to the tray, i only let it run for a few minutes though. This would suggest the issue might lie with your code, gnim or some ffi interaction.

I did find memory leaks in the wireplumber and cava libs which should be fixed in #418, but they are not related to the tray

[navidmafi]

My experiments didn't result in any useful findings.

Currently, I'm using the following workaround which i guess works fine for the time being.

  const updateMenu = () => {
    popovermenu?.set_menu_model(item.menuModel);
    popovermenu?.insert_action_group("dbusmenu", item.action_group);
  };
// ...
 <Gtk.GestureClick
          button={Gdk.BUTTON_SECONDARY}
          onPressed={() => {
            updateMenu();
            popovermenu?.popup();
          }}
        />

ref: https://github.com/navidmafi/ags_cfg/blob/0068745409d7dfe325ea11d6247d8eab55a63d7e/widget/Bar/Tray.tsx