Finding GUI Event-Handling Crashes

SGaist

From the doc:

Sometimes it is desirable to make someone else paint on an unusual QPaintDevice. QPainter supports a static function to do this, setRedirected().

Warning: When the paintdevice is a widget, QPainter can only be used inside a paintEvent() function or in a function called by paintEvent().

From a quick look at the Qt sources, I'd say no.
Misbehaving code ;) More seriously, 0 doesn't mean it was delete, you may not have a redirected object and that would be normal e.g. an QOpenGLWindow which doesn't have the current context..

JohnF-n-Kansas

I had seen the setRedirected() documentation and searched the source code (non-Qt sources, i.e. the code written here) to see if the app was intentionally doing this (and in a non-safe way), but couldn't find it, and didn't see any attempted chicanery on the part of the app to try to tweak the output device. That said I'll set a breakpoint in the setRedirected() entrypoint to see who's calling it.
Thank you for checking that!
Right, however, the 0xfdfd and 0xabababab in the member variables which looked like possibly memory clearing constants...e.g., I believe the C++ Windows runtime memory management code sets unused variables to strings of 0xcd, but if those values didn't jump out at anyone then my presumption about deallocated-memory-filling was wrong. Interestingly yesterday afternoon I did notice in what appeared to be adjacent memory those same values, so maybe there is some kind of unintended memory corruption or maybe even a pointer++ that I haven't seen. My entire premise was that the original author decided it would be a good idea to use Qt, or maybe had an intellectual curiosity about Qt, and thus cobbled together a chunk of code that "seems to work" but violates some of the Qt "no no's" in regards to stack variables, heap variables, or something along those lines.

I really appreciate your responses. Thank you for taking time to do so!

SGaist

One thing I would do, since you can't rewrite everything is to first cleanup the model related code. If the model is properly implemented, it should trigger the update to the view without additional help even if it's modified from somewhere else it the code.

That might help clear the issue you are having.

JohnF-n-Kansas

Thank you. I definitely believe that would help even if all it did was reduce the fog factor. By the same token, because of the size of the app and the way it's been implemented, correcting flow and components to be MVC/Delegate-pattern compliant is a bigger job than the return on the investment to do that. Users like the Find dialog and the functionality it provides. They just want it to stop crashing. If I could figure out a way to quickly/inexpensively rewrite or re-architect I certainly would, but I just can't get there painlessly.

So I'm hoping to find (or stumble over) badly-written code using pointers or stack space incorrectly, or maybe code that simply needs to be wrapped in a mutex. Since this beast works most of the time, I'm inclined to think the crashes occur because of an race condition or bad memory or thread misuse; I'm hoping I can find that place and make a small tweak that circumvents the issue.

A couple of general usage questions that may help.

Is there anything wrong in declaring a CWnd-derived class member variable as a QPointer variable declared as:

QPointer<DlgFindObjectU> m_pFindDialog;

The View class, which is where the main app displays objects, is not a Qt class but rather a CView class that's derived off an MFC CWnd class. There's a method that does the following which is called from code scattered all over and around the application:

