Review Request 127038: Fix crash when starting a new job if a job for given configuration is already running

[Kevin Funk]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127038/#review92252
-----------------------------------------------------------



I see the problem, but I think the fix is wrong.

The message box is still useful and we should keep it. Maybe we should simply `break` after we show the message box? Unlikely there are multiple jobs running of the same type. And we don't want to show a series of message boxes either...

@apol: Your feature afaik. Opinions?

- Kevin Funk


On Feb. 11, 2016, 8:46 a.m., Maciej Cencora wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127038/
> -----------------------------------------------------------
> 
> (Updated Feb. 11, 2016, 8:46 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> If we happen to enter the if branch, QMessageBox is shown and local processing loop gets invoked.
> If a job that was on currentJobs() list finishes before user closes the message box, the job will be deleted and on next iteration of the foreach loop, findNativeJob will crash trying to perform qobject_cast on deleted object.
> 
> currentJobList(job#1, job#2)
> enter loop:
>  - first loop iteration (process job#1)
>  - duplication detected
>  - show QMessageBox
>  - enter local event loop
>    - job#2 finishes
>    - user closes the QMessageBox
>  - QMessageBox returns
>  - next loop iteration (process job#2)
>  - findNativeJob(job#2) 
>    - qobject_cast on deleted object -> CRASH
> 
> 
> While attached patch fixes the crash, it does not fix a problem of running multiple test suites in parallel (when starting one after another manually by user).
> I don't know why we allow only one job per configuration. Is this loop needed at all?
> I need your help on where to go from this.
> 
> 
> Diffs
> -----
> 
>   plugins/execute/nativeappjob.cpp fcc0a38 
> 
> Diff: https://git.reviewboard.kde.org/r/127038/diff/
> 
> 
> Testing
> -------
> 
> To reproduce this issue:
> 
> 1) create empty cmake project with two tests:
> cmake_minimum_required(VERSION 2.6)
> project(unit_test_crash)
> 
> enable_testing()
> 
> find_program(SLEEP sleep)
> find_program(ECHO echo)
> 
> add_test(unit_test_crash ${SLEEP} 5)
> add_test(unit_test_crash2 ${ECHO} "Test 2 finished")
> 
> 2) show the Unit Tests view
> 3) start the test "unit_test_crash" (by single left mouse click on it)
> 4) while the first test is running, start the second test
> 
> 
> Thanks,
> 
> Maciej Cencora
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160211/a310e984/attachment-0001.html>