Fix inconsistent mutex unlocking due to double slot execution

[phoerious]

Description

Fixes crash on Windows due to double mutex unlock. Resolves #1561.

Motivation and context

On Windows, the rejected() slot of the Auto-Type selection dialog was called twice on screen lock, resulting in a double mutex unlock. I have no idea why it's doing that (perhaps a bug introduced in Qt 5.10), but this patch works around it by making sure the mutex is locked before we try to unlock it.

In addition, I added another mutex for the actual auto-typing (not just the dialog) to ensure we are never auto-typing two sequences at the same time (not that this was an issue, but when we're already at it...)

How has this been tested?

Manually on Windows. I could not reproduce the crash anymore after applying the patch.

Types of changes

• ✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

• ✅ I have read the CONTRIBUTING document. [REQUIRED]
• ✅ My code follows the code style of this project. [REQUIRED]
• ✅ All new and existing tests passed. [REQUIRED]
• ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

[TheZ3ro]

In addition, I added another mutex for the actual auto-typing (not just the dialog) to ensure we are never auto-typing two sequences at the same time (not that this was an issue, but when we're already at it...)

This was already implemented since the m_inAutoType was locked before the executeAutoTypeActions and unlocked after.

If the problem was a double mutex unlock, the easy fix (tryLocking before unlocking) is just a one line fix:

void AutoType::resetInAutoType()
{
    Q_UNUSED(m_inAutoType.tryLock());
    m_inAutoType.unlock();

    emit autotypeRejected();
}

Inserting another mutex is useless, the first one already take care about everything

[phoerious]

Yes, for global Auto-Type, the outer mutex also covers the inner one, but non-global Auto-Type should have a mutex as well.
The global one is really just for ensuring that we don't open two or more global Auto-Type dialogs at the same time. The inner one is for ensuring that we never auto-type two sequences at the same time (no matter if global or not).

Previously, performAutoType used the same mutex, but it makes a lot more sense to keep it local to the function doing the actual work, which prevents a lot of errors. By splitting the logic into two distinct mutexes, I can keep one entirely inside one function and spread the other one over as few as possible (i.e., one that locks and two which unlock, which is far from ideal, but better than having all sorts of functions taking care of it). I also don't mix our two use cases, which are preventing double dialogs and preventing double Auto-Type, making the semantics behind the mutexes a lot more transparent.

[TheZ3ro]

I agree that the double use case mix things up a little bit.

The old single-mutex approach already locked multiple global-autotype/single-autotype and mixed global - single autotype.
Having a mutex on the executeAutotypeActions (in the function itself or outside it) is enough since both scenario uses that function.
Having 2 mutex seems redundant to me when a single one does the job in both cases.

Double dialog is file if the single mutex locks/unlocks fine

[phoerious]

The problem with having only a single mutex is that it's quite easy to produce deadlocks by re-entrance and we cannot set the mutex to be recursive, because we are working only in a single thread.

[TheZ3ro]

So it's fine by me

[phoerious]

Then please approve. 😉

[TheZ3ro]

I was waiting for @droidmonkey since he explicitly self requested a review 😅

[droidmonkey]

Lol sorry I didnt see the core developers already in place. I wanted to run this on my machine to test that it fixed the issue, i trust you guys