Crash when closing application

SGaist

Hi,

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

kshegunov

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. ;)