Skip to content

Fix crash while loading hunks #944

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Murmele wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Fix crash while loading hunks #944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b05291a
Fix crash while loading hunks
Murmele Feb 23, 2026
ff1e8f6
Format
Murmele Feb 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/ui/DiffView/DiffView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,12 +374,37 @@ bool DiffView::canFetchMore() {
mFiles.size() < mDiffTreeModel->fileCount(dtw->selectedIndex());
}

class Guard {
Copy link
Contributor

@AHSauge AHSauge Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not observed this crash, but just to offer some thoughts based on the code changes. Presumably this involves two or more threads, which means you need to ensure data is synchronised between them (specifically synchronise L1 and L2 cache that might not be shared). The C++ way of solving that would be std::atomic_bool, which has load and store that will specify how to deal with the inter-thread synchronisation. There's more about this in https://en.cppreference.com/w/cpp/atomic/atomic.html and https://en.cppreference.com/w/cpp/atomic/memory_order.html

The second thing here is that I'd suggest not to define this class. I believe you can achieve the same effect by using std::mutex and instead of using a std::lock_guard or similar mechanisms, just use try_lock. If that fails, you can do mCancelFetchMore.store(false) (maybe with memory order specifically set), and post QApplication::processEvents you can unlock and check with mCancelFetchMore.load() (which also might need a memory order set)

Copy link
Owner Author

@Murmele Murmele Mar 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought at the beginning as well that two or more threads are involved, but there aren't.
When I call QApplication::processEvents() the current loop will be interrupted and mouse events are handled. This mouse event triggeres then again a new diff (when clicking on another element in the commitlist) which triggers the fetch.
The problem is that you cannot wait until the previous fetch is finished because it will never, because everything runs in a single thread so we have to return to go on in the fetch loop.

This is reproducable when you have a large number of hunks in a diff which must be loaded.

The Guard was just there to release the lock after the function returns so it cannot be forgotten.

But this actually does not solve the problem yet so I have to further investigate

public:
Guard(bool &variable, bool initValue)
: m_variable(variable), m_initValue(initValue) {
m_variable = m_initValue;
// qDebug() << "Guarding variable" << m_variable;
}
~Guard() {
m_variable = !m_initValue;
// qDebug() << "Releasing variable" << m_variable;
}

private:
bool &m_variable;
bool m_initValue;
};

/*!
* \brief DiffView::fetchMore
* Fetch maxNewFiles more patches
* use a while loop with canFetchMore() to get all
*/
void DiffView::fetchMore(int fetchWidgets) {
if (mFetchMoreInProgress) {
mCancelFetchMore = true;
// We cannot wait here because then processEvents() from below will never
// return
return;
}
auto g = Guard(mFetchMoreInProgress, true);

QVBoxLayout *layout = static_cast<QVBoxLayout *>(widget()->layout());

// Add widgets.
Expand All @@ -403,6 +428,9 @@ void DiffView::fetchMore(int fetchWidgets) {

// Load hunk(s) and update scrollbar
QApplication::processEvents();
if (mCancelFetchMore) {
return;
}

// Running the eventloop may trigger a view refresh
if (mFiles.isEmpty())
Expand Down
3 changes: 3 additions & 0 deletions src/ui/DiffView/DiffView.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,9 @@ class DiffView : public QScrollArea, public EditorProvider {
DiffTreeModel *mDiffTreeModel{nullptr};
QWidget *mParent{nullptr};
QVBoxLayout *mFileWidgetLayout{nullptr};

bool mFetchMoreInProgress{false};
bool mCancelFetchMore{false};
};

#endif
Loading