Unreproducible crashes

[letsfindaway]

I still have unreproducible crashes, one during startup, the next when loading a document, and one when terminating the application.

I got a useful stack trace here, during startup: All variables look ok, but malloc fails, so I assume some memory corruption happened already earlier.

[letsfindaway]

Another crash on exit: Here the UBBoardController was already deleted, this was 0x0, while a UBGraphicScene was still working.

The call is initiated by the QUndoStack. Don't know why, but this does not matter, it just can happen. It does not happen on the main thread, but on thread #48 named QThread. I assume that this thread is a consequence of UBPersistenceManager calling QCoreApplication::processEvents() in its destructor.

I assume that this problem already existed before all the shared_ptr work, but happened rarely and nobody cared as it was on exit.

What can we do here?

• Add a guard in UBGraphicsScene::updateSelectionFrame() check for the existence of the board controller? This would prevent this single problem, but not assure that there are many other similar problems. There are 383 occurrences of UBApplication::boardController, and none of them is guarded. They all safely assume that the board controller is already instantiated when they start, and they don't care (and should not have to care) about shutdown.
• Delete the board controller later? Currently this is done in UBApplication::cleanup(), which is called from main when the exec() call returns, i.e. before the application destructor is called. Should this better be in the destructor? But note that the board controller also has pointers to other objects, and it should not be deleted after them.
• Delete the QUndoStack before all the other objects? This would be the easiest and I would prefer this solution.

Edit: probably the fix to the next one also fixes this, because it avoids dangling scenes which just keep themselves alive with a circular reference.

[letsfindaway]

And not a crash, but I still got an

Release of profile requested but WebEnginePage still not deleted. Expect troubles !

on exit :disappointed: Why are still not all scenes deleted?

This is even reproducible!

• Open a document containing a widget.
• Then open another document without widget.
• Then close OpenBoard.

Somebody still has a reference to the scene of the first document and I don't know who.

More analysis: probably it is easy. The widget itself has a reference to the Uniboard API and this again has a shared_ptr to the active scene. So we have a circular dependency and have to replace that pointer by a weak_ptr.

[letsfindaway]

Still testing, but I have not seen any crashes since then. Probably they have all been caused by dangling scenes?

[letsfindaway]

The thread problem mentioned here https://github.com/letsfindaway/OpenBoard/issues/131#issuecomment-1549558194 still persists. After some more considerations and sleeping a night over the problem I now have the following reasoning:

We move the copied scene itself to the persistence worker thread. But the scene contains items, and some of them are also QObjects. Those are not moved, but they are deleted in the persistence worker thread. This causes the same issue as with the scene itself.

For solution we could do two things:

• Move all other QObjects also to the worker thread.
• Delete the copied scene in the main thread, as it was originally.

I tend to the second, as you might easily forget moving an object.

After some experiments it turned out that using shared_ptr here makes things more complicated. In the slot UBPersistenceManager::onScenePersisted we cannot safely delete the scene by removing it from a list of shared_ptr, because another shared_ptr is still holding a reference to the object from the worker thread. So the scene ends up to be deleted from the worker, just as it was. This would be much easier to implement and to understand when we would not use shared_ptr for the copied scenes.

[letsfindaway]

My next commit and PR contains now the following changes (long explanation here, shorter in PR):

Threading

As explained above, it is difficult to move all related objects to the target thread. When an object is wrapped in a shared_ptr, it is also difficult to determine when the object will be released, and especially to determine whether it will be on the source thread or the target thread.

It would be counter-intuitive to tweak the program to guarantee that it happens exactly on one specific thread. shared_ptr are used if you do not want to worry about the location of destruction. So I finally come to the conclusion that shared_ptr should not be transferred between threads.

Instead I reverted the UBPersistenceWorker to use raw pointers again and to send a signal containing that raw pointer when finished. The UBPersistenceManager keeps a list of unfinished scene saves holding the shared_ptrs to the scenes, and thus guaranteeing that the objects live during the time they are saved. When the slot is invokes indication that saving is complete, then the pointer for that scene is removed from the list. This will delete the scene on the originating thread, where it was also created.

Circular references

To find more circular references, I scanned all header files for variables of type shared_ptr<UBGraphicsScene> and checked, whether they are correctly released. Especially subclasses of QGraphicsItem may be problematic, because they are also contained in a scene.

I found problematic code in UBGraphicsStroke. I replaced the type by a weak_ptr and locked that to access the object.

Another place was UBGraphicsCache. This class keeps a static map of instances, indexed by scene. As long as scenes are in this map, they will not be deleted. And as long as they are not deleted, they will not be removed from the cache. To break this I moved the reference to the cache directly to the scene, making this map unnecessary.

Unreleased references

I also found a possibly unreleased reference to a scene in UBPodcastController. Here we have a variable mSourceScene pointing to the current scene. This variable is not reset when the recording stops and might therefore inhibit to release that scene.

Here I just added a line to reset the pointer when recording stops.

A second unreleased referenced is in the UBDesktopAnnotationController created by the UBApplicationController. This object is not released when terminating the application, but it has a shared_ptr to a scene. Here I just added a line deleting this object when the UBApplicationController is deleted.

Scene for UBWidgetUniboardAPI

The UBWidgetUniboardAPI used by UBGraphicsWidget needs a pointer to the current scene. Until now, they listed to the activeSceneChanged event and set the scene according to that. This means however that when flipping through pages all widgets on all pages adjust their scene pointers. This is unproblematic, as you can only interact with widgets on the current scene, but it is also unnecessary.

I changed the behavior. The scene is now set once when the widget is created and will never change later on.

Other changes

In the UBGraphicsScene I also removed some #includes which are never used. There are much more elsewhere, but I just started here do make this cleanup, as the clang tools nicely direct me to such code.

Lessons learnt

This work on moving to the use of shared_ptr was not as straight forward as expected. Let me summarize what I have learnt:

• Take care of circular references. When using a shared_ptr as a member variable always ask yourself whether the pointed-to object could point back in some way. If yes, you have to break the cycle somewhere. A weak_ptr is the tool for that.
• Never pass a shared_ptr to a subclass of QObject to another thread. Fullstop. Do not try to mitigate the occurring problems. Avoid them.
• Clear pointers you no longer need. When using plain pointers, you might have a member variable storing a pointer to an object which you currently work on. You don't have to worry about nullifying this pointer in the end. With shared_ptr, nullifying is essential to release the reference and to allow cleanup of the pointed-to object.
• Avoid storing a shared_ptr in static variables. They are deleted somewhere in the cleanup procedure of C++, but that's not under your control and probably too late.