Crash when closing application

Joel Bodenmann

@kshegunov
I actually have three plugin classes, each inheriting from the other:
UML
The corresponding headers:
Plugin:

#pragma once

class QString;
class QStringList;
class QWidget;

class Plugin
{
public:
    virtual ~Plugin() {}

    virtual QString pluginName() const = 0;
    virtual QString pluginVersion() const = 0;
    virtual QString pluginAuthor() const = 0;
    virtual QWidget* toolWidget() = 0;
    virtual QString toolWidgetName() const = 0;
    virtual QWidget* settingsWidget() = 0;
    virtual void settingsApply() = 0;
};

#define Plugin_iid "com.mycrashyapp.Plugin"
Q_DECLARE_INTERFACE(Plugin, Plugin_iid)

Viewer:

#pragma once

#include <memory>
#include "interfaces/plugin.h"

class QString;
class QStringList;
class QWidget;

class Viewer : public Plugin
{
public:
    virtual ~Viewer() {}

    virtual std::unique_ptr<Viewer> clone() const = 0;  // Can't use copy-constructor in an interface
    virtual QStringList fileExtensions() const = 0;
    virtual bool fileOpen(const QString& filePath) = 0;
    virtual void fileClose() = 0;
    virtual bool fileExists() const = 0;
    virtual QString fileName() const = 0;
    virtual QString filePath() const = 0;
};

#define Viewer_iid "com.mycrashyapp.Viewer"
Q_DECLARE_INTERFACE(Viewer, Viewer_iid)

Editor:

#pragma once

#include "interfaces/viewer.h"

class Editor : public Viewer
{
public:
    virtual ~Editor() {}

    virtual bool fileSave() = 0;
    virtual bool fileSetPath(const QString& filePath) = 0;
    virtual bool fileNeedsSave() const = 0;
};

#define Editor_iid "com.mycrashyapp.Editor"
Q_DECLARE_INTERFACE(Editor, Editor_iid)

Finally, this is how I create a plugin:

#pragma once

#include <QObject>
#include "../elixpad/interfaces/editor.h"
#include "settings/settings.h"

class ToolWidget;
class SettingsWidget;

class CodeEditorPlugin : public QObject, public Editor
{
    Q_OBJECT
    Q_PLUGIN_METADATA(IID Editor_iid)
    Q_INTERFACES(Editor)

public:
    explicit CodeEditorPlugin(QObject* parent = nullptr);
    virtual ~CodeEditorPlugin() override;

    virtual std::unique_ptr<Viewer> clone() const override;
    virtual QString pluginName() const override;
    virtual QString pluginVersion() const override;
    virtual QString pluginAuthor() const override;
    virtual QString toolWidgetName() const override;
    virtual QWidget* toolWidget() override;
    virtual QWidget* settingsWidget() override;
    virtual void settingsApply() override;
    virtual QStringList fileExtensions() const override;
    virtual bool fileOpen(const QString& filePath = QString()) override;
    virtual void fileClose() override;
    virtual bool fileExists() const override;
    virtual QString fileName() const override;
    virtual QString filePath() const override;
    virtual bool fileSave() override;
    virtual bool fileSetPath(const QString& filePath) override;
    virtual bool fileNeedsSave() const override;

private:
    Q_DISABLE_COPY(CodeEditorPlugin)

    ToolWidget* _toolWidget;
    SettingsWidget* _settingsWidget;
};

I hope I didn't screw up too much... :S
Using that code always returns a nullptr when using qobject_cast. Could this be related to my crash issues?

And thank you very much for your help. I appreciate it a lot!

P.S.: I don't really like the STL API ;)

kshegunov

@Joel-Bodenmann said:

I hope I didn't screw up too much... :S

Nope, it looks pretty okay.

 virtual std::unique_ptr<Viewer> clone() const override;

Forget this stuff when working with QObjects they are non-copyable for a reason. You're only entering a world of hurt for no reason by exposing this.

Using that code always returns a nullptr when using qobject_cast. Could this be related to my crash issues?

It shouldn't but perhaps you should list all the interfaces:

