Fix crash in application shutdown (alternate fix)
ClosedPublic

Authored by davidedmundson on Oct 21 2016, 12:13 AM.

Details

Summary

I didn't like the last fix, here's another one, which is maybe a bit
neater?

We have a SourceModel that inherits from AbastractModel
AbastractModel inherits from both QObject and Ref.

When we call the destructor of Ref, that kills the
Pulseaudio::context that emits that the default source has changed during it's teardown.

Because the QObject destructor hasn't run yet, our signals are still
attached; however they refer to data from SourceModel which we've
already run the destructor on - hence crash.

This patch does the ref counting in an alternate way, destroying
the PA Context after our model has disconnected all the
signals, like it would be if the context was a child of the model.

Test Plan

ran kquitapp5 plasmashell a lot, everything still seems fine.

I couldn't recreate the original crash, it requires
multiple outputs being deleted in the wrong order, which I
don't happen to have.

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidedmundson retitled this revision from to Fix crash in application shutdown (alternate fix).
davidedmundson updated this object.
davidedmundson edited the test plan for this revision. (Show Details)
davidedmundson added a reviewer: Plasma.
davidedmundson added a project: Plasma.
davidedmundson added a subscriber: plasma-devel.
drosca requested changes to this revision.Oct 21 2016, 8:28 AM
drosca added a reviewer: drosca.
drosca added a subscriber: drosca.

Sorry, I've noticed this one only after accepting the previous.
This looks better, as long as it works without regressions. So, let's push the first fix to Plasma/5.8 and this one to master?

src/pulseaudio.cpp
42

Either don't create c variable here (as it is used only once), or also catch it and use it from the lambda.

100

Context *AbstractModel ...

This revision now requires changes to proceed.Oct 21 2016, 8:28 AM
In D3124#58026, @drosca wrote:

Sorry, I've noticed this one only after accepting the previous.
This looks better, as long as it works without regressions. So, let's push the first fix to Plasma/5.8 and this one to master?

I'll do that if you want, but I'd rather not as it means we have something on 5.8 that isn't being tested by anyone.

davidedmundson edited edge metadata.

Minor fixes

drosca accepted this revision.Oct 21 2016, 1:45 PM
drosca edited edge metadata.
In D3124#58026, @drosca wrote:

Sorry, I've noticed this one only after accepting the previous.
This looks better, as long as it works without regressions. So, let's push the first fix to Plasma/5.8 and this one to master?

I'll do that if you want, but I'd rather not as it means we have something on 5.8 that isn't being tested by anyone.

Alright, let's go only with this one.

This revision is now accepted and ready to land.Oct 21 2016, 1:45 PM
davidedmundson marked 2 inline comments as done.Oct 21 2016, 1:46 PM
This revision was automatically updated to reflect the committed changes.