Re: possible data corruption while scrolling through rows

Mike Richardson wrote:
> On Sunday 16 January 2005 23:22, abel deuring wrote:
> 
>>hi,
>>
>>I noticed the following bug in Rekall (I'm using version 2.2.3):
>>
>>If a form block has a sub block, it can happen that the contents of some
>>rows of the sub block is deleted, when the user presses for example the
>>up or down arrow quickly two or more times in the parent block. (The
>>problem may perhaps also occur for a simple form block, if row updates
>>take a long time -- I didn't read the sources carefully enough to tell
>>for sure...)
>>
>>At some time during a call of KBFormBlock::doOperation e.g. for a down
>>arrow keystroke, a progress bar is opened, and TKProgress::setDone calls
>>qApp->processEvents. If the next key stroke arrives in the event queue
>>before this call, processEvents causes another call of
>>KBFormBlock::doOperation. The problem now is that
>>KBFormBlock::doOperation is not reentrant safe. It happened to me quite
>>often, that the first call of KBFormBlock::doOperation was at the point,
>>where KBControl::clearValue() has been called, but where
>>KBControl::getIniValue returns a non-empty value. Hence, during the
>>second call of doOperation, KBControl::changed returns true, and all
>>values of the affected row are deleted.
>>
>>I fixed this (more or less...) by not allowing recursive calls of
>>doOperation. See the attached patch. It detects recursive calls and
>>queues them so that the "real execution" can be deferred until the first
>>call of doOperation is finished. This may lead to an unexpected
>>behaviour, because the execution sequence for events can be changed, if
>>events are queued for different block at the same time. Any comments?
>
> 
> Wow! That is seriously subtle! I'll see about getting the fix in (tho' I think 
> I might use a QValueList rather than a linked list).

Right, a QValueList is of course more "QT-ish" ;) Another possible issue 
is the integer field inOperation. If some future version of Rekall will 
use threads, using QSemaphore instead would be more robust.

> 
> Can you describe the circumstances that the progress bar can appear? I'm 
> curious in case there are some other places that this might arrise.

TKProgress::setDone is called by KBQryLevel::InsertRC and by 
KBCopyExec::showProgress.

But qApp->processEvents is called by more methods: 
KBDumper::dumpDetails, KBDumper::slotTimer, KBDumper::showProgress, 
KBLoader::loadDetails, KBLoader::slotTimer, KBLoader::showProgress,
KBCopyExec::showProgress, KBErrorDlg::slotDetails, 
KBConductor::snapshot, TKProgress::setDone, TKTextEditor::print and 
TKTextEditor::printPreview.

<comment type="back seat driver">
I think the main question is, on which "level" qApp->processEvents 
is/should be called, and/or if/how events that can't be processed 
immediately can be queued for later execution.

Of course, a decent program must handle user input and other events as 
quickly as possible, and some actions like database queries can take a 
long time, hence it makes sense to call qApp->processEvents from time to 
time. OTOH, making them reentrant safe can be quite complex, so I am 
wondering, if of how is it possible to check, if certain events can be 
executed immediately, or if it should queued for later execution. While 
my patch does just that, but only "locally", based on a symptom; but I 
think it is possible that other parts of Rekall might have similar 
reentrance problems, so it would perhaps be better to check for possible 
reentrance problems at a more centralized location.

This sounds quite vague, but I am not familiar enough with QT's event 
handling concept to write something more specific...
</comment>

Abel