GNOME Bugzilla – Bug 788882
Audit usages of meta_window_get_monitor indexing directly into an array
Last modified: 2018-06-28 16:35:03 UTC
As discussed in https://bugzilla.gnome.org/show_bug.cgi?id=788834 , `meta_window_get_monitor` might be called on an unmanaged window, and so it needs to return a value as opposed to crashing. There are some cases where we take the return value and use it to index directly into an array of monitors. This was dangerous since the return value is int and negative values are permitted (and will occur in the former case).
Created attachment 361426 [details] [review] windowMenu: Check return value of meta_window_get_monitor
Created attachment 361427 [details] [review] layout: Remove unused code
Review of attachment 361426 [details] [review]: In addition to the nit below, please use the full bugzilla URL in commit message (we'll probably migrate to gitlab soon, so #12345 will become ambiguous) ::: js/ui/windowMenu.js @@ +132,3 @@ + // meta_window_get_monitor can return -1, so handle that case + // appropriately. + if (monitorIndex !== -1) { Nit: We now end up with two separators and an empty section in between. Better as far as I can see (also in terms of indentation levels): let nMonitors = screen.get_n_monitors(); let monitorIndex = window.get_monitor(); if (nMonitors > 1 && monitorIndex > -1) { // unchanged }
Review of attachment 361427 [details] [review]: ::: js/ui/layout.js @@ -548,3 @@ }, - get focusIndex() { This one isn't unused
Created attachment 361468 [details] [review] windowManager: Check the return value of meta_window_get_monitor Makes the indentation nicer (though I note that in that section of the code, the indentation is actually incorrect anyway, maybe this should be fixed with a followup?)
Created attachment 361469 [details] [review] layout: Remove focusMonitor as it is unused focusIndex however, is still used (as a property, which I must not have picked up on).
Created attachment 361470 [details] [review] keyboard: Handle case where keyboardMonitor is unset
Review of attachment 361468 [details] [review]: I think this is all good...
Review of attachment 361470 [details] [review]: Ack
Review of attachment 361468 [details] [review]: Was this path ever hit? I don't think its needed: if there are any monitors, a window will always fallback to be assigned to one of them.
Review of attachment 361469 [details] [review]: Florian said this is still used (I suspect in extension(s)). Better just return null if focusIndex was -1.
(In reply to Jonas Ådahl from comment #12) > Review of attachment 361469 [details] [review] [review]: > > Florian said this is still used (I suspect in extension(s)). No, that was about focusIndex (which is still used for now in keyboard.js). focusMonitor may of course be used by extensions, but: - I don't actually know (none of ours in the gnome-shell-extensions module do) - replacing layoutManager.focusMonitor with layoutManager.monitors[layoutManager.focusIndex] looks easy enough So while I'm not in favor of blindly removing anything we are not using ourselves, I'm not opposed to remove this particular property.
Review of attachment 361469 [details] [review]: > So while I'm not in favor of blindly removing anything we are not using ourselves, I'm not opposed to remove this particular property. Alright. Marking as acn then.
Fixed with commit c9bf72c5, commit 19e864ed, commit 84d2d3fe, commit 87894696, commit 996dd741