obs-studio icon indicating copy to clipboard operation
obs-studio copied to clipboard

UI: Shortcut filter refactor to fix numerous memory leaks

Open notr1ch opened this issue 3 years ago • 2 comments

Description

This PR refactors the shortcut filter implementation to create a single, public OBSEventFilter instance for the entire application instead of relying on every widget to create and free its own shortcut filter. An event filter can be shared provided it's created in the same thread, and since these are all UI operations they should all be executing in the Qt UI thread.

Motivation and Context

Many CreateShortcutFilter calls were not tracked and would leak memory.

Indirect leak of 96 byte(s) in 2 object(s) allocated from:
    #0 0x55fe148a765d in operator new(unsigned long) (/home/r1ch/obs-studio/build/rundir/RelWithDebInfo/bin/64bit/obs+0x21365d) (BuildId: 361227000132ec995ad483aef4b23909e4e88dc4)
    #1 0x55fe148e4f0a in CreateShortcutFilter() /home/r1ch/obs-studio/UI/obs-app.cpp:115:9
    #2 0x55fe14c90c82 in OBSBasicProperties::OBSBasicProperties(QWidget*, OBSSafeRef<obs_source*, &(obs_source_get_ref), &(obs_source_release)>) /home/r1ch/obs-studio/UI/window-basic-properties.cpp:91:21
    #3 0x55fe14b13f4e in OBSBasic::CreatePropertiesWindow(obs_source*) /home/r1ch/obs-studio/UI/window-basic-main.cpp:2849:19
    #4 0x7f03bd1a9d7c in QWidget::event(QEvent*) (/lib/x86_64-linux-gnu/libQt6Widgets.so.6+0x1d9d7c) (BuildId: 1b8a809c3e427a6488ddc759bd04d21b53094660)

How Has This Been Tested?

Compiled and ran OBS, verified filter behaved as intended.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • [x] My code has been run through clang-format.
  • [x] I have read the contributing document.
  • [x] My code is not on the master branch.
  • [x] The code has been tested.
  • [x] All commit messages are properly formatted and commits squashed where appropriate.
  • [x] I have included updates to all appropriate documentation.

notr1ch avatar Oct 07 '22 18:10 notr1ch

My reasons for keeping this draft at the moment:

  • Is having the shortcutFilter on OBSApp OK?
  • Is there a better way to do this, e.g. with the global event hook we use for display affinity, so it also applies to windows and dialogs created by plugins?

notr1ch avatar Oct 10 '22 09:10 notr1ch

Undrafting to encourage feedback :).

notr1ch avatar Dec 26 '22 15:12 notr1ch

Is there a better way to do this, e.g. with the global event hook we use for display affinity, so it also applies to windows and dialogs created by plugins?

I think it might be possible to just install an event filter on OBSApp (e.g., in the OBSApp constructor)? Question is if/how that would interfere with widget-level filters that we still use in some places.

gxalpha avatar Jun 11 '23 01:06 gxalpha

IMO a single global event filter is desirable over a multitude of event filters all doing their own thing without any oversight (also makes debugging events a chore).

I think this change is a good enough step in the right direction and if this doesn't break in major ways, we could extend it to other areas after that.

PatTheMav avatar Dec 08 '23 14:12 PatTheMav