Confirmed.  This happens because, in gimp_display_shell_canvas_tool_events(), in
gimpdisplayshell-callbacks.c, the function tool_manager_button_release_active()
is called before the keyboard and pointer are ungrabbed.  The obvious thing to
try is to move the ungrabs up a few lines.  Will that break anything?

That's a long-standing and well-known bug so this is most certainly a duplicate.

Well, I did search for dups before confirming, and didn't find anything using
the terms "focus" or "grab" either in summaries and comments.  Any suggestions?

We cannot simply release the grab before calling the tool's
button_release() because that would enable event dispatching
and you could do evil things while the (maybe lengthy) tool
operation is performed.

So to fix this, we probably need to replace the server-wide
grabs by app-wide ones before calling the tool's
button_release(). Not entirely sure about that, though.

This is a special case of the bug "Allow tool operations to be cancelled" (bug
#113564).

I don't know if allowing X focus events and the like to go through is an easier
problem to fix. By the way, what evil stuff can we do, mitch?

Cheers,
Dave.

Is deleting the layer while the blend tool
renders a gradient to it evil enough? ;-)

I don't understand the comment about
"X focus events and the like to go through".

What if we did something like this?

            if (! shell->space_pressed && ! shell->space_release_pending)
              gdk_display_keyboard_ungrab (gdk_display, time);

            gdk_display_pointer_ungrab (gdk_display, time);

            if (active_tool &&
                (! gimp_image_is_empty (gimage) ||
                 gimp_tool_control_handles_empty_image (active_tool->control)))
              {
                if (gimp_tool_control_is_active (active_tool->control))
                  {
                    gtk_grab_add (GTK_WIDGET (canvas));

                    tool_manager_button_release_active (gimp,
                                                        &image_coords,
                                                        time, state,
                                                        gdisp);

                    gtk_grab_remove (GTK_WIDGET (canvas));
                  }
              }

That's exactly what I meant. Please don't just apply the patch,
because this is the EEKyest part of GIMP and there are probably
other things needed to make it work this way.

So -- just trying to understand -- are the issues related to multithreading, or
do the tools somehow stop and process user input while they are in the middle of
doing the button release callback?

GIMP doesn't do multithreading except if compiled with --enable-mp and even then
the usage of threads are limited to very few places.

Do we really need to explain the concept of the GMainLoop here? In order to
display progress and to update the UI, the main-loop is driven from the tool
operation. This also means that user input is processed.

If I understood all issues involved with gimp_display_shell_tool_events()
I'd simply have stated them above. Fact is that almost each change to this
function broke other things. This particular change involves invariants
about which window has the focus and when exactly the focus leaves
the display, and I'm pretty sure there are other issues as well.

Thank you.  Now I understand.  I had forgotten about the progress update.

I've been experimenting with the change described in comment #7, by setting up a
6000x6000 image and doing a shapeburst gradient fill, which takes about five
minutes.  Nothing I have tried (iconifying, resizing windows, running plug-ins
while the tool is working) has made GIMP crash or even seem to misbehave.  Any
suggestions of more subtle ways it might be likely to break?

The first obligation of any interactive program is to be a good citizen on the
desktop, so we should really try to improve this behavior if it's possible.

Try playing with modifiers which affect the tool behavior before, while
and after releasing the mouse button. Focus other windows the same
way and try with both focus-follows-mouse and click-to-focus.
And try all (well, many :-) permutations of these.

If the tool options are always in the correct state after the
operation is finished, we're safe. Also, tool options
*must*not*change* until the tool operation is finished.

Okay, I have tried a bunch of those sorts of things, with several tools, again
with no evil effects (other than leaving me twiddling my thumbs while the image
redraws).  It seems to me that the greatest danger comes from plugins, which are
not affected by the gtk_grab_add().  If a plugin could alter tool options, then
that would be an obvious path to trouble -- but that's no longer possible now
that plugins operate on a separate context, right?  

We should either try this now, early in the unstable cycle when there is plenty
of time for problems to show up and be dealt with, or make a plan to try it at
some specific time, or come up with some concrete thing that should be done
before trying it.  This is a problem that doesn't come up very often for most
people, but when it does it can be worse than a crash, so we shouldn't just let
it slide.

Actually, we would like to fix this in 2.2, but of course it should be tried in
HEAD first.

Created attachment 36027 [details] [review]
patch for app/display/gimpdisplayshell-callbacks.c

As requested by mitch, patch implementing the change described in comment #7.

mitch, is this match good to commit?

ping!

Yes, sure, go ahead and commit it to CVS HEAD so that it gets some testing.

In HEAD:

2005-02-09  Bill Skaggs  <weskaggs@primate.ucdavis.edu>

	* app/display/gimpdisplayshell-callbacks.c 
	(gimp_display_shell_canvas_tool_events):  For testing, apply
	patch switching display-wide grab to app-wide grab while
	handling button-release event, see bug #162823.

2005-02-22  Sven Neumann  <sven@gimp.org>

	Merged from HEAD:

	* app/display/gimpdisplayshell-callbacks.c
	(gimp_display_shell_canvas_tool_events): switch from display-wide
	grab to application-wide grab while handling button-release event.
	Fixes bug #162823.