GNOME Bugzilla – Bug 729139
Refactor OperationDetail to address random behavior
Last modified: 2014-06-10 17:17:16 UTC
Refactor OperationDetail to address random behavior Following the enhancements for bug 685740 - Refactor to use asynchronous command execution, some random behavior has been witnessed that might also result in a program crash. Some of the strange behaviour, such as an intermittent inability to cancel a running operation, has been noted in bug 467925 - gparted: add progress bar during operation. This bug report is meant to provide separate tracking for a patch to address this random behaviour. Following is the link to the initial patch: https://bugzilla.gnome.org/show_bug.cgi?id=467925#c93 I will rebase the patch to include this new bug number title and repost the patch in this bug report. Curtis Following is a #parted IRC discussion of this patch Phillip Susi: <gedakc> RE: OperationDetail patch, do you have a test example that shows the problem before and after? That would make it easier to review the patch. <psusi> hrm... I think I was able to see the errors in valgrind most of the time <psusi> but they only rarely caused an actual crash and I never found a way to reproduce it reliably <gedakc> Okay. If it's hard to reproduce then I will focus on reviewing the patch logic. Perhaps LarryT will be able to perform some tests too. <psusi> yea, the reason for the change should be pretty clear from the changelog and following the code... you can't have pointers to things sitting in a std::vector laying around since the vector can decide to move them around to make room when you add another <psusi> in other words, when you push into the vector, if the existing array isn't large enough, it creates a new array, copy constructs the new objects, then destroys the old ones, invalidating your pointer <psusi> I believe valgrind did a good job catching the invalid references to the pointer after this happened... but that only sometimes causes an actual crash <psusi> then sometimes it slightly corrupts the heap, leading to a crash later on like the one in that bug report <psusi> this started from larry's testing of another patch... showing progress wasn't it? <psusi> he found it crashed sometimes... I believe if he applies that patch, those crashes should go away <gedakc> Would it help to have this OperationDetail patch in a separate bug report? I'm thinking of making it easier so that you might incorporate the patch into ubuntu 14.04 LTS when the patch is completed. <psusi> sure <gedakc> My guess it that it would be much harder to get a full GParted 0.19.0 into 14.04, than it is to get a patch to fix a bug. <psusi> oh definately <gedakc> Since you created the patch did you also wish to create the bug report? <psusi> you go ahead, I've been a little busy ;)
Created attachment 275376 [details] [review] Refactor OperationDetail patch v2 The attached patch has had the git comment amended and originated from the following link: https://bugzilla.gnome.org/show_bug.cgi?id=467925#c93 This patch is an attempt to address a problem reported by Laurent de Trogoff with cancelling an operation in bug #467925 comment #54. Laurent, If you have a chance, please test this new patch to see if it fixes the problem you first discovered when cancelling an operation. Curtis
Hello Curtis, OK, I will give it a try asap. Probably tomorrow afternoon. I will update with last git repo and then apply the patch.
Okay. I ended up finding enough time to make tests :) Running patched version on ubuntu 14.04 (dev branch, not updated) Shrink ext3 partition (52go to 22go) Try to cancel here : Relocating blocks XXXXXXX---------------- Get the warning and then force cancel with 5 seconds left Forced cancel here :Relocating blocks XXXXXXXXXXXXXXXXXXXXXXXX---------------- Partition size come back to previous size, just like nothing has been done. (save details and screenshots) Let me know if you want different tests ? Larry
Thank you Larry for your testing. I have done some tests on Kubuntu 12.04 and so far the new patch has worked 100% every time. Having said that I find that even with the unpatched code it is extremely difficult to get a Cancel/Force Cancel/Cancel Operation action to fail to cancel. If you have a chance it would help if you could test on a few different distros too. I plan to look more closely at the code again to see if I can get my head around how it works, probably in a week or two. Curtis
Created attachment 276402 [details] [review] Refactor OperationDetail patch v3 Hi All, Attached is an updated patch which includes minor updates to the commit message, and also makes the format of one if/else statement in OperationDetail::on_update() consistent with others in the code. I have successfully tested this patch, copying/moving partitions with/without cancelling, on the following distros: debian 7 fedora 20 kubuntu 12.04 opensuse 12.3 ubuntu 14.04 It should be noted that the condition when the unpatched code fails is very difficult to reproduce. As such I have reviewed the patch and the changes and logic behind these make sense to me. If there are no objections over the next few days, then I will commit this patch to the master branch, along with: Bug 729800 - Prevent GSource double-destroy warning messages. Since this refactor patch might address some of the recent ubuntu crash reports, I plan to prepare for a new release soon. Please let me know if there are other "almost ready" bug reports that should be included in the release. Curtis
The patch in comment #5 has been committed to the git master branch for inclusion in the next release of GParted. The relevant git commit can be viewed at the following link: Change OperationDetail to not store complex objects in STL containers (#729139) https://git.gnome.org/browse/gparted/commit/?id=947cd028575ceb9eee0e7facc2c1749397e50aa9
This enhancement was included in the GParted 0.19.0 release on June 10, 2014.