After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 735972 - warnings (and eventually a crash) when dragging window from a workspace thumbnail
warnings (and eventually a crash) when dragging window from a workspace thumb...
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: overview
unspecified
Other Mac OS
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2014-09-03 14:26 UTC by Carlos Garnacho
Modified: 2014-09-11 22:58 UTC
See Also:
GNOME target: 3.14
GNOME version: ---


Attachments
workspace: Take a MetaWindow as argument to setReservedSlot() (2.42 KB, patch)
2014-09-03 14:28 UTC, Carlos Garnacho
none Details | Review
workspace: Take a MetaWindow as argument to setReservedSlot() (2.43 KB, patch)
2014-09-03 14:31 UTC, Carlos Garnacho
needs-work Details | Review
workspace: Take a MetaWindow as argument to setReservedSlot() (3.08 KB, patch)
2014-09-03 19:59 UTC, Carlos Garnacho
committed Details | Review
overview: Use a MetaWindow argument in window-drag-* signals/API (4.20 KB, patch)
2014-09-03 19:59 UTC, Carlos Garnacho
committed Details | Review

Description Carlos Garnacho 2014-09-03 14:26:57 UTC
When dragging windows from a workspace thumbnail at the right of the overview, I see warnings about gfloat properties on StWidgets/ClutterActors receiving NaNs, as a very indirect result of that, it eventually crashes on st_theme_node_reduce_border_radius() when receiving a NULL node.

I traced back the origin of NaNs to UnalignedLayoutStrategy.computeLayout():
                let width = window.width * s;
                let height = window.height * s;

both width/height would be undefined on the WindowClone, which would propagate the NaNs across multiple calculations and data. 

After some more investigation I figured out that the WindowClone object was not actually as per implemented in workspace.js, but rather came from code in workspaceThumbnail.js, which didn't contain all the data that workspace.js was expecting on it. That WindowClone seems to come from setReservedSlot(), set by overview.js during the DnD operation.

I'm not sure why there's 2 implementations of window thumbnails when code is so similar between both, but I suspect these objects aren't to be used interchangeably. I'm attaching a patch that makes setReservedSlot() take a MetaWindow, so the WindowClone is looked up from the list maintained by the Workspace object.
Comment 1 Carlos Garnacho 2014-09-03 14:28:05 UTC
Created attachment 285255 [details] [review]
workspace: Take a MetaWindow as argument to setReservedSlot()

And use it to lookup the local WindowClone that applies. Otherwise,
WorkspaceThumbnail.WindowClone objects may be mistakenly set, which
are not usable interchangeably with Workspace.WindowClone ones. This
may lead to several misbehaviors as fields available in the second
object but not in the first one are accessed, some those undefined
values get used in math ops, which result in NaNs over the place.
Comment 2 Carlos Garnacho 2014-09-03 14:31:30 UTC
Created attachment 285256 [details] [review]
workspace: Take a MetaWindow as argument to setReservedSlot()

And use it to lookup the local WindowClone that applies. Otherwise,
WorkspaceThumbnail.WindowClone objects may be mistakenly set, which
are not usable interchangeably with Workspace.WindowClone ones. This
may lead to several misbehaviors as fields available in the second
object but not in the first one are accessed, some those undefined
values get used in math ops, which result in NaNs over the place.
Comment 3 Florian Müllner 2014-09-03 14:42:35 UTC
Review of attachment 285256 [details] [review]:

Untested, but generally LGTM

::: js/ui/workspacesView.js
@@ +128,3 @@
     },
 
     _setReservedSlot: function(clone) {

There is another implementation of that method in ExtraWorkspaceView

@@ +131,3 @@
+        for (let i = 0; i < this._workspaces.length; i++) {
+            if (clone)
+                this._workspaces[i].setReservedSlot(clone.metaWindow);

You could write this as

let win = clone ? clone.metaWindow : null;
for (let i = 0; i < length; i++)
    this._workspaces[i].setReservedSlot(win);

to avoid re-testing for each workspace
Comment 4 Florian Müllner 2014-09-03 15:34:37 UTC
It think it is also worth considering changing Overview.{begin,end,cancelled}WindowDrag to MetaWindow - having the only parameter to 'window-drag-begin' and friends be something other than a window has always been odd API ...
(the fallout should be minimal - afaics, the only consumer who cares about the signal parameters is WorkspacesView, and likewise the overview methods are only called from Workspace/WorkspacesThumbnails)
Comment 5 Jasper St. Pierre (not reading bugmail) 2014-09-03 15:46:50 UTC
We need to stop passing around window actors and random clones in general when we mean windows.
Comment 6 Carlos Garnacho 2014-09-03 19:59:19 UTC
Created attachment 285284 [details] [review]
workspace: Take a MetaWindow as argument to setReservedSlot()

And use it to lookup the local WindowClone that applies. Otherwise,
WorkspaceThumbnail.WindowClone objects may be mistakenly set, which
are not usable interchangeably with Workspace.WindowClone ones. This
may lead to several misbehaviors as fields available in the second
object but not in the first one are accessed, some those undefined
values get used in math ops, which result in NaNs over the place.

Likewise, the similar functions in WorkspacesViewBase subclasses take
now MetaWindow arguments too.
Comment 7 Carlos Garnacho 2014-09-03 19:59:25 UTC
Created attachment 285285 [details] [review]
overview: Use a MetaWindow argument in window-drag-* signals/API

It is quite weird to have those calls/signals using WindowClone as an
argument, it is neater to pass MetaWindows around, and have each user
deal with their own representations of these.
Comment 8 Florian Müllner 2014-09-09 13:21:30 UTC
Review of attachment 285284 [details] [review]:

LGTM
Comment 9 Florian Müllner 2014-09-09 13:21:39 UTC
Review of attachment 285285 [details] [review]:

OK
Comment 10 Matthias Clasen 2014-09-11 22:55:21 UTC
Florian, Carlos is off this week, so maybe you should push these yourself.
Comment 11 Florian Müllner 2014-09-11 22:58:22 UTC
Attachment 285284 [details] pushed as b886656 - workspace: Take a MetaWindow as argument to setReservedSlot()
Attachment 285285 [details] pushed as 33e35f2 - overview: Use a MetaWindow argument in window-drag-* signals/API

Oh, OK ...