class CodeEditorPlugin : public QObject, public Editor
{
    Q_OBJECT
    Q_PLUGIN_METADATA(IID Editor_iid)
    Q_INTERFACES(Plugin Viewer Editor)
    // ... 
};

And thank you very much for your help. I appreciate it a lot!

You're welcome, let's hope I'll be able to help in the end. :)

P.S.: I don't really like the STL API ;)

It's so terrible that I'm pretty sure even its authors can't really stand it ... ;)

May we take a look at how you create ToolWidget* _toolWidget; and where you pass it on to Qt (I assume you're doing), also if/where you delete the object?

Joel Bodenmann

@kshegunov said:

Forget this stuff when working with QObjects they are non-copyable for a reason. You're only entering a world of hurt for no reason by exposing this.

Sorry for annoying you with those unique pointers. I will get rid of them. I promise! :p
It just seems a good idea - in this case - as they explicitly transfer ownership of the cloned object to the caller.

It shouldn't but perhaps you should list all the interfaces:

Did that, no change, still crashing :(

It's so terrible that I'm pretty sure even its authors can't really stand it ... ;)

I thought I'm the only one... The API really is ridiculous in my opinion. Of course I understand that certain things are due to historical, legacy & consistency reasons but... such a terrible API :p

May we take a look at how you create ToolWidget* _toolWidget; and where you pass it on to Qt (I assume you're doing), also if/where you delete the object?

Yes Sir we may:

CodeEditorPlugin::CodeEditorPlugin(QObject* parent) : QObject(parent)
{
    // Tool widget
    _toolWidget = new ToolWidget;

    // Settings widget
    _settingsWidget = new SettingsWidget;
}

CodeEditorPlugin::~CodeEditorPlugin()
{
    delete _toolWidget;
    delete _settingsWidget;
}

QWidget* CodeEditorPlugin::toolWidget()
{
    return _toolWidget;
}

And this is where I add the toolWidget to by Dock class which is a QDockWidget subclass:

Dock::Dock(QWidget* parent) : QDockWidget(parent)
{
    ...
    setAttribute(Qt::WA_DeleteOnClose, false);
    ...
}

bool Dock::setViewer(Viewer* viewer)
{
    // Sanity check
    if (!viewer) {
        return false;
    }

    // Make sure that the viewer has a tool widget
    if (!viewer->toolWidget()) {
        return false;
    }

    setWidget(viewer->toolWidget());

    return true;
}

And here's my cleanup / closing sequence:

bool MainWindow::fileClose(Viewer& viewer)
{
    // First, save if it's an editor
    Editor* editor = dynamic_cast<Editor*>(&viewer);
    if (editor) {
        if (!ViewerManager::viewerClose(*editor)) {
            return false;
        }
    } else {
        viewer.fileClose();
    }

    // Get rid of the dock
    Dock* dock = dockFromViewer(viewer);
    if (dock) {
        _docks.removeAll(dock);
        delete dock;
    }

    return true;
}

bool MainWindow::fileCloseAll()
{
    for (Viewer* viewer : viewers()) {
        // Sanity check
        if (!viewer) {
            qCritical("MainWindow::fileCloseAll(): Invalid Viewer (nullptr).");
            return false;
        }

        // Close the viewer. Will prompt if user action required.
        if (!fileClose(*viewer)) {
            qInfo("MainWindow::fileCloseAll(): fileClose() failed.");
            return false;
        }
    }

    return true;
}

void MainWindow::closeEvent(QCloseEvent* event)
{
    // Close all editors. Will prompt if save required.
    if (!fileCloseAll()) {
        event->ignore();
        qInfo("MainWindow::closeEvent(): fileCloseAll() failed.");
        return;
    }

    // Save the state
    QSettings settings;
    settings.setValue("geometry", saveGeometry());
    settings.setValue("windowState", saveState());

    // Close the window
    QMainWindow::closeEvent(event);
}

Note: I commented out both the delete in CodeEditorPlugin::~CodeEditorPlugin() on the _toolWidget and the delete in MainWindow::fileClose() on the dock. The crash remains.

Just for completeness, here's the relevant parts of the ViewerManager class that is used in MainWindow::fileClose() (both static methods):

bool ViewerManager::viewerClose(Viewer& viewer)
{
    Editor* editor = dynamic_cast<Editor*>(&viewer);
    if (editor) {
        // Let the user decides if he wants to save or discard (or cancel)
        if (editor->fileNeedsSave()) {
            QMessageBox msgBox;
            msgBox.setWindowIcon(Icons::favicon());
            msgBox.setWindowTitle("Closing File");
            msgBox.setText("The document has been modified.");
            msgBox.setInformativeText("Do you want to save your changes?");
            msgBox.setStandardButtons(QMessageBox::Save | QMessageBox::Discard | QMessageBox::Cancel);
            msgBox.setDefaultButton(QMessageBox::Save);

            switch (msgBox.exec()) {
            case QMessageBox::Save:
                if (!ViewerManager::editorSave(*editor)) {
                    qInfo("ViewerManager::viewerClose(): ViewerManager::editorSave() failed.");
                    return false;
                }
                 break;

            case QMessageBox::Cancel:
                return false;

            case QMessageBox::Discard:
                break;

            default:
                break;
            }
        }
    }

    // Ask the editor to close the file
    viewer.fileClose();

    return true;
}

bool ViewerManager::editorSave(Editor& editor)
{
    // No reason to do anything if the file doesn't need to be saved
    if (!editor.fileNeedsSave()) {
        return true;
    }

    // Ask the user for a file path if the file doesn't exist
    if (!editor.fileExists()) {
        // Get the path
        QString filePath = QFileDialog::getSaveFileName(nullptr, "Save file as...");
        if (filePath.isEmpty()) {
            qInfo("ViewerManager::editorSave(): Invalid file name.");
            return false;
        }

        // Apply the path
        if (!editor.fileSetPath(filePath)) {
            qCritical("ViewerManager::editorSave(): Editor::fileSetPath() failed.");
            return false;
        }
    }

    // Perform the actual saving action
    if (!editor.fileSave()) {
        qCritical("ViewerManager::editorSave(): Editor::fileSave() failed.");
        return false;
    }

    return true;
}

Thank you for going through all this code. I hope that it's not too terrible to read.

SGaist

Hi,

Do I understand correctly that you have three plugins that are linked one to another ?

kshegunov

@Joel-Bodenmann said:

Sorry for annoying you with those unique pointers. I will get rid of them. I promise! :p

I actually meant the cloning. You shouldn't clone QObjects at all. And since this is a plugin interface, it's a pretty safe bet the implementation would be a QObject subclass. :)

It shouldn't but perhaps you should list all the interfaces:

Did that, no change, still crashing :(

I meant to make qobject_cast work, not to solve the crash.

The API really is ridiculous in my opinion.

Couldn't have put it better myself.

Right. I think you may be getting a double delete somehow (although your PS makes that less probable). Still this:

 setWidget(viewer->toolWidget());

Will take ownership of the passed widget.

Could you try the following. Change the raw pointers in the plugin with QPointer<> specializations:

QPointer<ToolWidget> _toolWidget;
QPointer<SettingsWidget> _settingsWidget;

Then in the destructor you can do:

CodeEditorPlugin::~CodeEditorPlugin()
{
    if (_toolWidget)
        delete _toolWidget;

    if (_settingsWidget)
        delete _settingsWidget;
}

Also there two:

setAttribute(Qt::WA_DeleteOnClose, false);

and

 delete dock; // MainWindow::fileClose

~~Look somewhat suspicious.~~ (EDIT: I missed that you actually disable the delete on close attribute)
But then you said you removed the delete. Perhaps you could try stripping most of the code to prepare a minimal example that reproduces this? I can't find anything plainly wrong besides the two remarks above.

Joel Bodenmann

@SGaist

Do I understand correctly that you have three plugins that are linked one to another ?

Yes I am aware of that. In fact, they are not just somehow "linked" together but they actually inherit from each other. This is the behavior that I want. I want to be able to load plugins in my application that allow just viewing a file without modifying it and plugins that are full editors that allow modifying and saving the file. As you can see in the UML diagram from my previous post the Editor needs everything the Viewer provides.
The Plugin class is just a base class that allows me retrieving basic information about the plugin such as the plugin name, version and the author. This base class will also be useful in the future when there are plugins that aren't actually file viewers.
To summarize: Viewer inherits form Plugin, Editor inherits from Viewer.

@kshegunov said:

Sorry for annoying you with those unique pointers. I will get rid of them. I promise! :p
I actually meant the cloning. You shouldn't clone QObjects at all. And since this is a plugin interface, it's a pretty safe bet the implementation would be a QObject subclass. :)

I need a way to clone my Viewer. As you can see, the Viewer is a plugin that allows opening and displaying a file. The Viewer::toolWidget() is the actual widget that displays the file that will be added to the dock widget. As I want to be able to open multiple files at once, I need a way to create multiple instances of the same Viewer class/plugin.
But when actually writing this... I guess the name clone() is misleading in this case. Because all it does is creating a new object and returning it directly:

std::unique_ptr<Viewer> CodeEditorPlugin::clone() const
{
    return std::unique_ptr<Viewer>(new CodeEditorPlugin);
}

I actually meant the cloning. You shouldn't clone QObjects at all. And since this is a plugin interface, it's a pretty safe bet the implementation would be a QObject subclass. :)

Oh... Sorry... Yeah now the qobject_cast is actually working. I guess I have some more reading to do before I understand why. But thanks for the remark! I like to be Qt compliant wherever possible ;)

