GNOME Bugzilla – Bug 168582
Application crashes when dragging the jpeg quality slider
Last modified: 2008-01-15 12:48:56 UTC
Steps to reproduce: 1. Open some jpeg image in GIMP 2. Edit it a bit (anything will do) 3. choose "Save as" from the menu 4. enter some name in the Name field of the "Save Image" dialog 5. press the save button 6. The "save as JPEG" dialog pops up 7. mark "Show previes in image window" 8. Now: select a rectangular region on the image 9. move this selection a bit 10. Drag the "Quality" slider of the "Save as JPEG" dialog GIMP crashes! Stack trace: Other information:
Doesn't crash here (but gives tons of warnings). Well, manipulating the layer that says "JPEG preview" is of course asking for trouble.
It isn't clear to me what GIMP ought to do in this situation. The best thing would be to somehow mark the preview "read only" so that the situation doesn't arise, but there isn't currently a way to do that. Should GIMP then silently undo whatever the user has done to the preview, or what?
Well, it definitely must not crash. So the first step is to find out what exactly the problem is. Running gimp in gdb with --g-fatal-warnings should be good start.
Well, doing so basically just tells us that the image is corrupt. And that's not so surprising: the jpeg plug-in, in updating the preview in this situation, is removing a layer and then creating a new layer in an image that has a floating selection.
Here's a thought: the jpeg plug-in could mark the image as "clean" after creating a preview, and then, when trying to update the preview, bail out if the image has become "dirty" in the meantime.
Here is a patch implementing the idea suggested above -- this is not ready to apply, and is presented more to give the idea. At the very least there must be a better way to bail out than simply calling exit, but from a callback I don't know how to do it. I switched from freeze-thaw to an undo group for two reasons: first, if it is necessary to bail out, an undo can then remove any cruft that has gotten into the image during the save process. Second, the approach taken here will leave the image marked clean even if the user cancels or saves a copy. Doing this right would ultimately involve having to implement gimp_image_undo() in the pdb.
Created attachment 38982 [details] [review] patch for plug-ins/jpeg/jpeg-save.c and jpeg.c
w/o looking at your patch: the preferred way to abort a plug-in is to call gimp_quit() instead of exit().
Thank you. But I am really asking for an opinion on the acceptability of an approach that involves (when previewing in-image for a non-exported image): 1) Starting an undo group when the preview is generated. 2) Marking the image as clean after generating a preview. 3) Checking whether the image has become dirty when updating the preview, and if so, aborting the save. 4) Either on success or abort, when finished, closing the undo group and undoing. This is the only way I see to prevent the user doing things that can cause GIMP to crash (other than dropping the ability to preview in-image).
Bill, I don't understand your approach. We haven't even figured out yet where the problem is. How can we discuss a solution then?
As a start, here's a stack trace of the crash: Program received signal SIGSEGV, Segmentation fault.
+ Trace 57233
Thread 1083470624 (LWP 29332)
It appears that the problem is that we have a floating selection that has a pointer to a layer which doesn't exist any longer. This is something that must not happen and needs to be fixed in GIMP, not in the JPEG plug-in. I suggest that when a layer is removed from an image, that the associated floating selection is removed as well.
Created attachment 39145 [details] [review] remove the floating selection with the layer it belongs to This change seems to fix it but I don't have the time right now to study it in all detail. Attaching it here so that it doesn't get lost ...
The patch should use gimp_drawable_has_floating_sel() and this should be done whenever removing a layer, channel or layer mask.
Oh, and it should put the stuff into an undo group.
Note that part of the problem here is that the jpeg plug-in freezes undo when generating previews. There are a couple of reasons why it needs to do this: (1) it shouldn't change the dirtyness state of the image, and (2) it shouldn't leave the undo stack altered. A solution would require at least making gimp_image_undo available in the pdb. But really the basis of my proposed "fix" is that there's no way to win if you allow the user to manipulate the image while the plug-in is using it to hold previews. If you freeze undo, then the user's changes will permanently mess up the undo history. If you work inside an undo group and undo it after the save is finished, then you are undoing the changes that the user has made. So the only viable approach, as far as I can see, is to prevent the problem from arising in the first place, by bailing out if the user alters the image while it contains a plugin-generated preview.
There's no point of fixing this in a plug-in. The problem is in the core and needs to be fixed there. A plug-in must not be able to crash gimp nor to mess up the undo stack.
More generally, the problem here is lack of drawable locking and the lack of an API that would allow plug-ins to use the image window for their preview needs. None of this can be fixed in the JPEG plug-in.
Well, with gimp_image_undo_disable and gimp_image_undo_freeze available in the pdb, there is no way to prevent a malicious plug-in from messing up the undo system if it wants to. I completely agree with comment #18.
Bill, there is a way. We can clean up after the plug-in exits and make sure that the stack isn't left with open undo groups and/or frozen. We already do that for contexts and pop contexts if the plug-in failed to do that. Of course the plug-in can trash the undo stack by calling gimp_image_undo_disable() but that will still leave the undo stack in a consistent state.
Created attachment 39222 [details] [review] Different patch to fix the problem This patch tries to avoid possible side effects by removing the floating selection in the same order as if it was a separately called operation.
Created attachment 39412 [details] [review] revised patch This is a successor of mitch's patch. This one actually compiles and also handles floating selections on channels. It doesn't handle removal of layer masks with attached floating selection yet but it should be sufficient for the 2.2 branch.
Applied to both branches, closing as FIXED: 2005-04-05 Sven Neumann <sven@gimp.org> * app/core/core-enums.[ch] * app/core/gimpimage.c (gimp_image_remove_layer, gimp_image_remove_channel): handle a floating selection attached to the layer or channel that is being removed. Fixes bug #168582 but doesn't handle floating selections attached to layer masks.