mrview: Proposed mitigation for segfault on -overlay.load

[Lestropie]

Closes #1506.

Gave a verbose description of the fault to Claude, and this is what it came up with.
Have not been able to verify that it fixes the problem because I've not been successful in reproducing the problem; maybe something has been rectified in the Qt internals. So might benefit from a couple of people with different platforms testing.

[daljit46]

I investigated the issue in #1506 some time ago. I think the solution proposed by the AI is very hacky and honestly not satisfactory. It's effectively proposing a very ugly band-aid for what really needs a deeper architectural change IMO, without even considering that the changes proposed don't fully address the problem.
The proposed patch tries to prevent nested event processing. For example, we have this piece of code:

progress_dialog->setValue (p.value());
if (!inside_process_events) {
  inside_process_events = true;
  qApp->processEvents();
  inside_process_events = false;
}

One might think that this prevents a recursive call of Qt's event processing, but it doesn't. This is because QProgressDialog::setValue() itself calls processEvents() for modal dialogs (it might be worth considering whether you really need modal dialogs in this case). To get around this problem, the guard would need to be active before calling setValue(), not after it:

if (!inside_process_events) {
  inside_process_events = true;
  progress_dialog->setValue (p.value());
  qApp->processEvents();
  inside_process_events = false;
}

Furthermore, the code written as above is not exception safe. What happens if processEvents() throws an exception? The line inside_process_events = false; might never be called.
A better way to do this would be to use QScopedValueRollback, which is what Qt internally uses in QProgressDialog.

Even addressing these issues, I think the direction proposed is here isn't really idiomatic Qt/C++ code. The problem that needs addressing is we're doing sustained GL work while manually pumping the GUI event loop (which in turn triggers callbacks that mess with state that was touched by the function from processEvents() was called from).
A good solution to this problem is to make the texture upload work in chunks rather than uploading the whole 3D texture synchronously. Currently, the OpenGL code appears to upload the texture in one go. That means the GUI thread is occupied for the duration of the upload. A better design would be to split the upload into smaller units and let the normal Qt event loop drive progress in between. Something like this:

void uploadNextChunk() 
{
uploadSlice();
progress->setValue(...);
// Check if we need to do upload more first
QTimer::singleShot(0, this, &SomeUploadHandler::uploadNextChunk);
}

This would avoid the need of manually calling qApp->processEvents().
A more radical solution would be defer the uploading of textures into a background OpenGL thread, but that is a bit more complicated as it would require you to use multiple OpenGL contexts (something that I have dealt with in the past and don't really recommend as it can lead to subtle issues).

On this note, I would like to add that, while I completely understand the temptation of delegating these very annoying to debug issues to AI agents, I do think that one needs to be careful with the solutions they propose (at least in their current state). This is exactly the sort of issue where a superficially plausible fix can look convincing while not actually addressing the underlying failure mode.

[Lestropie]

Under no illusion that this one is a panacea. Just couldn't reproduce the fault after generating the candidate, so putting it here out of my work directory so that if I or anyone else encounters the fault specifically with -overlay.load it can be tested.

I think there's general agreement that there's considerable fragility around the Qt code. But it's very difficult to fully wrap one's head around unless it becomes the sole focus for a period of time. Deeper refactoring would be great but not going to happen for 3.1.