[prev in list] [next in list] [prev in thread] [next in thread]
List: kfm-devel
Subject: Re: Review Request: Fix crash due to deleteLater being sent to the wrong object sometimes
From: "Frank Reininghaus" <frank78ac () googlemail ! com>
Date: 2012-12-20 14:57:37
Message-ID: 20121220145737.2006.23699 () vidsolbach ! de
[Download RAW message or body]
> On Dec. 11, 2012, 7:23 a.m., Frank Reininghaus wrote:
> > Thanks for the patch!
> >
> > > If updateItemStates was called from slotThreadFinished,
> > > m_updateItemStatesThread would be different when slotThreadFinished was
> > > over. deleteLater would be called on the *new* m_updateItemStatesThread,
> > > instead of the just-finished one, causing it to be deleted before
> > > it had finished running, and a crash.
> >
> > Are you sure? VersionControlObserver::updateItemStates() connects the finished() \
> > signal and the deleteLater() slot of the *same* thread, so I don't see how a \
> > thread can be deleted before it's finished.
> > About the "} else if(!m_updateItemStatesThread){" changes: could you elaborate on \
> > why this could fix a crash? Moreover, could it be a problem that your new \
> > implementation may return an empty list 'actions' if lockPlugin() fails?
> > Sorry if my questions are stupid! I have little experience with the version \
> > control stuff ;-)
>
> Simeon Bird wrote:
> I'm not sure, but I have a 2-sigma confidence :)
>
> This is how I figured it out:
>
> I was getting this crash occasionally, so I tried removing the unlock/relock pair.
> This was slower, of course, so then I tried making the lock a QReadWriteLock.
> Rather to my surprise, I then got the crash all the time! And in the logs there was \
> the message:
> QThread: Destroyed while thread is still running
>
> Which means that a delete event has to be passed to the QThread *before* the \
> finished() signal is emitted.
> Here is what I think happens:
>
> updateItemStates() connects m_updateItemStatesThread's finished() to \
> slotThreadFinished() and m_updateItemStatesThread->deleteLater() (in that order). \
> So when the event is fired, Qt calls slotThreadFinished(), and then once it has \
> finished, calls m_updateItemStatesThread->deleteLater().
> BUT m_updateItemStatesThread is changed in slotThreadFinished, and thus does not \
> point to the same object that emitted finished()!
> Most of the time we have m_updateItemStatesThread=0 (which just leaks), but \
> sometimes we call updateItemStates() from slotThreadFinished(), which assigns \
> m_updateItemStatesThread to a new QThread, which then gets deleted too early.
>
> As to the } else if(!m_updateItemStatesThread){ bit:
>
> The code tries to get the plugin lock if m_updateItemStatesThread != 0. But getting \
> the plugin lock may fail (because m_updateItemStatesThread is holding onto it too \
> long). If it does fail to get the lock, however, the code used the plugin anyway, \
> which is not safe, because it may be being changed by \
> m_updateItemStatesThread::setData at the time. I think returning an empty action \
> list is fine, because it will just mean that some file will not have the "git add" \
> menu items. I think in practice that the plugin lock can always be obtained though, \
> so it is a bit academic.
> You want I should put all this in the commit messages? :)
>
> Frank Reininghaus wrote:
> First of all, thanks for your work! I really appreciate it. The only reason why I \
> ask so many questions is that I want to make sure that we don't push something that \
> just hides the real bug (making it harder to find and fix it later on - I've seen \
> such things happen) rather than fixing it.
> > Here is what I think happens:
> >
> > updateItemStates() connects m_updateItemStatesThread's finished() to \
> > slotThreadFinished() and m_updateItemStatesThread->deleteLater() (in that order). \
> > So when the event is fired, Qt calls slotThreadFinished(), and then once it has \
> > finished, calls m_updateItemStatesThread->deleteLater().
> > BUT m_updateItemStatesThread is changed in slotThreadFinished, and thus does not \
> > point to the same object that emitted finished()!
> > Most of the time we have m_updateItemStatesThread=0 (which just leaks), but \
> > sometimes we call updateItemStates() from slotThreadFinished(), which assigns \
> > m_updateItemStatesThread to a new QThread, which then gets deleted too early.
>
> Just to make sure that I really understand this: You are saying that we fist have \
> an UpdateItemStatesThread "thread1". Its finished() signal is connected to its own \
> deleteLater() slot and to VersionControlObserver::slotThreadFinished(). When the \
> latter is called, a new UpdateItemStatesThread "thread2" might be created in \
> VersionControlObserver::updateItemStates(). This function connects "thread2"'s \
> finished() signal to "thread2"'s deleteLater() slot. I don't see a problem here - \
> "thread1"'s finished() signal is still connected to its own deleteLater() slot, not \
> to the one of "thread2".
> If I understand your statement
>
> > BUT m_updateItemStatesThread is changed in slotThreadFinished, and thus does not \
> > point to the same object that emitted finished()!
>
> correctly, then you are saying that now "thread1"'s finished() signal invokes \
> "thread2"'s deleteLater() slot? AFAIK, this is not possible. The connection between \
> "thread1"'s signal and its own slot is not changed just because a member of the \
> VersionControlObserver changes.
> > As to the } else if(!m_updateItemStatesThread){ bit:
> >
> > The code tries to get the plugin lock if m_updateItemStatesThread != 0. But \
> > getting the plugin lock may fail (because m_updateItemStatesThread is holding \
> > onto it too long). If it does fail to get the lock, however,
> > the code used the plugin anyway, which is not safe, because it may be being \
> > changed by m_updateItemStatesThread::setData at the time.
>
> But UpdateItemStatesThread::setData() is only called by the VersionControlObserver, \
> so this can't really happen at the same time as a call to \
> VersionControlObserver::actions() because both are supposed to run in the main \
> thread, right? Moreover, UpdateItemStatesThread::setData() only changes the \
> m_plugin member of the thread (by assigning a new value to the pointer), so I don't \
> really see why we have to protect accesses to the m_plugin member of the \
> VersionControlObserver at all, but maybe I'm missing something.
> > I think in practice that the plugin lock can always be obtained though, so it is \
> > a bit academic.
>
> As I said, I don't see why the lock is needed at all, maybe we should ask Peter \
> about it.
> > You want I should put all this in the commit messages? :)
>
> Not necessarily, but I want to understand why a change makes sense before it is \
> pushed :-)
>
> Simeon Bird wrote:
> > First of all, thanks for your work! I really appreciate it. The only reason why I \
> > ask so many questions is that I want to make sure that we don't push something \
> > that just hides the real bug (making it harder to find and fix it later on - I've \
> > seen such things happen) rather > than fixing it.
>
> You're very welcome! Sorry for the late reply, real life intervened. And please do \
> ask the questions, because I am new to this, and especially because in this case \
> you are right.
> > correctly, then you are saying that now "thread1"'s finished() signal invokes \
> > "thread2"'s deleteLater() slot? AFAIK, this is not possible. > The connection \
> > between "thread1"'s signal and its own slot is not changed just because a member \
> > of the VersionControlObserver changes.
>
> That is exactly what I was saying. Also, you're right, signals and slots do indeed \
> work like that (I checked with a test program), and the event really should not be \
> delivered to the wrong item. Furthermore, I can still get the crash, just less \
> often with all these patches. So I think at the moment that moving the deleteLater \
> around doesn't fix it, but it changes whatever race is happening just enough to \
> make it less likely to occur.
> Also there was a deadlock bug in the posted code (I forgot to change the order the \
> mutexes are taken in setData() ), which was causing the loading to hang sometimes, \
> hiding the crash.
> So to summarize:
>
> I have this crash bug. It persists.
>
> I'm getting this message:
>
> QThread: Destroyed while thread is still running
>
> from gdb, which should be completely impossible. The QThread is only deleted from \
> once the finished() signal is emitted. QObject auto-deletion can't be happening \
> here because the calling object is manifestly still there, blocking in setData \
> (right?).
> > But UpdateItemStatesThread::setData() is only called by the \
> > VersionControlObserver, so this can't really happen at the same time as a call \
> > to VersionControlObserver::actions() because both are supposed to run in the main \
> > thread, right? Moreover, UpdateItemStatesThread::setData() only changes the \
> > m_plugin member of the thread (by assigning a new value to the pointer), so I \
> > don't really see why we have to protect accesses to the m_plugin member of the \
> > VersionControlObserver at all, but maybe I'm missing something.
>
> Ah, I see what you mean. I thought when reading the code (but I didn't check this) \
> that plugin->beginRetrieval() could internally have some state which could affect \
> the output of plugin->actions(), and that was why the mutex was there. I don't \
> think the git plugin has any such thing (and for that endRetrieval is a no-op). I \
> also tried disabling the locking, and it didn't seem to make much difference, so \
> it's up to you.
> Simeon Bird wrote:
> Wait, m_plugin is a KVersionControlPlugin, which is a QObject, and we're using it \
> from multiple threads. Is that the real cause of the crash here?
> Wait, m_plugin is a KVersionControlPlugin, which is a QObject, and we're using it \
> from multiple threads. Is that the real cause of the crash here?
Accessing a QObject from multiple threads is in principle not a problem, provided \
that no threads call non-thread-safe methods or read and write data at the same time.
> I'm getting this message:
>
> QThread: Destroyed while thread is still running
Then one could put a gdb breakpoint in the function where that message is printed \
(probably QThread's destructor?) and check where that call comes from. That won't \
help much though if it comes from a signal-slot connection between threads. But if \
the thread is still running, its finished() signal cannot have been emitted yet, so \
it should not be the finished->deleteLater connection that is responsible for this.
> QObject auto-deletion can't be happening here because the calling object is \
> manifestly still there, blocking in setData (right?).
What exactly do you mean with 'QObject auto-deletion'? AFAIK, a QObject is only \
deleted if delete/deleteLater are used directly, or if the destructor of its parent \
is executed (but the UpdateItemStatesThread does not have a parent at all).
Another idea what might be causing a crash: if the UpdateItemStates thread is deleted \
while the main thread is trying to lock the mutex in \
UpdateItemStatesThread::setData(), that could be a problem. But I don't see how that \
could happen.
- Frank
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107656/#review23301
-----------------------------------------------------------
On Dec. 10, 2012, 4:27 a.m., Simeon Bird wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107656/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2012, 4:27 a.m.)
>
>
> Review request for Dolphin and Frank Reininghaus.
>
>
> Description
> -------
>
> Three commits here:
>
> commit bd89a3ebe4f546e2032676c4b445befd9606e877
>
> Fix a potential crash if the plugin lock could not be taken
> in VersionControlObserver.
>
> (the "} else if(!m_updateItemStatesThread){" changes: I didn't see this, and I \
> think it is unlikely, but best to be safe.)
>
> commit b1473eb1247e9168805ac92964cab11a10d6a0e2
>
> Make the UpdateItemStatesThread use a QReadWriteLock for protection
> instead of a QMutex, now the previous crash is fixed and we can.
>
> Makes listings a bit faster.
>
> commit 8f17e50800a7769521e496ab324d608ec0e36df4
>
> VersionControlObserver:
>
> If updateItemStates was called from slotThreadFinished,
> m_updateItemStatesThread would be different when slotThreadFinished was
> over. deleteLater would be called on the *new* m_updateItemStatesThread,
> instead of the just-finished one, causing it to be deleted before
> it had finished running, and a crash.
>
> Instead call deleteLater from slotThreadFinished directly.
>
> BUG: 302264
> FIXED-IN: 4.10
>
> This should really fix bug 302264 this time.
>
>
> This addresses bug 302264.
> http://bugs.kde.org/show_bug.cgi?id=302264
>
>
> Diffs
> -----
>
> dolphin/src/views/versioncontrol/updateitemstatesthread.h f0f91d7
> dolphin/src/views/versioncontrol/updateitemstatesthread.cpp e07d72c
> dolphin/src/views/versioncontrol/versioncontrolobserver.cpp 42e00de
>
> Diff: http://git.reviewboard.kde.org/r/107656/diff/
>
>
> Testing
> -------
>
> Bug is fixed!
>
> It was very easy to reproduce (for me) once the window for it was made wider by \
> using QReadWriteLock in UpdateItemStatesThread, and is no longer there.
>
> Thanks,
>
> Simeon Bird
>
>
[Attachment #3 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/107656/">http://git.reviewboard.kde.org/r/107656/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On December 11th, 2012, 7:23 a.m., <b>Frank \
Reininghaus</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px \
solid #d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Thanks for the patch!
> If updateItemStates was called from slotThreadFinished,
> m_updateItemStatesThread would be different when slotThreadFinished was
> over. deleteLater would be called on the *new* m_updateItemStatesThread,
> instead of the just-finished one, causing it to be deleted before
> it had finished running, and a crash.
Are you sure? VersionControlObserver::updateItemStates() connects the finished() \
signal and the deleteLater() slot of the *same* thread, so I don't see how a \
thread can be deleted before it's finished.
About the "} else if(!m_updateItemStatesThread){" changes: could you \
elaborate on why this could fix a crash? Moreover, could it be a problem that your \
new implementation may return an empty list 'actions' if lockPlugin() fails?
Sorry if my questions are stupid! I have little experience with the version control \
stuff ;-)</pre> </blockquote>
<p>On December 12th, 2012, 2:57 a.m., <b>Simeon Bird</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm not sure, but I \
have a 2-sigma confidence :)
This is how I figured it out:
I was getting this crash occasionally, so I tried removing the unlock/relock pair.
This was slower, of course, so then I tried making the lock a QReadWriteLock.
Rather to my surprise, I then got the crash all the time! And in the logs there was \
the message:
QThread: Destroyed while thread is still running
Which means that a delete event has to be passed to the QThread *before* the \
finished() signal is emitted.
Here is what I think happens:
updateItemStates() connects m_updateItemStatesThread's finished() to \
slotThreadFinished() and m_updateItemStatesThread->deleteLater() (in that order). \
So when the event is fired, Qt calls slotThreadFinished(), and then once it has \
finished, calls m_updateItemStatesThread->deleteLater().
BUT m_updateItemStatesThread is changed in slotThreadFinished, and thus does not \
point to the same object that emitted finished()!
Most of the time we have m_updateItemStatesThread=0 (which just leaks), but sometimes \
we call updateItemStates() from slotThreadFinished(), which assigns \
m_updateItemStatesThread to a new QThread, which then gets deleted too early.
As to the } else if(!m_updateItemStatesThread){ bit:
The code tries to get the plugin lock if m_updateItemStatesThread != 0. But getting \
the plugin lock may fail (because m_updateItemStatesThread is holding onto it too \
long). If it does fail to get the lock, however, the code used the plugin anyway, \
which is not safe, because it may be being changed by \
m_updateItemStatesThread::setData at the time. I think returning an empty action \
list is fine, because it will just mean that some file will not have the "git \
add" menu items. I think in practice that the plugin lock can always be obtained \
though, so it is a bit academic.
You want I should put all this in the commit messages? :)</pre>
</blockquote>
<p>On December 12th, 2012, 11:59 a.m., <b>Frank Reininghaus</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">First of all, thanks for \
your work! I really appreciate it. The only reason why I ask so many questions is \
that I want to make sure that we don't push something that just hides the real \
bug (making it harder to find and fix it later on - I've seen such things happen) \
rather than fixing it.
> Here is what I think happens:
>
> updateItemStates() connects m_updateItemStatesThread's finished() to \
slotThreadFinished() and m_updateItemStatesThread->deleteLater() (in that order). \
So when the event is fired, > Qt calls slotThreadFinished(), and then once it has \
finished, calls m_updateItemStatesThread->deleteLater(). >
> BUT m_updateItemStatesThread is changed in slotThreadFinished, and thus does not \
point to the same object that emitted finished()! >
> Most of the time we have m_updateItemStatesThread=0 (which just leaks), but \
sometimes we call updateItemStates() from slotThreadFinished(), which assigns \
m_updateItemStatesThread > to a new QThread, which then gets deleted too early.
Just to make sure that I really understand this: You are saying that we fist have an \
UpdateItemStatesThread "thread1". Its finished() signal is connected to its \
own deleteLater() slot and to VersionControlObserver::slotThreadFinished(). When the \
latter is called, a new UpdateItemStatesThread "thread2" might be created \
in VersionControlObserver::updateItemStates(). This function connects \
"thread2"'s finished() signal to "thread2"'s \
deleteLater() slot. I don't see a problem here - "thread1"'s \
finished() signal is still connected to its own deleteLater() slot, not to the one of \
"thread2".
If I understand your statement
> BUT m_updateItemStatesThread is changed in slotThreadFinished, and thus does not \
point to the same object that emitted finished()!
correctly, then you are saying that now "thread1"'s finished() signal \
invokes "thread2"'s deleteLater() slot? AFAIK, this is not possible. \
The connection between "thread1"'s signal and its own slot is not \
changed just because a member of the VersionControlObserver changes.
> As to the } else if(!m_updateItemStatesThread){ bit:
>
> The code tries to get the plugin lock if m_updateItemStatesThread != 0. But \
getting the plugin lock may fail (because m_updateItemStatesThread is holding onto it \
too long). > If it does fail to get the lock, however,
> the code used the plugin anyway, which is not safe, because it may be being \
changed by m_updateItemStatesThread::setData at the time.
But UpdateItemStatesThread::setData() is only called by the VersionControlObserver, \
so this can't really happen at the same time as a call to \
VersionControlObserver::actions() because both are supposed to run in the main \
thread, right? Moreover, UpdateItemStatesThread::setData() only changes the m_plugin \
member of the thread (by assigning a new value to the pointer), so I don't really \
see why we have to protect accesses to the m_plugin member of the \
VersionControlObserver at all, but maybe I'm missing something.
> I think in practice that the plugin lock can always be obtained though, so it is \
a bit academic.
As I said, I don't see why the lock is needed at all, maybe we should ask Peter \
about it.
> You want I should put all this in the commit messages? :)
Not necessarily, but I want to understand why a change makes sense before it is \
pushed :-) </pre>
</blockquote>
<p>On December 20th, 2012, 6:29 a.m., <b>Simeon Bird</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> First of all, \
thanks for your work! I really appreciate it. The only reason why I ask so many \
questions is that I want to make sure that > we don't push something that \
just hides the real bug (making it harder to find and fix it later on - I've seen \
such things happen) rather > than fixing it.
You're very welcome! Sorry for the late reply, real life intervened. And please \
do ask the questions, because I am new to this, and especially because in this case \
you are right.
> correctly, then you are saying that now "thread1"'s finished() \
signal invokes "thread2"'s deleteLater() slot? AFAIK, this is not \
possible. > The connection between "thread1"'s signal and its own \
slot is not changed just because a member of the VersionControlObserver changes.
That is exactly what I was saying. Also, you're right, signals and slots do \
indeed work like that (I checked with a test program), and the event really should \
not be delivered to the wrong item. Furthermore, I can still get the crash, just less \
often with all these patches. So I think at the moment that moving the deleteLater \
around doesn't fix it, but it changes whatever race is happening just enough to \
make it less likely to occur.
Also there was a deadlock bug in the posted code (I forgot to change the order the \
mutexes are taken in setData() ), which was causing the loading to hang sometimes, \
hiding the crash.
So to summarize:
I have this crash bug. It persists.
I'm getting this message:
QThread: Destroyed while thread is still running
from gdb, which should be completely impossible. The QThread is only deleted from \
once the finished() signal is emitted. QObject auto-deletion can't be happening \
here because the calling object is manifestly still there, blocking in setData \
(right?).
> But UpdateItemStatesThread::setData() is only called by the \
VersionControlObserver, so this can't really happen at the same time as a call \
> to VersionControlObserver::actions() because both are supposed to run in the \
main thread, right? Moreover, > UpdateItemStatesThread::setData() only changes \
the m_plugin member of the thread (by assigning a new value to the pointer), so I \
don't > really see why we have to protect accesses to the m_plugin member of \
the VersionControlObserver at all, but maybe I'm missing something.
Ah, I see what you mean. I thought when reading the code (but I didn't check \
this) that plugin->beginRetrieval() could internally have some state which could \
affect the output of plugin->actions(), and that was why the mutex was there. I \
don't think the git plugin has any such thing (and for that endRetrieval is a \
no-op). I also tried disabling the locking, and it didn't seem to make much \
difference, so it's up to you.</pre> </blockquote>
<p>On December 20th, 2012, 6:31 a.m., <b>Simeon Bird</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Wait, m_plugin is a \
KVersionControlPlugin, which is a QObject, and we're using it from multiple \
threads. Is that the real cause of the crash here?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> Wait, m_plugin is a \
KVersionControlPlugin, which is a QObject, and we're using it from multiple \
threads. > Is that the real cause of the crash here?
Accessing a QObject from multiple threads is in principle not a problem, provided \
that no threads call non-thread-safe methods or read and write data at the same time.
> I'm getting this message:
>
> QThread: Destroyed while thread is still running
Then one could put a gdb breakpoint in the function where that message is printed \
(probably QThread's destructor?) and check where that call comes from. That \
won't help much though if it comes from a signal-slot connection between threads. \
But if the thread is still running, its finished() signal cannot have been emitted \
yet, so it should not be the finished->deleteLater connection that is responsible \
for this.
> QObject auto-deletion can't be happening here because the calling object is \
manifestly still there, blocking in setData (right?).
What exactly do you mean with 'QObject auto-deletion'? AFAIK, a QObject is \
only deleted if delete/deleteLater are used directly, or if the destructor of its \
parent is executed (but the UpdateItemStatesThread does not have a parent at all).
Another idea what might be causing a crash: if the UpdateItemStates thread is deleted \
while the main thread is trying to lock the mutex in \
UpdateItemStatesThread::setData(), that could be a problem. But I don't see how \
that could happen.</pre> <br />
<p>- Frank</p>
<br />
<p>On December 10th, 2012, 4:27 a.m., Simeon Bird wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Dolphin and Frank Reininghaus.</div>
<div>By Simeon Bird.</div>
<p style="color: grey;"><i>Updated Dec. 10, 2012, 4:27 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Three commits here:
commit bd89a3ebe4f546e2032676c4b445befd9606e877
Fix a potential crash if the plugin lock could not be taken
in VersionControlObserver.
(the "} else if(!m_updateItemStatesThread){" changes: I didn't see \
this, and I think it is unlikely, but best to be safe.)
commit b1473eb1247e9168805ac92964cab11a10d6a0e2
Make the UpdateItemStatesThread use a QReadWriteLock for protection
instead of a QMutex, now the previous crash is fixed and we can.
Makes listings a bit faster.
commit 8f17e50800a7769521e496ab324d608ec0e36df4
VersionControlObserver:
If updateItemStates was called from slotThreadFinished,
m_updateItemStatesThread would be different when slotThreadFinished was
over. deleteLater would be called on the *new* m_updateItemStatesThread,
instead of the just-finished one, causing it to be deleted before
it had finished running, and a crash.
Instead call deleteLater from slotThreadFinished directly.
BUG: 302264
FIXED-IN: 4.10
This should really fix bug 302264 this time. </pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Bug is fixed!
It was very easy to reproduce (for me) once the window for it was made wider by using \
QReadWriteLock in UpdateItemStatesThread, and is no longer there.</pre> </td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=302264">302264</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>dolphin/src/views/versioncontrol/updateitemstatesthread.h <span style="color: \
grey">(f0f91d7)</span></li>
<li>dolphin/src/views/versioncontrol/updateitemstatesthread.cpp <span style="color: \
grey">(e07d72c)</span></li>
<li>dolphin/src/views/versioncontrol/versioncontrolobserver.cpp <span style="color: \
grey">(42e00de)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107656/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic