GNOME Bugzilla – Bug 735972
warnings (and eventually a crash) when dragging window from a workspace thumbnail
Last modified: 2014-09-11 22:58:33 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.
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.
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.
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
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)
We need to stop passing around window actors and random clones in general when we mean windows.
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.
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.
Review of attachment 285284 [details] [review]: LGTM
Review of attachment 285285 [details] [review]: OK
Florian, Carlos is off this week, so maybe you should push these yourself.
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 ...