if ( ! m_pFindDialog.isNull() )
{
    m_pFindDialog->Restart();
    ...

Note that this code could be getting called from threads other than the main thread.

When Find is clicked from the main window, a new instance is created as follows -- from purely a Qt standpoint, does the following look okay:

// Use a new "win" to prevent "Debug Assertion Failed!" in the end.
QWinWidget *win = new QWinWidget(theApp.GetMainWnd());
win->showCentered();
m_pFindDialog = new DlgFindObjectU( this, win );
m_pFindDialog->setAttribute(Qt::WA_DeleteOnClose);
m_pFindDialog->show();
m_pFindDialog->Restart();

Note that the creation of the instance of the Qt Find dialog is happening on the Main (GUI) thread, but it happens each time you click the "Find" menu option. The variable named theApp is a staticly-declared instance of the main application which is derived from CWinApp and utlimately becomes the "parent" object in the QDialog constructor. The DlgFindObjectU is derived from QDialog.

is it acceptable to allocate the Find dialog as a member variable in a CWnd object, then fire off calls like the following that could at times possibly originate from background threads in the CWnd-derived class:

m_pFindDialog->Restart();

which then calls:

void DlgFindByObjectU::Restart()
{
    ScheduleDialogRestart();
}

which then calls the following code that gets generated in the moc_DlgFindByObjectU.cpp file:

// SIGNAL 1
void DlgFindByObjectNameU::ScheduleDialogRestart()
{
    QMetaObject::activate(this, &staticMetaObject, 1, 0);
}

Is that kind of usage okay? Or should it move the request over to the GUI thread, akin to the way Windows Forms apps do an Invoke() on Windows?

I searched to determine if there's anything problematic with what's being done, but I couldn't find anything obvious. If I did then I missed it when reading.

Thanks for any comments you have time for and are willing to provide!

SGaist

Warning: I haven't use the QtWinMigrate.

That said, the way it is used looks a bit weird.

AFAIK, there's nothing wrong in having a QPointer member of a class however, the dialog has the WA_DeleteOnClose attribute set so I fail to see the usefulness of that member.
From a quick look at the example from the QtWinMigrate module, it looks a bit fishy.
That restart call doesn't look clean at all. By the way, is it really activate ? I can't find it in Qt 4 nor Qt 5 QMetaObject doc.and I don't have a Qt 3 handy to check.

Everything involving a GUI related activity should be called on the GUI thread however model related stuff can be multithreaded as long as you put in place the proper protections.

I can understand the motivation of creating that dialog only once but the implementation doesn't look to be doing that.

I'd rather start by cleaning up the dialog handling itself using a dummy QDialog just to ensure that showing, dismissing and showing again works correctly. Then I'd gradually add back the searching functionality.

JohnF-n-Kansas

I know this is old, but I had to park this and just came back to it recently.

I got the okay to do some remodeling and I took your advice to make some major changes. I've reduced the emission of UI-updates, and I'm now using a QThread to get all the items and sort them and then signal back to the UI thread when complete.

Everything is working really well except for one quirky issue that I'm guessing is due to view/model violations. I'm not deeply familiar with Qt's rules of engagement on this, but here's where it seems to be brittle.

The underlying container structure is a std::vector. QDialog containing a QTableView in which these items are shown flies as a non-model child window with the main app. The main app shows a graphical map with features shown as rows in the QDialog. The user can select items on the map and "Unload" them.

The Unload operation invokes code in the QDialog implementation which clears the underlying vector in the model like this:

m_model->beginResetModel();
m_vector.clear();
m_model->endResetModel();

The m_model member variable is defined like this:

QPointer<MapFeatureTableModel> m_model;

MapFeatureTableModel is declared as:

class MapFeatureTableModel : public QAbstractTableModel

The member variables of the MapFeatureTableModel are:

std::vector<FeatureObj*>* m_pVector;
QTableView* m_pTable;

The m_pVector is a pointer to the m_vector item that gets cleared in the scenario I stated.

I'm not sure this relationship is setup correctly, but it's what was there when I started.

In any event, I believe the beginResetModel(); endResetModel(); guardians are correct, but is it acceptable to clear the vector that the model has a pointer to? Should the MapFeatureTableModel class house the actual vector and not a pointer, and then use some kind of "clear()" invocation betwixt the beginResetModel() / endResetModel() ?

My suspicion is that even though the updates occur inside the begin/end pair, modifying the vector in this way is possibly leaving the model in an undefined state.

I tried calling

m_model->clear();

inside the begin/end, but this generates a compiler error that the clear() method is not defined for a QAbstractTableModel, and after trying several incantations including dynamic_cast's and still getting the compiler errors I aborted the mission.

If it helps to illuminate, the version of Qt (moc.exe) I see in the moc_* files is:

Created by: The Qt Meta Object Compiler version 67 (Qt 5.2.1)

Thanks for any insights or suggestions anyone might throw out there. In the interim I'm going to continue to see if I can find exactly why this call causes issues.

Oh, one other question, should this be using a QVector instead of std::vector?

SGaist

Why is m_pVector a pointer ?

You should first delete all objects from the vector before calling clear.

JohnF-n-Kansas

@SGaist I know, it's odd. The vector instance resides in the backing code for the QDialog. From what I can gather about the history of this code it was originally written as pure MFC then "crossbred" with Qt in 2010 at which point it was a single class, then refactored into two classes in 2013 and created the MapFeatureTableModel class. The engineer who did this was not the original author, so I suppose he wanted to impose the View/Model pattern and maybe didn't fully understand how to do that...not sure.

Do you think I should move the std::vector into the MapFeatureTableModel class and access it through the data() interface? Would that add stability to the Qt framework?

The architecture of this thing is akin to "Frankenstein's Monster" IMHO, and while I've made some (what I believe to be) good changes in the architecture for acquiring the data, I haven't done anything, sans adding a QPointer<> here or there, to reconfigure the class hierarchy, ownership, chain of responsibility, etc. At the same time, I'm learning Qt as I go, so I'm reluctant to impose my dangerously-ignorant level of Qt "best practices" into the mix.

If the model is where the container should live, but that's predominately a "the correct way to do it" and is unlikely to improve stability, I'm not keen on changing it. If it would make it less brittle, I'm all for it.

Last side notes:

The objects in the vector are not owned by this code -- they're pointers to objects owned by the main app which manages the map. That's why the "clear"-like logic simply empties the vector of pointers rather than looping through and issuing deletes on them.
To be clear (slight pun intended), I can't call "clear()" because it seems to not be part of the inheritance chain of a QAbstractTableModel. I found this stack-overflow article that describes the wall I've hit:

https://stackoverflow.com/questions/22917170/qt-clearing-qtableviews-contents

One answer says that QAbstractItemModel does not contain the clear() method, and QAbstractTableModel derives from that. The poster goes on to say "You should call clear method of your model". Does this suggest I should create a "clear()" method in which the "m_vector.clear()" would be moved and call that instead of clearing the vector inline? If that were the case, why wouldn't the begin/endResetModel() calls reside there? Guessing it's not the recommended pattern to follow.

My apologies for what are surely ridiculously pedestrian questions. I'm well versed in many systems, but not Qt, and once I'm done with this fix I don't anticipate developing in Qt going forward (we are not a Qt shop -- Qt was used as a means to support unicode because MFC, being the T-Rex that it is, does not). As a result, I'm trying to learn just enough Qt to fix this without causing new problems for the next Qt-naiveté to attempt to fix, if that makes sense.

Thanks for the suggestions, guidance, or commentary you've made and any you choose to make now. Gratefully received on this end to be sure!

SGaist

Not a question of Qt stability, but moving the all the data handling inside the model will allow you to control it's existence in only one place with a clean interface (single responsibility) thus you can then share the model with all the classes that needs to access these data.

Note that you don't need a view to interact with a model.

JohnF-n-Kansas

@SGaist That would limit scope and definitely makes sense. Thank you. I am clueless as to why this structured as it is, but you'll have to take my word for it that this is not the only thing going on with this code that makes my brain hurt. At least there are no goto's or setjmp/longjmp's...

I'm also presuming "clear()" is not a method of the Model framework but simply a pseudo-code placeholder for whatever is apropos for the container anchored in the Model, true? When I'd read this "http://doc.qt.io/qt-5/qabstractitemmodel-obsolete.html#reset" about reset() being deprecated in Qt 5.x and how one should wrap the "myData.clear()" with the begin/end-ResetModel() calls, my initial thought was that "clear()" was part of Qt's Model, but on further review (and a search of the header files) it appears my initial understand was wrong. So if I'm still wrong, don't hesitate to point that out.

My plan now will be to ll encapsulate the vector inside the Model. Access will be done through calls to the Model. That makes sense, and I'll see if that in conjunction with tighter controls helps to stabilize behavior.

One last question: if one wishes to relinquish control to the Qt event-dispatch loop:
A) Is it acceptable to do such a thing?
B) If the answer to A is "yes" does it matter whether the app calls "qApp->processEvents();" vs "QApplication::processEvents();"? I tried to find out and can't find any caveats -- do you know if there are implications of using one over the other?

