Differential D27809

decorationbridge: Fix crash on plasma mobile
ClosedPublic
Actions

Authored by apol on Mar 3 2020, 6:08 PM.

Details

Reviewers
zzag
Group Reviewers
KWin
Commits
R108:17e6bd8587cd: decorationbridge: Fix crash on plasma mobile
Summary

For some reason there m_settings is null there, don't crash in this case.
The code definitely contemplates the possibility since it's null by default and on some DecorationBridge::reconfigure() paths.

Diff Detail

Repository
R108 KWin
Branch
fix-crash-supportInfo
Lint
Lint ErrorsExcuse: boop
SeverityLocationCodeMessage
Errordecorations/decorationbridge.cpp:62CppcheckunknownMacro
Unit
No Unit Test Coverage
Build Status
Buildable 23218
Build 23236: arc lint + arc unit
apol created this revision.Mar 3 2020, 6:08 PM
Restricted Application added a project: KWin. · View Herald TranscriptMar 3 2020, 6:08 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
apol requested review of this revision.Mar 3 2020, 6:08 PM
anthonyfieroni added a subscriber: anthonyfieroni.Mar 3 2020, 6:52 PM
anthonyfieroni added inline comments.
decorations/decorationbridge.cpp
123 ↗(On Diff #76871)

It does not have settings?

287 ↗(On Diff #76871)

Is there problem here?

apol added inline comments.Mar 3 2020, 7:54 PM
decorations/decorationbridge.cpp
123 ↗(On Diff #76871)

I guess it's never called?

287 ↗(On Diff #76871)

I can imagine this never reaches since there's no decoration on the phone.

zzag added a subscriber: zzag.Mar 3 2020, 9:29 PM
zzag added inline comments.
decorations/decorationbridge.cpp
115 ↗(On Diff #76871)

@apol Is NoPlugin set on PlaMo?

zzag added inline comments.Mar 3 2020, 9:34 PM
decorations/decorationbridge.cpp
314 ↗(On Diff #76871)

I see a potential crash when decorations are disabled. Perhaps we need to check m_noPlugin, e.g.

if (m_noPlugin) {
    b.append(QStringLiteral("Decorations are disabled"));
} else {
    b.append(QStringLiteral("Plugin: %1\n").arg(m_plugin));
    b.append(QStringLiteral("Theme: %1\n").arg(m_theme));
    b.append(QStringLiteral("Plugin recommends border size: %1\n").arg(m_recommendedBorderSize.isNull() ? "No" : m_recommendedBorderSize));
    b.append(QStringLiteral("Blur: %1\n").arg(m_blur));
    const QMetaObject *metaOptions = m_settings->metaObject();
    for (int i=0; i<metaOptions->propertyCount(); ++i) {
        const QMetaProperty property = metaOptions->property(i);
        if (QLatin1String(property.name()) == QLatin1String("objectName")) {
            continue;
        }
        b.append(QStringLiteral("%1: %2\n").arg(property.name()).arg(settingsProperty(m_settings->property(property.name()))));
    }
}
apol updated this revision to Diff 76889.Mar 3 2020, 10:24 PM

Do the same fix in a different way

apol marked an inline comment as done.Mar 3 2020, 10:36 PM
apol marked an inline comment as done.
zzag added a comment.Mar 4 2020, 6:45 AM

Hmm, I see no difference between the previous diff and the current one.

apol updated this revision to Diff 76924.Mar 4 2020, 11:35 AM

somehow arc ignored my last changes :/

zzag accepted this revision.Mar 4 2020, 12:19 PM

Thanks.

decorations/decorationbridge.cpp
319–326 ↗(On Diff #76871)

Unrelated whitespace change.

This revision is now accepted and ready to land.Mar 4 2020, 12:19 PM
Closed by commit R108:17e6bd8587cd: decorationbridge: Fix crash on plasma mobile (authored by apol). · Explain WhyMar 4 2020, 6:59 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mdecorations/decorationbridge.cpp (15 lines)

Diff 76889

View Options

decorations/decorationbridge.cpp

Loading...