Could you try the following. Change the raw pointers in the plugin with QPointer<> specializations:

Did that, same result. The issue persists.

I will try to create a minimum test-case that allows you reproducing the problem. However, that might take a couple of days (most likely I'll get it done over the weekend).
I just hope that I am able to reproduce the error in a stripped down version :p

SGaist

Just to be sure, you have something like:

plugin2.pro:

LIBS += -lplugin1

plugin3.pro:

LIBS += -lplugin2

?

Joel Bodenmann

@SGaist
Not at all. The Plugin, Viewer and Editor files are all single header files in my main application directory. My plugins (eg. the CodeEditor) are separate projects that simply include those files:

elixpad.pro and plugins.pro are just project files containing the sub-projects. Nothing in there besides SUBDIRS.

This is what the plugin project files look like (codeeditorplugin.pro):

TEMPLATE        = lib
CONFIG         += plugin silent c++11
QT             += widgets
INCLUDEPATH    += ../../elixpad
TARGET          = $$qtLibraryTarget(codeeditorplugin)
DESTDIR         = ../../elixpad/plugins

@SGaist Just to be sure that we are on the same page: Everything works great. I can use the plugins and I don't have any issues at all. The only issue I am experiencing is the crash when the application closes.

kshegunov

@Joel-Bodenmann said:

The Plugin class is just a base class that allows me retrieving basic information about the plugin such as the plugin name, version and the author. This base class will also be useful in the future when there are plugins that aren't actually file viewers.
To summarize: Viewer inherits form Plugin, Editor inherits from Viewer.

I wouldn't design it exactly like this, but I suggest leaving that question for after finding the problem.

But when actually writing this... I guess the name clone() is misleading in this case.

I must have been misled. I thought you're copying QObjects with this.

As you can see, the Viewer is a plugin that allows opening and displaying a file.

I don't think it should be. But rather a class that's exposed from the plugin. That's what I was talking about in the first sentence. A plugin is a single(ton) object (of the .dll/.so) that represents the entry point. You can think of it as a factory of sorts. So usually the most convenient way to deal with that is like this:

class ViewerProvider
{
public:
    virtual QList<Viewer> viewers() = 0;
}
Q_DECLARE_INTERFACE(ViewerProvider) //< A plugin interface

class EditorProvider
{
public:
    virtual QList<Editor> editors() = 0;
};
Q_DECLARE_INTERFACE(EditorProvider) //< A plugin interface

Then the plugin will implement both if it provides both editors and viewers:

class MyCoolPlugin : public QObject, public ViewerProvider, public EditorProvider
{
    Q_OBJECT
    Q_PLUGIN_METADATA(...)
    Q_INTERFACES(ViewerProvider EditorProvider) //< Listing all interfaces so moc will generate the TI we need for qobject_cast
    // ....
};

Now, when loading we can inquire the plugin what it provides:

QPluginLoader pluginLoader(...);
QObject * pluginObject = pluginLoader.instance(); //< This is the plugin - the entry point of the library, not the functionality we're after
EditorProvider * editorprovider = qobject_cast<EditorProvider>(pluginObject);
if (editorprovider)  {
   // Superb our plugin provides editors, we can load those, or store the pointer to the plugin or whatever we need to do
   // The point is the plugin itself only aggregates the functionality
}
// ... And so on (analogically with ViewerProvider)

And by the way you don't need the destructors in your interfaces. They will generate object code (because virtual methods can't be inlined) and you don't really need to enforce them virtual, as QObject already does that.