In case you're wondering, my reason for doing this is to set a data-acquisition-is-active flag just before starting the QTread, and then to use that flag to disable UI controls. The controls' event handlers are what launch the background thread. Once the thread begins pulling in data, I want to prevent the users from changing the request while the previous one is in flight.

I set the flag and the "UI refresh" code interprets the flag to enable/disable the controls accordingly, but I found if I don't relinquish control after setting the flag and emitting a UI-refresh signal, a window -- albeit short -- exists where the thread is running and the controls are enabled. When I call "processEvents()" the UI refresh bubbles off the queue and gets processed.

As an alternative, I could check the flag within every "OnClicked", "OnTextChanged", yadda, event handler, but that's noisy and a lot of extra code. If there's a real-time way to immediately disable a control this is a place where that would be optimal, but it seems that would be contrary to the event-handling architecture around with Qt is built. <shrug> True/False?

Thanks for any thoughts, suggests, criticisms or observations put forth. All will be considered thoughtfully and are appreciated and valued.

JohnF-n-Kansas

I want to add to my previous post that the UI-refresh signal I'm emitting uses Qt::AutoConnection on the connect() call...I'm ONLY using this right after I set the thread-active flag...all other UI-refresh requests emit a similar (ends up in the same place) but different signal that is connect()'d via Qt::QueuedConnection.

