Created attachment 364980 [details] [review]
tweener: save extra handlers on target and remove them on destroy

Saving extra handlers we had using the wrapper as a property
of the object and delete them when resetting the object state.
Without doing this an handler could be called on a destroyed
target when this happens on the onComplete callback.

Created attachment 364981 [details] [review]
dash: nullify the label reference after destroying it

Created attachment 364982 [details] [review]
workspace: nullify the actor reference after destroying it

And avoid access to null instances, causing JS errors.

One more happens in:

wrapper@resource:///org/gnome/gjs/modules/_legacy.js:83:9
DashItemContainer<.animateOutAndDestroy/<.onComplete<@resource:///org/gnome/shell/ui/dash.js:207:32
_addHandler/params[name]@resource:///org/gnome/shell/ui/tweener.js:113:17
_callOnFunction@resource:///org/gnome/gjs/modules/tweener/tweener.js:203:13
_updateTweenByIndex@resource:///org/gnome/gjs/modules/tweener/tweener.js:332:9
_updateTweens@resource:///org/gnome/gjs/modules/tweener/tweener.js:345:18
_onEnterFrame@resource:///org/gnome/gjs/modules/tweener/tweener.js:360:10
_emit@resource:///org/gnome/gjs/modules/signals.js:126:27
ClutterFrameTicker<._onNewFrame@resource:///org/gnome/shell/ui/tweener.js:252:9
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
ClutterFrameTicker<._init/<@resource:///org/gnome/shell/ui/tweener.js:227:17

Basically on the onComplete callback of

    Tweener.addTween(this,
                        { childScale: 0.0,
                        childOpacity: 0,
                        time: DASH_ANIMATION_TIME,
                        transition: 'easeOutQuad',
                        onComplete: Lang.bind(this, function() {
                            this.destroy();
                        })
                        });

So probably Tweener.removeTweens(this); should be added on .destroy method of this or in the previous patch, just threating `oldHandler.apply(eventScope, oldParams);` as another handler to be stopped on target destroy.

One more

(gnome-shell:19883): Gjs-WARNING **: 13:47:05.237: JS ERROR: Error: Object .Gjs_DashItemContainer (0x5640ae9af5a0), has been already finalized. Impossible to set any property to it.
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:83:9
Dash<._redisplay@resource:///org/gnome/shell/ui/dash.js:822:17
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
_runDeferredWork@resource:///org/gnome/shell/ui/main.js:571:5
_runAllDeferredWork@resource:///org/gnome/shell/ui/main.js:580:9
queueDeferredWork/_deferredTimeoutId<@resource:///org/gnome/shell/ui/main.js:662:13

Review of attachment 364982 [details] [review]:

This looks extremely random - if we considered accessing those objects after calling destroy() valid, then we should consistently check for this.actor being null (and probably give a couple of other properties the same treatment), not just check one or two places.

However I really don't want to go down that route - we are fairly consistent through-out the platform that accessing an object after destroying it is not valid. By that notion, the patch is not fixing a bug, but papering over erroneous accesses of invalid objects elsewhere. Let's locate those places and fix those errors instead ...

