Avoid blocking while mapping an X11 window

ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h"
 
 #include <X11/extensions/shape.h>
 #include <X11/extensions/XInput2.h>
 #include <X11/Xatom.h>
 #include <X11/Xregion.h>
@@ skipping 169 matching lines @@
 // DesktopWindowTreeHostX11, public:
 
 DesktopWindowTreeHostX11::DesktopWindowTreeHostX11(
     internal::NativeWidgetDelegate* native_widget_delegate,
     DesktopNativeWidgetAura* desktop_native_widget_aura)
     : xdisplay_(gfx::GetXDisplay()),
       xwindow_(0),
       x_root_window_(DefaultRootWindow(xdisplay_)),
       atom_cache_(xdisplay_, kAtomsToCache),
       window_mapped_(false),
+      wait_for_map_(false),
       wait_for_unmap_(false),
       is_fullscreen_(false),
       is_always_on_top_(false),
       use_native_frame_(false),
       should_maximize_after_map_(false),
       use_argb_visual_(false),
       drag_drop_client_(NULL),
       native_widget_delegate_(native_widget_delegate),
       desktop_native_widget_aura_(desktop_native_widget_aura),
       content_window_(NULL),
@@ skipping 377 matching lines @@
 
 void DesktopWindowTreeHostX11::ShowMaximizedWithBounds(
     const gfx::Rect& restored_bounds) {
   ShowWindowWithState(ui::SHOW_STATE_MAXIMIZED);
   // Enforce |restored_bounds_in_pixels_| since calling Maximize() could have
   // reset it.
   restored_bounds_in_pixels_ = ToPixelRect(restored_bounds);
 }
 
 bool DesktopWindowTreeHostX11::IsVisible() const {
-  return window_mapped_ && !wait_for_unmap_;
+  return wait_for_map_ || (window_mapped_ && !wait_for_unmap_);
 }
 
 void DesktopWindowTreeHostX11::SetSize(const gfx::Size& requested_size) {
   gfx::Size size_in_pixels = ToPixelRect(gfx::Rect(requested_size)).size();
   size_in_pixels = AdjustSize(size_in_pixels);
   bool size_changed = bounds_in_pixels_.size() != size_in_pixels;
   XResizeWindow(xdisplay_, xwindow_, size_in_pixels.width(),
                 size_in_pixels.height());
   bounds_in_pixels_.set_size(size_in_pixels);
   if (size_changed) {
@@ skipping 594 matching lines @@
 
 void DesktopWindowTreeHostX11::ShowImpl() {
   ShowWindowWithState(ui::SHOW_STATE_NORMAL);
   native_widget_delegate_->OnNativeWidgetVisibilityChanged(true);
 }
 
 void DesktopWindowTreeHostX11::HideImpl() {
   if (IsVisible()) {
     XWithdrawWindow(xdisplay_, xwindow_, 0);
     wait_for_unmap_ = true;
+    wait_for_map_ = false;
   }
   native_widget_delegate_->OnNativeWidgetVisibilityChanged(false);
 }
 
 gfx::Rect DesktopWindowTreeHostX11::GetBounds() const {
   return bounds_in_pixels_;
 }
 
 void DesktopWindowTreeHostX11::SetBounds(
     const gfx::Rect& requested_bounds_in_pixel) {
   gfx::Rect bounds_in_pixels(requested_bounds_in_pixel.origin(),
                              AdjustSize(requested_bounds_in_pixel.size()));
   bool origin_changed = bounds_in_pixels_.origin() != bounds_in_pixels.origin();
   bool size_changed = bounds_in_pixels_.size() != bounds_in_pixels.size();
   XWindowChanges changes = {0};
   unsigned value_mask = 0;
 
-  delayed_resize_task_.Cancel();
 
   if (size_changed) {
+    // Only cancel the delayed resize task if we're already about to call
+    // OnHostResized in this function.
+    delayed_resize_task_.Cancel();
+
     // Update the minimum and maximum sizes in case they have changed.
     UpdateMinAndMaxSize();
 
     if (bounds_in_pixels.width() < min_size_in_pixels_.width() ||
         bounds_in_pixels.height() < min_size_in_pixels_.height() ||
         (!max_size_in_pixels_.IsEmpty() &&
          (bounds_in_pixels.width() > max_size_in_pixels_.width() ||
           bounds_in_pixels.height() > max_size_in_pixels_.height()))) {
       gfx::Size size_in_pixels = bounds_in_pixels.size();
       if (!max_size_in_pixels_.IsEmpty())
@@ skipping 659 matching lines @@
     XChangeProperty(
         xdisplay_, xwindow_, atom_cache_.GetAtom("_NET_WM_USER_TIME"),
         XA_CARDINAL, 32, PropModeReplace,
         reinterpret_cast<const unsigned char*>(&wm_user_time_ms), 1);
   }
 
   ui::X11EventSource* event_source = ui::X11EventSource::GetInstance();
   DCHECK(event_source);
 
   if (wait_for_unmap_) {
     // Block until our window is unmapped. This avoids a race condition when

[Tom (Use chromium acct), 2016/09/12 22:45:07]

We don't need lines 1900 to 1905 any more now that we don't
BlockUntilWindowMapped().  This means you must rename |wait_for_unmap_| too :)

You can get rid of X11EventSource::BlockUntilWindowUnmapped() too since this is
the only place it's used.

[dackerman, 2016/09/23 14:12:37]

I didn't remove this since I don't know in what scenario this might run, and
it's not necessary to fix this issue. I felt that the risk of creating a new bug
by removing it (when it wasn't necessary) was higher than the benefit of
removing all blocking.

Do you think there's a decent chance leaving this here would make things
actively worse? If not, it seems safer to make the smallest change possible
(especially in light of how hard this area of the code is to test/verify). That
way, we'll have more fine-grained control if things need to be reverted.

[Tom (Use chromium acct), 2016/09/23 18:35:48]

This code was only recently added to fix the problem where we indefinitely
BlockUntilWindowMapped, although it clearly didn't work in all situations. 
Since this CL removes BlockUntilWindowMapped, we don't need it any more, and it
only introduces the problem of waiting forever in BlockUntilWindowUnmapped
instead.

[dackerman, 2016/09/25 19:45:36]

Done.

     // remapping an unmapped window.
     event_source->BlockUntilWindowUnmapped(xwindow_);
     DCHECK(!wait_for_unmap_);
   }
 
   XMapWindow(xdisplay_, xwindow_);
-
-  // We now block until our window is mapped. Some X11 APIs will crash and
-  // burn if passed |xwindow_| before the window is mapped, and XMapWindow is
-  // asynchronous.
-  event_source->BlockUntilWindowMapped(xwindow_);
+  wait_for_map_ = true;
 }
 
 void DesktopWindowTreeHostX11::SetWindowTransparency() {
   compositor()->SetHostHasTransparentBackground(use_argb_visual_);
   window()->SetTransparent(use_argb_visual_);
   content_window_->SetTransparent(use_argb_visual_);
 }
 
 void DesktopWindowTreeHostX11::Relayout() {
   Widget* widget = native_widget_delegate_->AsWidget();
@@ skipping 202 matching lines @@
         default:
           NOTREACHED();
       }
 
       // If we coalesced an event we need to free its cookie.
       if (num_coalesced > 0)
         XFreeEventData(xev->xgeneric.display, &last_event.xcookie);
       break;
     }
     case MapNotify: {
+      wait_for_map_ = false;
       window_mapped_ = true;
 
       FOR_EACH_OBSERVER(DesktopWindowTreeHostObserverX11,
                         observer_list_,
                         OnWindowMapped(xwindow_));
 
       UpdateMinAndMaxSize();
 
       // Some WMs only respect maximize hints after the window has been mapped.
       // Check whether we need to re-do a maximization.
@@ skipping 163 matching lines @@
   if (linux_ui) {
     ui::NativeTheme* native_theme = linux_ui->GetNativeTheme(window);
     if (native_theme)
       return native_theme;
   }
 
   return ui::NativeThemeAura::instance();
 }
 
 }  // namespace views