I thought it was possibly an important element that I'd failed to mention. Even with Qt::AutoConnection it seems the request is being deferred for later processing.

SGaist

Indeed, clear is up to you to implement if it makes sense for your model.

A) There should be no need to force the event processing. It looks like something is blocking the event loop if you need to call that.
B) qApp is the instance of the QApplication but processEvent is static so it's rather a question of taste in this case. AFAIK most Qt examples uses the application object (i.e. QSplashScreen documentation).

JohnF-n-Kansas

@SGaist 10/4. Thanks for your help!

JohnF-n-Kansas

Final update on this...unless I learn more about why my changes worked and believe it could help others...

Today I deployed a (seemingly) fixed version after almost two months' testing. I made many changes including most of those @SGaist had suggested -- thanks for the super helpful feedback and suggestions!

This was a difficult bug to fix. The program is a really, really large in-house application used by hundreds of people every day. Some of the source files were written in the late 90's and are still used today; it uses MFC; it uses the QMfcApp class; it uses Qt; it uses Boost; and I'm skeptical of the smooth integration of these frameworks into a cohesive unit. In fact, I'm not sure QMfcApp was intended to be a long-term solution...it looks more like a bridge for moving from MFC to Qt...

At any rate, I made several changes that led to a fix. First, I moved the vector into the Model rather than injecting a pointer to a vector anchored in the dialog as the original author had done. I also disable UI controls while the data is acquired, then disable updates ( setUpdatesEnabled(false) ) to the TableView and TableModel while major changes are made, remove events (removePostedEvents(Table, 0) & removePostedEvents(Model, 0)) before clearing the vector, and I changed the app from emitting a restart-event after N items are processed and then exited knowing that when the restart bubbles off the queue it can pickup where it left off, to simply processing every item and issuing a qApp->processEvents() periodically to keep the UI responsive. I created a state variable to control when the app is busy or idle.

I don't know if all of this is in line with the standard Qt rules of engagement, particularly disabling controls and flushing their event queues, but it has been working after hours and hours of testing. Before I disabled updates and removed enqueued pending update events, I would get intermittent update events for stale data. I thought the documentation said if you emptied your Model's container between a beginModelReset() and endModelReset() the framework would recognize that data requests were stale and would purge them. Either I misunderstood how this works, failed some other necessary step, or there's just a bug in Qt or our code or the commingling of both that allowed UI updates after the table was emptied.

If something illuminating happens that provides greater understanding as to why these changes "fixed" the crashes, I'll come back here and update this thread. But I do want to say thanks to @SGaist for your input and suggestions. It helped a lot.

john