Skip to content
This repository has been archived by the owner on May 16, 2020. It is now read-only.

Fix crash at exit by allocating QObjects on the heap #303

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix crash at exit by allocating QObjects on the heap #303

wants to merge 2 commits into from

Conversation

joanbm
Copy link

@joanbm joanbm commented Apr 19, 2020

In multiple commits between 1.6.6 and 1.6.7, some objects that derive
from QObject were added which do not have their own heap allocation,
but rather are global variables or live inside another object.
However, because ultimately Qt owns those objects and is responsible
for releasing them, they must have their own allocation.
Otherwise, Qt will attempt to call free on an invalid pointer, which
will most likely cause a crash on exit.

This commit should not introduce a leak since the objects are released
by Qt. Additionally, AFAICT the call to std::atexit() can be safely
removed, since the settings are already saved at VodManager's
destructor, which happens at application exit.

Fixes: #302

CC @mrgreywater

@ahjolinna
Copy link
Contributor

ahjolinna commented Apr 20, 2020

seems to work & builds just fine https://build.opensuse.org/package/show/home:ahjolinna/orion

@mrgreywater
Copy link
Contributor

mrgreywater commented Apr 20, 2020

Thank you for contributing. I'll look into this PR/issue when I have some time. Just my two cents after glancing over it:

  • The way you're creating the singleton now looses thread-safety.
  • Please verify the destructor of the vodmanager is called properly by the qt before removing the std::atexit callback on assumption.

@joanbm
Copy link
Author

joanbm commented Apr 20, 2020

* The way you're creating the singleton now looses thread-safety.

I've gone ahead and updated my PR to make both the singleton initializers for VodManager and SettingsManager thread safe (and still lazy), by initializing them using a static local variables. I've also updated the rest of the singletons (except NetworkManager which requires a more complex implementation) to follow this pattern in a new commit, as to have both uniform and thread-safe singletons everywhere.

Note that also thread-safety wasn't currently an issue for VodManager and SettingsManager anyway since were they were created early on src/main.cpp:main before creating any more threads ourselves. I assume it wasn't also an issue for the other ones, since they were not thread-safe before, but making them thread-safe now can't make things worse.

* Please verify the destructor of the vodmanager is called properly by the qt before removing the std::atexit callback on assumption.

The VodManager destructor is called at exit time since its parent QObject is set to the QApplication (see the call to src/main.cpp:registerQmlComponents from src/main.cpp:main), so it is destroyed (using the QObject ownership mechanism) at the end of main, when the application is closed. The destructors for the other objects are also called as well when their parent is destroyed by the same mechanism.

I have also done a primitive test to verify this works in practice (i.e. all destructors are called at the right time and don't leak) by printing constructor/destructor events and checking that they all match up and are called at the right time:

diff --git a/src/model/imageprovider.cpp b/src/model/imageprovider.cpp
index 3810dbb..3f847f4 100644
--- a/src/model/imageprovider.cpp
+++ b/src/model/imageprovider.cpp
@@ -217,6 +217,7 @@ void DownloadHandler::replyFinished() {
 
 // CachedImageProvider
 CachedImageProvider::CachedImageProvider(ImageProvider const* provider) : QQuickImageProvider(QQuickImageProvider::Image), _provider(provider) {
+    printf("CREATE CACHEDIMAGEPROVIDER\n");
 
 }
 
diff --git a/src/model/imageprovider.h b/src/model/imageprovider.h
index fab4271..d5472e2 100644
--- a/src/model/imageprovider.h
+++ b/src/model/imageprovider.h
@@ -52,6 +52,7 @@ class ImageProvider;
 class CachedImageProvider : public QQuickImageProvider {
 public:
     CachedImageProvider(ImageProvider const* provider);
+    ~CachedImageProvider() { printf("DESTROY CACHEDIMAGEPROVIDER\n"); }
     QImage requestImage(const QString &id, QSize * size, const QSize & requestedSize);
 private:
     ImageProvider const* _provider;
diff --git a/src/model/settingsmanager.cpp b/src/model/settingsmanager.cpp
index 60d8705..bdc5005 100644
--- a/src/model/settingsmanager.cpp
+++ b/src/model/settingsmanager.cpp
@@ -5,6 +5,7 @@
 SettingsManager::SettingsManager(QObject *parent) :
     QObject(parent), settings(QCoreApplication::organizationName(), QCoreApplication::applicationName(), this)
 {
+    printf("CREATE SETTINGSMANAGER\n");
     load();
     //Connections
     connect(HttpServer::getInstance(), &HttpServer::codeReceived, this, &SettingsManager::setAccessToken);
diff --git a/src/model/settingsmanager.h b/src/model/settingsmanager.h
index d756add..b564380 100644
--- a/src/model/settingsmanager.h
+++ b/src/model/settingsmanager.h
@@ -81,6 +81,7 @@ class SettingsManager : public QObject
     bool mKeepOnTop = false;
 
     explicit SettingsManager(QObject *parent = nullptr);
+    ~SettingsManager() { printf("DESTROY SETTINGSMANAGER\n"); }
 public:
     static SettingsManager *getInstance();
 
diff --git a/src/model/vodmanager.cpp b/src/model/vodmanager.cpp
index af6913e..876e23a 100644
--- a/src/model/vodmanager.cpp
+++ b/src/model/vodmanager.cpp
@@ -21,6 +21,7 @@ VodManager::VodManager(QObject *parent) :
     QObject(parent),
     netman(NetworkManager::getInstance())
 {
+    printf("CREATE VODMANAGER\n");
     qmlRegisterInterface<VodListModel>("VodListModel");
     _model = new VodListModel(this);
 
@@ -50,6 +51,7 @@ VodManager *VodManager::getInstance() {
 
 VodManager::~VodManager()
 {
+    printf("DESTROY VODMANAGER\n");
     saveSettings();
     delete _model;
 }

joanbm added 2 commits April 20, 2020 14:28
In multiple commits between 1.6.6 and 1.6.7, some objects that derive
from QObject were added which do not have their own heap allocation,
but rather are global variables or live inside another object.
However, because ultimately Qt owns those objects and is responsible
for releasing them, they must have their own allocation.
Otherwise, Qt will attempt to call free on an invalid pointer, which
will most likely cause a crash on exit.

This commit does not introduce a leak since the objects are released
by Qt (using the QObject ownership mechanism). Note the call to
`std::atexit()` can be safely removed, since the settings are also
saved at `VodManager`'s destructor, which happens at application exit.

Fixes: #302
This commit simplifies and uniformizes parameterless singleton
initialization patterns through the code base.

Additionally, and though currently not relevant, the idiom used is both
thread-safe and lazy, as guaranteed by the C++11 standard. See also:
https://en.cppreference.com/w/cpp/language/storage_duration#Static_local_variables

Note that the initializers for the VodManager and SettingManager
singletons were changed in 25575da.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crashes on opening channels and on exit (memory safety issues?)
3 participants