::: js/ui/workspacesView.js
@@ +74,3 @@
     destroy: function() {
         this.actor.destroy();
+        this.actor = null;

There's not even a single check for this ...

Review of attachment 364981 [details] [review]:

::: js/ui/dash.js
@@ -180,2 @@
     destroy: function() {
-        if (this.label)

This check should really have been removed in commit 36e5ae4a25.

@@ +191,2 @@
             this.label.destroy();
+            this.label = null;

Instead of having two places where the label may be destroyed, I'd prefer:

animateOutAndDestroy: function() {
    this.label.hide();
    // ...
},

destroy: function() {
    this.label.destroy();
    this.parent();
}

Created attachment 365064 [details] [review]
tweener: save extra handlers on target and remove them on destroy

Patch cleanup

Created attachment 365065 [details] [review]
dash: destroy item container label just once on destruction

Updated patch as per requested in review, using destroy signal
to fix error mentioned in comment 4

Created attachment 365069 [details] [review]
dnd: nullify _dragActor after we've destroyed it, and avoid invalid access

We need to protect the call to Shell.util_set_hidden_from_pick when
the dragActor has been destroyed or we'll get errors.

Another trace we get all the times you drag an icon from the launcher is:

(gnome-shell:91270): Gjs-WARNING **: 16:33:50.747: JS ERROR: Error calling onComplete: Error: Object St.Icon (0x5594260d06c0), has been already deallocated - impossible to access to it. This might be caused by the fact that the object has been destroyed from C code using something such as destroy(), dispose(), or remove() vfuncs
_Draggable<._dragComplete@resource:///org/gnome/shell/ui/dnd.js:646:13
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
_Draggable<._finishAnimation@resource:///org/gnome/shell/ui/dnd.js:622:13
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
_Draggable<._onAnimationComplete@resource:///org/gnome/shell/ui/dnd.js:641:9
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
_addHandler/params[name]@resource:///org/gnome/shell/ui/tweener.js:116:17
_callOnFunction@resource:///org/gnome/gjs/modules/tweener/tweener.js:203:13
_updateTweenByIndex@resource:///org/gnome/gjs/modules/tweener/tweener.js:332:9
_updateTweens@resource:///org/gnome/gjs/modules/tweener/tweener.js:345:18
_onEnterFrame@resource:///org/gnome/gjs/modules/tweener/tweener.js:360:10
_emit@resource:///org/gnome/gjs/modules/signals.js:126:27
ClutterFrameTicker<._onNewFrame@resource:///org/gnome/shell/ui/tweener.js:260:9
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
ClutterFrameTicker<._init/<@resource:///org/gnome/shell/ui/tweener.js:235:17

Another way to avoid this is to just always nullify this._dragActor on destroy
signal or otherwise calling Shell.util_set_hidden_from_pick on the destroy
callback, but I think it's just pointless to do it when the destruction
has happened and this._dragAction points for sure to some invalid object.

Review of attachment 365065 [details] [review]:

Code looks good to me now.

Some style nits regarding the commit message:
 - subject should use sentence capitalization, so
   "dash: Destroy ..."

 - body should use full sentences ("No need to check ..."
   is missing a subject

 - body should use imperative mood ("Remove ..." instead
   of "Removed ...")


I would consider splitting this into two:

"dash: Make sure item labels are only destroyed once

Labels are currently destroyed from both animateOutAndDestroy()
and destroy(), which now (rightfully) triggers a gjs warning. As
the label is created unconditionally since commit 36e5ae4a250,
mirror that and always release it in destroy() and hide it
elsewhere."


"dash: Do not shadow ClutterActor's destroy()

Since commit ef1e27966db2 turned DashItemContainer into an StWidget,
the destroy() method overrides the ClutterActor method, which is at
the very least bad style. Instead, follow the usual pattern of using
a ::destroy handler."

Created attachment 365070 [details] [review]
dash: Make sure item labels are only destroyed once

split into two commits and using your commit messages...

Created attachment 365071 [details] [review]
dash: Do not shadow ClutterActor's destroy()

And this one too

Review of attachment 365069 [details] [review]:

This doesn't work in testing:

 - start dragging an icon, press escape; if you move
   the mouse, the dragged icon magically reappears
   until canceled again

 - drag a favorite and drop it elsewhere in the dash;
   this will trigger a different dragActor.destroy()
   call that isn't guarded by your patch ...

This will need more investigation and a more in-depth fix I'm afraid

::: js/ui/dnd.js
@@ +636,3 @@
         } else {
             dragActor.destroy();
+            this._dragActor = null;

This is also questionable style, as it uses the knowledge that the 'dragActor' parameter is always this._dragActor

Review of attachment 365070 [details] [review]:

LGTM

Review of attachment 365071 [details] [review]:

Dto.

Committed dash patches as commit 6c655a6 and commit 74683b7 and backports in gnome-3-26 branch

(In reply to Florian Müllner from comment #14)
> Review of attachment 365069 [details] [review] [review]:
> 
> This doesn't work in testing:
> 
>  - start dragging an icon, press escape; if you move
>    the mouse, the dragged icon magically reappears
>    until canceled again
> 
>  - drag a favorite and drop it elsewhere in the dash;
>    this will trigger a different dragActor.destroy()
>    call that isn't guarded by your patch ...
> 
> This will need more investigation and a more in-depth fix I'm afraid

So, we can go back to the more protective way, where we just unset the this._dragActor when it's destroyed (something like this https://pastebin.ubuntu.com/26121586/), which was also how I did initially, then I thought to be less .
The only side effect of it is that Shell.util_set_hidden_from_pick won't be called on it in case of _finishAnimation is called as destroy callback, but I guess is fine. Or we add a this._dragActorDestroyed

Another option would be to also nullify dragActor when it's destroyed in _dragActorDropped and _cancelDrag, but I think that all this would be more fragile when adding a new one, and I agree with you that it's not really nice to threat dragActor in _onAnimationComplete as this._dragActor.

Created attachment 365082 [details] [review]
dnd: nullify _dragActor after we've destroyed it, and avoid invalid access

We need to avoid that we use the _dragActor instance after that it has
been destroyed or we'll get errors. We now set it to null when this
happens, protecting any access to that.

Created attachment 365090 [details] [review]
appDisplay: don't try to close the popup menu that is already destroyed

This would lead to a JS error otherwise, as we might end up
in deleting actors that have been already destructed.

Such error can be easily reproduced by right-click on a favorite in the
launcher and selecting "Remove from favorite".

(gnome-shell:76512): Gjs-WARNING **: 19:06:31.986: JS ERROR: Error: Object St.Bin (0x55c798f76c70), has been already finalized. Impossible to get any property from it.
PopupMenu<.close@resource:///org/gnome/shell/ui/popupMenu.js:877:1
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
AppIconMenu<._init/<@resource:///org/gnome/shell/ui/appDisplay.js:1877:17
DashItemContainer<.animateOutAndDestroy/<.onComplete<@resource:///org/gnome/shell/ui/dash.js:199:32
_addHandler/params[name]@resource:///org/gnome/shell/ui/tweener.js:111:17
_callOnFunction@resource:///org/gnome/gjs/modules/tweener/tweener.js:203:13
_updateTweenByIndex@resource:///org/gnome/gjs/modules/tweener/tweener.js:332:9
_updateTweens@resource:///org/gnome/gjs/modules/tweener/tweener.js:345:18
_onEnterFrame@resource:///org/gnome/gjs/modules/tweener/tweener.js:360:10
_emit@resource:///org/gnome/gjs/modules/signals.js:126:27
ClutterFrameTicker<._onNewFrame@resource:///org/gnome/shell/ui/tweener.js:250:9
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
ClutterFrameTicker<._init/<@resource:///org/gnome/shell/ui/tweener.js:225:1

The error is caused because this._boxPointer.actor is deleted first on the call on destroy() and again during the unmap that is requested on destruction which triggers a call to "close()".

Created attachment 365166 [details] [review]
dnd: Declare restore location variables

Avoid warning on assigning restore variables

Created attachment 365170 [details] [review]
workspaceThumbnail: Ensure destruction of WindowClone happens once

Closing a window from the activities always triggers this error:

(gnome-shell:55671): Gjs-WARNING **: 20:24:53.960: JS ERROR: Error: Object Clutter.Actor (0x55ba880bcb60), has been already deallocated - impossible to access to it. This might be caused by the fact that the object has been destroyed from C code using something such as destroy(), dispose(), or remove() vfuncs
WindowClone<.destroy@resource:///org/gnome/shell/ui/workspaceThumbnail.js:145:9
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
WorkspaceThumbnail<._doRemoveWindow@resource:///org/gnome/shell/ui/workspaceThumbnail.js:384:9
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
WorkspaceThumbnail<._windowRemoved@resource:///org/gnome/shell/ui/workspaceThumbnail.js:455:9
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22

Avoiding to double-destroy it, fixes that.

Created attachment 365171 [details] [review]
workspaceThumbnail: Remove WindowClone's from _windows when destroyed

A WindowClone might be destroyed earlier than its MetaWindow counterpart
as its WindowActor could be destroyed earlier, thus when happens it's safer
to remove the clone from the windows list, without waiting the workspace
to request to do so.

WindowClone now emits a 'destroy' signals earlier enough and this now
triggers a _doRemoveWindow on WorkspaceThumbnail which will lead
to the proper cleanup; keeping track of the signal connections, in
order to avoid callback loops (not really harmful in this case, but
good practice).

Created attachment 365172 [details] [review]
workspace: Ensure destruction of WindowClone happens once

Another error triggered by closing a window from the activitiy view is:

(gnome-shell:55671): Gjs-WARNING **: 20:24:53.959: JS ERROR: Error: Object St.Widget (0x55ba88ae5fd0), has been already deallocated - impossible to access to it. This might be caused by the fact that the object has been destroyed from C code using something such as destroy(), dispose(), or remove() vfuncs
WindowClone<.destroy@resource:///org/gnome/shell/ui/workspace.js:311:9
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
Workspace<._doRemoveWindow@resource:///org/gnome/shell/ui/workspace.js:1461:9
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
Workspace<._windowRemoved@resource:///org/gnome/shell/ui/workspace.js:1559:9
wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22

Created attachment 365173 [details] [review]
workspace: Remove WindowClone's from _windows when destroyed

A WindowClone might be destroyed earlier than its MetaWindow counterpart
as its WindowActor could be destroyed earlier, thus when this happens
it's safer to remove the clone from the windows list, without waiting
the workspace to request to do so.

WindowClone now emits a 'destroy' signals earlier enough and this now
triggers a _doRemoveWindow on Workspace which will lead to the proper
cleanup; keeping track of the signal connections, in order to avoid
callback loops (not really harmful in this case, but good practice).

*** Bug 791378 has been marked as a duplicate of this bug. ***

*** Bug 791367 has been marked as a duplicate of this bug. ***

*** Bug 791388 has been marked as a duplicate of this bug. ***

*** Bug 791389 has been marked as a duplicate of this bug. ***

*** Bug 791367 has been marked as a duplicate of this bug. ***

*** Bug 791417 has been marked as a duplicate of this bug. ***

*** Bug 791436 has been marked as a duplicate of this bug. ***

Review of attachment 365166 [details] [review]:

lgtm

Review of attachment 365082 [details] [review]:

::: js/ui/dnd.js
@@ +665,3 @@
+        }
+
+        this._dragActorDestroyed = false;

I think it'd be much clearer if we added a state instead of relying on multiple booleans for figuring out the state. I suppose it should be

INIT, DRAGGING, CANCELLED

INIT: well, initially and after completed
DRAGGING: after start dragging where this._dragActor != null
CANCELLED: set by _cancelDrag() and in the destroy handler

then DRAGGING implies that we have a drag actor, cancelled implies that we don't want to start a new one etc

Review of attachment 365090 [details] [review]:

lgtm

Review of attachment 365170 [details] [review]:

::: js/ui/workspaceThumbnail.js
@@ +139,3 @@
     destroy: function () {
+        if (this._destroyed)
+            return;

This looks suspicious.. we shouldn't "destroy" something twice. I suggest that the _doRemoveWindow() gets a parameter that says whether it should call destroy() on the lcone, and make it so that the cloneDestroyedId closure calls _doRemoveWindow() with that parameter set to false (while the other callers call it with true)

Review of attachment 365172 [details] [review]:

::: js/ui/workspace.js
@@ +306,3 @@
     destroy: function () {
+        if (this._destroyed)
+            return;

Same here as with the workspace thumbnail; we should only call destroy(), meaning the "destroy" handler that you add in the next patch should not try to destroy again.

Review of attachment 365170 [details] [review]:

My main gripe here is that when using the delegate pattern, destroying the delegate actor and calling the destroy() method should be interchangeable

Review of attachment 365173 [details] [review]:

Nit:
"without waiting the workspace" => "without waiting *for* the workspace"

Review of attachment 365082 [details] [review]:

I agree with Jonas that adding another boolean to the mix doesn't look very compelling. Also some nits:

::: js/ui/dnd.js
@@ +228,2 @@
                 return this._updateDragPosition(event);
+            } else if (this._dragActor == null && !this._dragActorDestroyed) {

The ::destroy handler doesn't immediately unset dragInProgress, so this should be added to the conditions to not call maybeStartDrag() unintentionally. Alternatively you can nest the if block:

  if (this._dragInProgress) {
      if (this._dragActor)
          return this._updateDragPosition(event);
  } else if (...)

Or move the dragActor check into _updateDragPosition:

  if (!this._dragActor)
      return Clutter.EVENT_PROPAGATE;

@@ +345,3 @@
         }
 
+        this._dragActorDestroyId = this._dragActor.connect('destroy', () => {

Not a fan of connecting two handlers to the same signal - given that _finishAnimation() is a noop when there is no ongoing animation, can we do:

  // Cancel ongoing animation (if any)
  this._finishAnimation();

  this._dragActor = null;
  ...

Created attachment 366961 [details] [review]
dnd: nullify _dragActor after we've destroyed it, and avoid invalid access

Updated the dnd patch following Jonas comment.

Florian, as per that I don't see the point of keeping _dragInProgress around, while
updating the drag position doesn't seem needed when cancelled.
While I did what you suggested on 'destroy' signal.

Attachment 365090 [details] pushed as 35eac69 - appDisplay: don't try to close the popup menu that is already destroyed
Attachment 365166 [details] pushed as 69da686 - dnd: Declare restore location variables

Created attachment 367072 [details] [review]
workspaceThumbnail: Disconnect from window signals on destruction

Patch updated not to use this._destroyed

Created attachment 367073 [details] [review]
workspaceThumbnail: Remove WindowClone's from _windows when destroyed

Use destructured optional parameters to allow passing cloneDestroy to _doRemoveWindow

Created attachment 367074 [details] [review]
workspace: Disconnect from window signals on destruction

Patch updated not to use this._destroyed

Created attachment 367075 [details] [review]
workspace: Remove WindowClone's from _windows when destroyed

A WindowClone might be destroyed earlier than its MetaWindow counterpart
as its WindowActor could be destroyed earlier, thus when happens it's safer
to remove the clone from the windows list, without waiting for the workspace
to request to do so.

WindowClone now emits a 'destroy' signals earlier enough and this now
triggers a _doRemoveWindow on WorkspaceThumbnail which will lead
to the proper cleanup; keeping track of the signal connections, in
order to avoid callback loops (not really harmful in this case, but
good practice).

Lets continue at https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/4

Something is fixed at commit d0bdea3178bcec2c510d847a534ab01773bdbda0 and commit d9a1434ae978465a016ec987094131ef6d325eaf

Due to the git commits, 2 commits is contained by gnome-shell 3.29.91.
But this issue is still reproduced on 3.19.91.

Created attachment 373567 [details]
Stack is still reproduced on gnome-shell 3.29.91

Sorry for typo, gnome-shell is 3.29.91.
Please check the log.