Created attachment 163380 [details] [review]
[dnd] If actor destroyed, cancel drag

Review of attachment 163380 [details] [review]:

The patch looks good and fixes the problem, the only "flaw" is this warning:

Window manager warning: Log level 16: gsignal.c:2390: instance `0x9e104d0' has no handler with id `1999'

This is obviously way better than crashing, so unless there's a quick fix you should push the change.

A temporary fix crash that leaves a warning like that can be OK, but basically any warning like that is *not* OK, and this bug needs to be left open until we have a fix that doesn't warn. 

Being sloppy about serious warnings means that you can't distinguish errors that we need to fix from stuff that's "expected" - if we're always getting stray "instance '...' has no handler with id '...'" warnings, then we'll never notice if we start disconnecting junk signal handler ids.

(Yes, we are sloppy about leaving some other warnings around for a long time, like the warnings that the window triggers. That's bad, and we shouldn't be adding more such.)

Comment on attachment 163380 [details] [review]
[dnd] If actor destroyed, cancel drag

> A temporary fix crash that leaves a warning like that can be OK, but
> basically any warning like that is *not* OK, and this bug needs to be
> left open until we have a fix that doesn't warn. 

Marking as needs-work based on Owen's comment then.

Created attachment 171207 [details] [review]
[dnd] If actor destroyed, cancel drag

Review of attachment 171207 [details] [review]:

This is a pretty intrusive patch - nothing wrong with that per se, but the already complicated code gets quite a bit harder to read (e.g. I doubt that we really need two new status properties to handle the case of the actor going away during drag).

::: js/ui/dnd.js
@@ +88,2 @@
         this.actor.connect('destroy', Lang.bind(this, function() {
+            this._actorDestoyed = true;

Typo: dest_r_oyed

@@ +88,3 @@
         this.actor.connect('destroy', Lang.bind(this, function() {
+            this._actorDestoyed = true;
+            if (!this._acceptDropInProgress) {

Uhm - disabling the handler during drop and then calling its code manually elsewhere is rather confusing.  It would be much cleaner to check this._actorDestroyed during drop handling and cancel when necessary.

@@ +332,2 @@
         // If we are dragging, update the position
+        if (this._dragActor && this._dragInProgress) {

Why is this necessary?

@@ +431,3 @@
+                        this._acceptDropInProgress = false;
+                        this._restoreDragActor(event.get_time());
+                        return true;

Not that the code was very clear before, but the replacement looks wrong - it does something else than what the comment above implies and does handle the case where this._restoreOnSuccess == false differently if dragOrigParent is set.

@@ +436,3 @@
+                    this._emitDragEnd(this._dragActor, event.get_time(), false);
+                    if (this._actorDestoyed)
+                        this.disconnectAll();

Again, this looks like it changes dnd behavior: if this._dragOrigParent is set, this._emitDragEnd() will reparent/reposition/resize the dragActor, while previously this._dragActor had been destroyed at this point.

@@ +501,3 @@
     },
 
+    _emitDragEnd: function (dragActor, eventTime, dragActorDestroying) {

_emitDragEnd? This sounds like all it does is emit a signal and possibly update some internal status variables, but for a function which changes an actor's parent/scale/position, this is a clear misnomer.

Also the use of a boolean parameter to change the behavior of the function completely is rather unintuitive.

Created attachment 171445 [details] [review]
[dnd] If actor destroyed, cancel drag

> It would be much cleaner to check this._actorDestroyed during drop handling and cancel when necessary.
done.
Now 'drag-end' can be emited during call to acceptDrop (I try avoid this In previous patch. It was source of complexity)

Comment on attachment 171445 [details] [review]
[dnd] If actor destroyed, cancel drag

(In reply to comment #7)
> Now 'drag-end' can be emited during call to acceptDrop (I try avoid this In
> previous patch. It was source of complexity)

The code is much cleaner, but I get this exception repeatedly:

    JS ERROR: !!!   Exception was: TypeError: currentDraggable is null
    JS ERROR: !!!     lineNumber = '54'
    JS ERROR: !!!     fileName = '/home/florian/src/gnome-shell/js/ui/dnd.js'
    JS ERROR: !!!     stack = '([object _private_Clutter_Rectangle],[object _private_Clutter_Event])@/home/florian/src/gnome-shell/js/ui/dnd.js:54
'
    JS ERROR: !!!     message = 'currentDraggable is null'


Keyboard and pointer remain grabbed, so killing the shell is the only way out.

(Tested with 'zenity --info & sleep 5; kill %1', then dragging the zenity window in the overview until it goes away)

Comment on attachment 171445 [details] [review]
[dnd] If actor destroyed, cancel drag

Nevermind, I had some crap in my local tree ...

Review of attachment 171445 [details] [review]:

The patch fixes the crash, but I think it can still be improved:

 - Cancelling the drag completely on destruction of the drag actor means that the pointer grab
   is released. Now when the user releases the mouse button, the event is processed by the actor
   beneath the pointer - that's certainly not what the user intended, so it's wrong.

 - I get this:
   Window manager warning: Log level 8: g_object_unref: assertion `G_IS_OBJECT (object)' failed

::: js/ui/dnd.js
@@ +466,3 @@
         this._animationInProgress = true;
         // No target, so snap back
+        if (animate)

The animate parameter is actually this._actorDestroyed, right?

Further, with the additional conditions you added to _onAnimationComplete(), the function is reduced to

  global.unset_cursor();
  this.emit('drag-end', eventTime, false);
  this._dragComplete();

As noted before, I think the call to this._dragComplete() is actually wrong at this point (see how the Escape key is handled), which leaves two lines of code - in my opinion it is better to actually duplicate those lines (as already done in _dragActorDropped()) instead of calling _onAnimationComplete() without having animated anything. So what I propose is something like:

if (this._actorDestroyed) {
  global.unset_cursor();
  this.emit('drag-end', eventTime, false);
  return;
}

this._animationInProgress = true;
...

(or use an else part if you prefer ...)

Created attachment 172982 [details] [review]
[dnd] If actor destroyed, cancel drag

> - Cancelling the drag completely on destruction of the drag actor means that
> the pointer grab is released.
fixed

> I get this: Window manager warning: Log level 8: g_object_unref: assertion
> `G_IS_OBJECT (object)' failed
I think, It is not related with this patch. (I get the bunch of such messages, when I open overview)

Review of attachment 172982 [details] [review]:

Code looks good and works fine - even the warning seems to have been fixed by recent gjs updates! So OK to commit with a slightly more verbose commit message, e.g.

[dnd] Handle destruction of drag actor during drag

The shell crashes if the drag actor is destroyed during the drag
operation. Cancel the drag gracefully in this case.