cullmann created this revision.
Restricted Application added a project: Frameworks.
REVISION SUMMARY
See:
https://bugs.kde.org/show_bug.cgi?id=380114
#6 0x028c3780 in ?? ()
#7 0x7fabbd75a4f6 in
qDeleteAll::const_iterator> (end=..., begin=...) at
/opt/local/include/qt5
rjvbb added a comment.
A priori this should be fine, and it might even address the long standing bug
by leaving more time for Phonon objects to "do their thing". It might be an
idea though to include a `qCDebug()` probe that outputs the number of items
left for auto-cleanup in `m_reusablePho
cullmann abandoned this revision.
cullmann added a comment.
Hmm, not really, I again just missed that the auto-delete will anyways only
happen after the execution of ~NotifyByAudio, my patch just removes the earlier
cleanup :/
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabr
rjvbb added a comment.
> Hmm, not really
Not really what?
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D6376
To: cullmann, #frameworks
Cc: rjvbb
cullmann added a comment.
> Hmm, not really, I again just missed that the auto-delete will anyways only
happen after the execution of ~NotifyByAudio, my patch just removes the earlier
cleanup :/
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D6376
To: cullma
cullmann updated this revision to Diff 15850.
cullmann edited the summary of this revision.
REPOSITORY
R289 KNotifications
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6376?vs=15844&id=15850
REVISION DETAIL
https://phabricator.kde.org/D6376
AFFECTED FILES
src/notifybyaudio.cpp
cullmann added a comment.
Next try ;=)
Question is, can really such a race happen, that finishNotification triggers
duplicate insertion?
At least with a hash set, this won't lead to later issues.
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D6376
To: cu
mpyne added a comment.
In https://phabricator.kde.org/D6376#119359, @rjvbb wrote:
> But what's the official stance on deleting objects (widgets) that have a
parent and are thus capable of auto-cleanup? AFAIK one can still delete them
explicitly, and if they're reparented during that proc
rjvbb added a comment.
In https://phabricator.kde.org/D6376#119376, @cullmann wrote:
> Next try ;=)
> Question is, can really such a race happen, that finishNotification
triggers duplicate insertion?
Do you see any other explanation why a crash could occur under qDeleteAll,
on
cullmann added a comment.
I don't speak about threading races. The whole class is not thread-safe, if
threading occurs, all is lost.
But I see the chance, that you arrive twice at the
m_reusablePhonons.insert(m); line, as there are multiple ways into the function
doing that, one e.g. vi
aacid added a comment.
I've had a look at the code and honestly i don't see how this can fix
anything.
Do you have a reproduceable testcase for the crash?
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D6376
To: cullmann, #frameworks
Cc: aacid, mpyne, rj
cullmann added a comment.
Hi,
if you can arrive twice for the same mediaobject in finishNotification e.g.
via close and onAudioFinished you might insert it twice and delete it twice.
If not, this change helps nothing.
Unfortunately I have no way to reproduce.
We can just disca
cullmann abandoned this revision.
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D6376
To: cullmann, #frameworks
Cc: aacid, mpyne, rjvbb
rjvbb added a comment.
In https://phabricator.kde.org/D6376#119500, @cullmann wrote:
> I don't speak about threading races. The whole class is not thread-safe, if
threading occurs, all is lost.
Really? Only a single thread at a time should trigger audio notifications, is
that wha
aacid added a comment.
I was able to half reproduce it for a while and came up with
https://phabricator.kde.org/D11891 which I think is a bit less of a workaround
than this code.
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D6376
To: cullmann, #frameworks
15 matches
Mail list logo
