KModifierKeyInfo: we are sharing the internal implementation
ClosedPublic

Authored by apol on Jul 3 2019, 3:14 PM.

Details

Summary

Use data types that will only destroy it when necessary.

Diff Detail

Repository
R273 KGuiAddons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.Jul 3 2019, 3:14 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 3 2019, 3:14 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
apol requested review of this revision.Jul 3 2019, 3:14 PM

I don't think any of these changes are ABI-compatible?

I don't understand the bug being fixed.

We have:

Q_DISABLE_COPY(KModifierKeyInfo)

So why does KModifierKeyInfo::Private need to be shared?

So why does KModifierKeyInfo::Private need to be shared?

It needs to be refcounted. In 0fe2990dbad992a4925a7b7bee09b1cdfbe5a7a7 a plugin infrastructure was introduced. Unbeknownst to us the instances are shared internally by Qt.

KModifierKeyInfoProvider* createProvider()
{
    QPluginLoader loader(QStringLiteral("kf5/kguiaddons/kmodifierkey/kmodifierkey_")+qGuiApp->platformName());
    auto instance = dynamic_cast<KModifierKeyInfoProvider*>(loader.instance());
    if (instance)
        return instance;
...
 
 KModifierKeyInfo::KModifierKeyInfo(QObject *parent)
    : QObject(parent), p(createProvider())

The instance returned is the same every time despite the QPluginLoader being on the stack.
Now the destructor of KModifierKeyInfo cleans up p and if you have two instances of KModifierKeyInfo, which Kile does for instance, it crashes on double delete.

apol added a comment.Jul 4 2019, 9:57 AM

I don't think any of these changes are ABI-compatible?

Yes it is, we're just modifying the size of a private class.

Also this was never released yet.

apol added a comment.Jul 5 2019, 5:45 PM

Landing because it works and we want it in before the freeze.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 5 2019, 5:46 PM
This revision was automatically updated to reflect the committed changes.