I will try to create a minimum test-case that allows you reproducing the problem. However, that might take a couple of days (most likely I'll get it done over the weekend).

If there's nothing secretive about the code (i.e. it's not protected by copyright or something like this) you could upload it in a repo somewhere as it is. I'd download it in the evening and check if I can find the problem.

Kind regards.

Joel Bodenmann

@kshegunov
Thank you for your comments regarding the design choices. I will be happy to talk more about this once the current issue is resolved. I am very keen on learning how to do it the right / better way. It's the first time I am working with plugins and the likelihood of getting it wrong is quite high with these sorts of things.

I assume that the poor design choices I made are not the issue that is leading to the current problem?

I'll make sure that you get access to the code base. Thank you once again for your help!

kshegunov

@Joel-Bodenmann
I found the error. The problem is you have widgets that outlive your QApplication object. So calling delete on them (after the root QObject has died) makes all kinds of nasty stuff inside Qt. You should clearly(!) declare your objects' lifetimes.

That singleton Preferences object is holding references to QObject instances which will be deleted after the main()'s stack frame has closed - not cool.
If you remove the delete statements in your plugin destructors you will not get a crash, but memory is leaked (not crucial here, but a good thing to fix).

The interesting part is that the exact same binary crashes on Windows 7, but not on Windows 10. I also get the crash on Ubuntu 15.04.

