GNOME Bugzilla – Bug 699452
Crash when applying operations before pending operations fully displayed
Last modified: 2013-09-18 16:48:45 UTC
Initial testing shows that this crash has been there a long time, introduced between GParted 0.9.1 and 0.10.0. Still reproducible with GParted 0.16.1. The crash doesn't seem to risk user data as it doesn't crash while manipulating user data. It would be quite unlikely for users to generate because of the small window of opportunity. Normally less than 1 second while the animated Pending Operations display is opening. How to reproduce: 0) Ensure there are no Pending Operations and the display is hidden. 1) Enter check operation. 2) While the Pending Operations display is scrolling up, press the [Apply All Operations] button. 2) Apply the pending operations and close the dialog. Reproduction hint: Normally the Pending Operations display animation completes in less than 1 second. However if running GParted remotely over a slow network connection the animation opening the Pending Operations display can easily take 5 seconds giving plenty of time to click the [Apply All Operations] button. Outcome: Operation is successfully applied. Applying pending operations dialog is closed. The Pending Operations display scrolls down. All the widgets in the main window become greyed out and the devices are scanned successfully. The main window is redrawn. The Pending Operations display scrolls up. --8<-- All perfectly normal to here --8<-- Segmentation fault (core dumped) So far check operation of jfs and ext2 leads to a crash, and delete and add ext2 operations don't crash. I don't have immediate plans to fix this. Though I suspect I'll get to it eventually. Mike
NOTE: Compiled with CXXFLAGS='-g -O0' so that variable 't' is not optimised away. # gdb ~mike/programming/c/gparted/src/gpartedbin core.18980 Core was generated by `/home/mike/programming/c/gparted/src/gpartedbin'. Program terminated with signal 11, Segmentation fault.
+ Trace 231958
Debug trace of a normal case of clicking [Apply] after the animation of showing the operation has been added. D: activate_label_partition() Called D: activate_label_partition() operations.size()=0 D: activate_label_partition() Calling Add_Operation(operation) ... D: activate_label_partition() Returned from Add_Operation(operation) D: activate_label_partition() operations.size()=1 D: activate_label_partition() Returns D: activate_apply() Called D: activate_apply() Returns Debug trace of crash case of clicking [Apply] before the animated showing of the operations as completed. D: activate_label_partition() Called D: activate_label_partition() operations.size()=0 D: activate_label_partition() Calling Add_Operation(operation) ... D: activate_apply() Called D: activate_apply() Returns D: activate_label_partition() Returned from Add_Operation(operation) D: activate_label_partition() operations.size()=0 D: activate_label_partition() t=0 D: activate_label_partition() t=1 D: activate_label_partition() t=2 D: activate_label_partition() t=3 Segmentation fault (core dumped) Selected lines of activate_label_partition() ... [Slightly edited] 2454 void Win_GParted::activate_label_partition() 2455 { ... 2472 Add_Operation( operation ) ; 2473 2474 // Verify if the two operations can be merged 2475 for (unsigned int t = 0; t < operations.size() - 1; t++) 2476 { 2477 if (operations[t]->type == OP..._LABEL_PARTITION) 2478 { 2479 if (Merge_Operations(t, operations.size()-1)) 2480 break; 2481 } 2482 } Analysis: So in the crashing case of the [Apply] button being pressed before the animation of the pending operation is fully displayed the sequence of events is: 1) Add_Operation() called 2) Operation added to operations[] vector 3) [Apply] button is enabled 4) Start animation of showing pending operations 5) Before animation completes [Apply] button pressed 6) Operation applied, visuals updated and operations[] vector cleared 7) Animation completes 8) Add_Operation() returns 9) For loop of t from 0 up to less than 4294967295 (unsigned int 0 minus 1 is 2^32 minus 1) 10) operations[] vector is access at invalid offsets 0 upward until one triggers a segmentation fault
Hi Mike, Following are some thoughts on this crash. From looking through your debugging details, it appears that we might need to de-activate GUI controls while the display is being refreshed. In the case of the crash the activate_apply() method made it into the event queue before Add_Operation() had completed. Both Add_Operation() and Merge_Operation() include calls to Refresh_Visual() so it looks like user invoked events can be queued while Add_Operation() or Merge_Operation() are in the process of adding their own events. If this is indeed the problem, then the place to do this is likely Win_GParted::Refresh_Visual(). Curtis
Hi Curtis, I've got a patch drafted which doesn't enable Undo and Apply buttons until after the merging of operations has been performed. Research and testing ongoing. Thanks, Mike
Created attachment 244573 [details] [review] Only enable Undo and Apply buttons after merging operations (draft 1) DRAFT PATCH DO NOT APPLY. Hi Curtis, I've posted this so that if you want you can tell me I'm fixing it wrong, my fix is too obscure or my reasoning is wrong. As you can see my commit message is huge. I still need to debug this to ensure that my reasoning is correct. Small quote from my draft commit message: --8<-- Fix this by not enabling the Undo and Apply buttons until after the merging of any relevant operations has been performed. This will prevent the user from applying the operations before merging has been performed. NOTE: This fixes the case of adding the first operation as the Undo and Apply buttons are previously disabled. When a second operation is added and the buttons are already active a user can press apply when Add_Operation() has added the operation to the operations[] vector but before the merge has been performed. However as there is no call to process GTK events until after the merge, by show_operationslist() -> Refresh_Visual(), adding additional operations is safe too. --8<-- Thanks, Mike
Created attachment 244607 [details] [review] Only allow Undo and Apply after merging operations (v1) Hi Curtis, Here's the final fix. Thanks for your pointer to look closer at Merge_Operations(). Made me realise that I could and should remove Refresh_Visual() from Merge_Operations() as well as Add_Operation() and made the commit message better. If you want to reproduce the crash just using a single desktop do: 1) Open label dialog and update the label; 2) Move the mouse over the grayed out Apply button; 3) Press Return key with one hand and immediately after click the mouse with the other. Aim to both within about 0.2 seconds or less. If the operation list is only partially open when the Apply Operations dialog opens then you got it. To test the difference the removal of flicker of a merged operation makes try adding a second check operation of the same partition. With the old code the operation list and reported number of operations in the status bar flicked. With the new code it doesn't. Thanks, Mike
Created attachment 244731 [details] GParted label with quick apply re-opens empty pending operations pane Thank you Mike for your work on this problem and also for providing steps on how to re-create the crash. Using your instructions I was able to confirm the crash. After applying the patch I noticed a different behaviour. Specifically when using the steps to re-create the crash, after closing the "Apply pending operations" dialog, GParted opened the pending operations pane with 0 operations pending. Following are the steps I used and the sequence of events I observed: 1) Open label dialog and update the label 2) Prepare/ready hand on CTRL+Enter keys 3) Click on "Ok" to queue Label; and quickly press CTRL+Enter and click "Apply" button. Results: a) The pending operation pane begins opening with "Set Partition Label....". b) The operation begins applying and finishes. Underneath the dialog, The "Set Partition Label..." can still be seen in the pending operations pane, though not full size because the "open pane" animation did not have time to complete. 4) Click on "Close" in the Applying pending operations dialog. c) GParted closes the pending operations pane, rescans the drives, then opens the pending operations pane with 0 operations pending. This is obviously a big improvement over the previous crash behaviour, but is still somewhat of an anomaly. There must still be some timing problem that GParted thinks it needs to open the now-empty pending operations pane. Does this anomaly occur for you as well?
Hi Curtis, Re: Pending operations list remaining open after early apply: This is not new behaviour with my patch. GParted has always done this. 1) GParted 0.16.1 (without my patch) does this. Just watch closely before it crashes. 2) GParted 0.9.1 (even before merging of operations) does this. 3) GParted 0.4.7 (the oldest version I can easily build) still does this. Without looking at the code I assume that after the opening animation completes it marks the dialog as open, so given that the apply of operations completed before this, the dialog remains open. As such this behaviour is a separate issue and not introduced by my patch. Thanks, Mike
Thank you Mike for confirming that this anomaly existed before the patch. I should have tested an earlier version and then I would have seen that for myself. The patch in comment #6 has been committed to the Gnome repository for inclusion in the next release of GParted. The relevant git commit can be viewed at the following link: Only allow Undo and Apply after merging operations (#699452) https://git.gnome.org/browse/gparted/commit/?id=4cc426c6cfb579548d432a90f12189494160a5f5
The patch to address this report has been included in GParted 0.16.2 released on September 18, 2013.