Try to fix "Pinned Chrome disappears when all Chrome windows are closed"
ClosedPublic

Authored by hein on Jan 4 2017, 9:13 AM.

Details

Summary

TasksModel filters out matching launchers when a startup or window
appears. When they go, dataChanged in the launcher model is triggered
to pop them back in. This used to be in response to
rowsAboutToBeRemoved, with a Qt::QueuedConnection to try and make sure
that dataChanged is emitted after the row removal is processed (other-
wise the filter would still take). While I have not been able to
reproduce the bug well enough to test things, my guess is that this
must be a race condition - perhaps something is spinning the event
loop.

This patch collects the indices in the handler for sourceModel::
rowsAboutToBeRemoved and triggers dataChanged for them in the handler
for ourselves::rowsRemoved. Keeping model indices around is usually a
huge red flag. This makes the (currently solid) assumption that nothing
will change the launchers model inbetween the halves of this transaction.

BUG:365617

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
hein updated this revision to Diff 9692.Jan 4 2017, 9:13 AM
hein retitled this revision from to Try to fix "Pinned Chrome disappears when all Chrome windows are closed".
hein updated this object.
hein edited the test plan for this revision. (Show Details)
hein added a reviewer: davidedmundson.
Restricted Application added a project: Plasma. · View Herald TranscriptJan 4 2017, 9:13 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
hein added a comment.Jan 4 2017, 9:13 AM

This is sort of a brainstorm trolling for David's input~

mart accepted this revision.Jan 4 2017, 12:30 PM
mart added a reviewer: mart.
This revision is now accepted and ready to land.Jan 4 2017, 12:30 PM
davidedmundson edited edge metadata.Jan 4 2017, 1:20 PM

I have an idea what could be wrong. (though it's a guess from reading a tonne of code)

LauncherTaskModel is activity aware.

When I change activity I will cause my (filtered) GroupsModel to change
and cause launchertasksmodel to change in the *same event*

Therefore the indexes you save in connect(groupingProxyModel, &QAbstractItemModel::rowsAboutToBeRemoved, will be mangled by the time you call dataChanged, one event later.

If that's the issue, then this code will do exactly the same thing.

What might work is storing the launchers to update by AppID instead?

Side suggestion: Maybe it's worth attaching the ModelTest class to every model here for debug builds. It basically asserts if anything goes wrong a bit weird, which can really help get better feedback

hein added a comment.Jan 5 2017, 5:19 AM

LauncherTaskModel is activity aware.

This is a new 5.9 feature though and the bug got reported as far back as 5.7.

Side suggestion: Maybe it's worth attaching the ModelTest class to every model here for debug builds.

I used it during development, but might be useful to add back.

hein added a comment.Jan 5 2017, 6:04 AM

re storing app ids: I'm reluctant to do so because it means not using appsMatch() which the code tries hard to avoid -- the old libtm suffered from different parts of the codebase using different logic to match things.

hein updated this revision to Diff 9744.Jan 5 2017, 9:12 AM
hein edited edge metadata.

Eliminate reliance on QueuedConnection.

In the previous version of the patch we still relied on QueuedConnection
for the dataChanged invocation, because we were connecting to our own
rowsRemoved. Proxy models emit rowsRemoved in response to their source
model emitting aboutToBeRemoved (a row has to be removed from a proxy
before it is removed from the source, otherwise things dangle). The
dataChanged causes filterAcceptsRow to run, which checks the filterProxyModel
for window/startup rows. With a direct connection it would still have the
window, since it's just in "about to remove" stage, necessitating the
QueuedConnection.

Queueing things is Bad because it's hard to be sure what happens while the
event loop spins. In this version of the patch we collect the indices in
the source model's aboutToBeRemoved as previously, but then invoke
dataChanged as the filterProxyModel (the one filterAcceptsRow checks) emits
rowsRemoved. Now we can be sure filterAcceptsRow won't find the window in
the filterProxyModel and will let the 'changed' launcher through, completing
the lifecycle transition.

This guarantees things happen synchronously without an event loop spin
inbetween, which should also take care of the activity thing.

hein updated this revision to Diff 9745.Jan 5 2017, 9:14 AM

Cleanup: filterProxyModel is always flat.

Side note: This is why it makes sense to still collect the indices in the
(tree-shaped) source model, because it avoids unnecessarily filling
dirtyLaunchers for the removal of a group member for a small optimization.

hein updated this revision to Diff 9746.Jan 5 2017, 9:23 AM

Mark unused.

davidedmundson added inline comments.Jan 6 2017, 1:11 PM
libtaskmanager/tasksmodel.cpp
388

I think it's a bit weird that we have a pair:

groupingProxyModel::aboutToRemove
filterProxyModel:rowsRemoved

As it doesn't match up evenly.

It'll work, but having them both the same will be less confusing long term.

I think changing the first connect to filterProxyModel might result in logic being overall simpler. You'd be processing the launcher every time any window got removed, but that might fix what's in the bug reports.

Other than that, the change looks good.

hein updated this revision to Diff 9797.Jan 6 2017, 1:52 PM

After the original bug became actually reproducible (thanks to
Roman for a tip - I needed multiple Chrome tabs to slow down
its teardown) I was able to pinpoint the actual cause, see the
updated summary and the code comment.

Re the lack-of-symmetry complaint: The rowsAboutToBeRemoved
connect is to the tree-shaped proxy because it allows for a
speed optimization by ignoring group children, which is now
even more worthwhile given the more coarse trigger. The
second connect is to the filterProxyModel because the actual
symmetry that's needed is to the model checked in
filterAcceptsRow, which is the filterProxyModel as well. As
previously explained proxy models fire rowsRemoved in response
to their source model firing rowsAboutToBeRemoved, so when
the grouping proxy (which is higher in the chain) fires
rowsRemoved, the filter proxy still has the window, causing
filterAcceptsRow not to allow the launcher to pass.

hein added a comment.Jan 6 2017, 1:54 PM

For some reason Phab didn't update the summary even though I rewrote the commit message. Here's the new text:

Fix "Pinned Chrome disappears when all Chrome windows are closed"

Summary:
It turns out that Chrome under certain conditions will change its
window metadata as it quits, causing a race we sometimes lose, failing
to reveal the associated launcher because we can no longer match it
to the window at window closing time. Instead we are now forced to                                                                                                                                                               
re-check all launchers after the window is gone. As a speed optimi-                                                                                                                                                              
zation we only consider top-level windows (and startups) as being in                                                                                                                                                             
a group implies matching siblings.                                                                                                                                                                                               
                                                                                                                                                                                                                                 
In addition this refactoring eliminates a use of Qt::QueuedConnection                                                                                                                                                            
that allowed for an unpredictable event loop spin inbetween things.
davidedmundson accepted this revision.Jan 6 2017, 2:00 PM
davidedmundson edited edge metadata.
romangg added a subscriber: romangg.Jan 6 2017, 2:09 PM

Just tested it. Last revision fixes the problem indeed.

This revision was automatically updated to reflect the committed changes.
hein added a comment.Jan 6 2017, 2:12 PM

Thanks Roman!