That's loader init/deinit for you. Or to cite myself:

As for the crash, if I had to guess, you have static QObject that's running de-init (the bottom of the stack) when the dll's unloading (but that's after QApplication's destructor has run, which isn't allowed).

Not an exact match, but close enough. You can't have widgets (or QObjects in general) outside of QApplication's lifetime. /there are few minor exceptions, which have no bearing here/ :)

If you ask for a "good" fix - I recommend reconsidering your design.
Also, use the designer(!), otherwise you'd end up only writing widget positioning/tweaking code and nothing else. :)

Kind regards.

Joel Bodenmann

So to wrap this up: The problem is/was that QPluginLoader::unload() is automatically called AFTER the application itself has been destroyed. Hence one shouldn't destroy the plugin objects before by either manually calling delete on them or by setting a parent that will be deleted.

Joel Bodenmann

@kshegunov said:

And by the way you don't need the destructors in your interfaces. They will generate object code (because virtual methods can't be inlined) and you don't really need to enforce them virtual, as QObject already does that.

The reason I added those virtual destructors there is because Qt documenation adviced me to do so: https://doc.qt.io/qt-4.8/qt-tools-plugandpaint-example.html

The class also has a virtual destructor. Interface classes usually don't need such a destructor (because it would make little sense to delete the object that implements the interface through a pointer to the interface), but some compilers emit a warning for classes that declare virtual functions but no virtual destructor. We provide the destructor to keep these compilers happy.

kshegunov

@Joel-Bodenmann

The reason I added those virtual destructors there is because Qt documenation adviced me to do so

Right, it's not an error by any means. However, I advise not using substandard compilers instead. ;)