Don't check autohide taskbar for WS_EX_TOPMOST when we are querying the autohide state.

chrome/browser/ui/views/chrome_views_delegate.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 "chrome/browser/ui/views/chrome_views_delegate.h"
 
 #include <memory>
 
 #include "base/location.h"
 #include "base/logging.h"
@@ skipping 81 matching lines @@
 PrefService* GetPrefsForWindow(const views::Widget* window) {
   Profile* profile = GetProfileForWindow(window);
   if (!profile) {
     // Use local state for windows that have no explicit profile.
     return g_browser_process->local_state();
   }
   return profile->GetPrefs();
 }
 
 #if defined(OS_WIN)
-bool MonitorHasTopmostAutohideTaskbarForEdge(UINT edge, HMONITOR monitor) {
+bool MonitorHasAutohideTaskbarForEdge(UINT edge, HMONITOR monitor) {
   APPBARDATA taskbar_data = { sizeof(APPBARDATA), NULL, 0, edge };
   taskbar_data.hWnd = ::GetForegroundWindow();
 
   // TODO(robliao): Remove ScopedTracker below once crbug.com/462368 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
-          "462368 MonitorHasTopmostAutohideTaskbarForEdge"));
+          "462368 MonitorHasAutohideTaskbarForEdge"));
 
   // MSDN documents an ABM_GETAUTOHIDEBAREX, which supposedly takes a monitor
   // rect and returns autohide bars on that monitor.  This sounds like a good
   // idea for multi-monitor systems.  Unfortunately, it appears to not work at
   // least some of the time (erroneously returning NULL) and there's almost no
   // online documentation or other sample code using it that suggests ways to
   // address this problem. We do the following:-
   // 1. Use the ABM_GETAUTOHIDEBAR message. If it works, i.e. returns a valid
   //    window we are done.
   // 2. If the ABM_GETAUTOHIDEBAR message does not work we query the auto hide
@@ skipping 12 matching lines @@
 
     taskbar_data.hWnd = ::FindWindow(L"Shell_TrayWnd", NULL);
     if (!::IsWindow(taskbar_data.hWnd))
       return false;
 
     SHAppBarMessage(ABM_GETTASKBARPOS, &taskbar_data);
     if (taskbar_data.uEdge == edge)
       taskbar = taskbar_data.hWnd;
   }
 
-  if (::IsWindow(taskbar) &&
-      (GetWindowLong(taskbar, GWL_EXSTYLE) & WS_EX_TOPMOST)) {
+  // We retrieve the autohide information in a worker thread to avoid
+  // reentrancy issues introduced by the SHAppBarMessage API. This introduces
+  // subtle timing bugs like the one below:

[Peter Kasting, 2016/12/02 21:11:30]

Nit: Since this partly duplicates your later comment, maybe just say "There is a
potential race condition here:" in place of this paragraph.

[ananta, 2016/12/03 00:07:25]

Done.

+  // 1. A maximized chrome window is fullscreened.
+  // 2. It is switched back to maximized.
+  // 3. In the process the window gets a WM_NCCACLSIZE message which calls us to
+  //    get the autohide state.
+  // 4. The worker thread is invoked and the main thread is suspended. The
+  // worker

[Peter Kasting, 2016/12/02 21:11:30]

Nit: Rewrap

I might remove "and the main thread is suspended" from here, as it implies the
thread is always suspended, and since that isn't what you mean, the text below
seems clear enough.

[ananta, 2016/12/03 00:07:25]

Done.

+  //    thread calls the API to get the autohide state. On Windows versions
+  //    earlier than Windows 7 taskbars could easily be always on top or not.
+  //    This meant that we only want to look for taskbars which have the topmost
+  //    bit set.  However this causes problems in cases where the window on the
+  //    main thread is still in the process of switching away from fullscreen.
+  //    In this case the taskbar would not have the topmost bit set.

[Peter Kasting, 2016/12/02 21:11:30]

Nit: Maybe "would not" -> "might not yet"?

[ananta, 2016/12/03 00:07:25]

Done.

+  // 5. The main thread resumes and does not leave space for the taskbar and
+  //    hence it does not popup up when hovered on.

[Peter Kasting, 2016/12/02 21:11:30]

Nit: popup -> pop; remove "on"?

[ananta, 2016/12/03 00:07:25]

Done.

+  //
+  // To address point 4 above, it is best to not check for the WS_EX_TOPMOST
+  // window style on the taskbar, as starting from Windows 7, the topmost
+  // style is always set. We don't support XP and Vista anymore.
+  if (::IsWindow(taskbar)) {
     if (MonitorFromWindow(taskbar, MONITOR_DEFAULTTONEAREST) == monitor)
       return true;
     // In some cases like when the autohide taskbar is on the left of the
     // secondary monitor, the MonitorFromWindow call above fails to return the
     // correct monitor the taskbar is on. We fallback to MonitorFromPoint for
     // the cursor position in that case, which seems to work well.
     POINT cursor_pos = {0};
     GetCursorPos(&cursor_pos);
     if (MonitorFromPoint(cursor_pos, MONITOR_DEFAULTTONEAREST) == monitor)
       return true;
   }
   return false;
 }
 
 int GetAppbarAutohideEdgesOnWorkerThread(HMONITOR monitor) {
   DCHECK(monitor);
 
   int edges = 0;
-  if (MonitorHasTopmostAutohideTaskbarForEdge(ABE_LEFT, monitor))
+  if (MonitorHasAutohideTaskbarForEdge(ABE_LEFT, monitor))
     edges |= views::ViewsDelegate::EDGE_LEFT;
-  if (MonitorHasTopmostAutohideTaskbarForEdge(ABE_TOP, monitor))
+  if (MonitorHasAutohideTaskbarForEdge(ABE_TOP, monitor))
     edges |= views::ViewsDelegate::EDGE_TOP;
-  if (MonitorHasTopmostAutohideTaskbarForEdge(ABE_RIGHT, monitor))
+  if (MonitorHasAutohideTaskbarForEdge(ABE_RIGHT, monitor))
     edges |= views::ViewsDelegate::EDGE_RIGHT;
-  if (MonitorHasTopmostAutohideTaskbarForEdge(ABE_BOTTOM, monitor))
+  if (MonitorHasAutohideTaskbarForEdge(ABE_BOTTOM, monitor))
     edges |= views::ViewsDelegate::EDGE_BOTTOM;
   return edges;
 }
 #endif
 
 #if defined(USE_ASH)
 void ProcessAcceleratorNow(const ui::Accelerator& accelerator) {
   // TODO(afakhry): See if we need here to send the accelerator to the
   // FocusManager of the active window in a follow-up CL.
   ash::WmShell::Get()->accelerator_controller()->Process(accelerator);
@@ skipping 283 matching lines @@
   return content::GetContextFactory();
 }
 
 std::string ChromeViewsDelegate::GetApplicationName() {
   return version_info::GetProductName();
 }
 
 #if defined(OS_WIN)
 int ChromeViewsDelegate::GetAppbarAutohideEdges(HMONITOR monitor,
                                                 const base::Closure& callback) {
+  // We use the SHAppBarMessage API to get the taskbar autohide state. This API

[Peter Kasting, 2016/12/02 21:11:30]

Nit: This whole comment probably belongs directly above the
PostTaskAndReplyWithResult() call.

[ananta, 2016/12/03 00:07:25]

Done.

+  // spins a modal loop which could cause callers to be reentered. This may not
+  // be desirable and to avoid that we retrieve the taskbar state in a worker

[Peter Kasting, 2016/12/02 21:11:30]

Nit: I'd remove "This may not be desirable and", which is implied (and it's
definitely undesirable, not just maybe)

[ananta, 2016/12/03 00:07:25]

Done.

+  // thread. This fixes the reentrancy problems but introduces subtle timing
+  // issues like the one described in the
+  // ChromeViewsDelegate::OnGotAppbarAutohideEdges() function.

[Peter Kasting, 2016/12/02 21:11:30]

Nit: I'd omit this last sentence, as that's really a concern in that function
itself, not to readers here.

[ananta, 2016/12/03 00:07:25]

Done.

+
   // Initialize the map with EDGE_BOTTOM. This is important, as if we return an
   // initial value of 0 (no auto-hide edges) then we'll go fullscreen and
   // windows will automatically remove WS_EX_TOPMOST from the appbar resulting
   // in us thinking there is no auto-hide edges. By returning at least one edge
   // we don't initially go fullscreen until we figure out the real auto-hide
   // edges.
   if (!appbar_autohide_edge_map_.count(monitor))
     appbar_autohide_edge_map_[monitor] = EDGE_BOTTOM;
   if (monitor && !in_autohide_edges_callback_) {
     base::PostTaskAndReplyWithResult(
@@ skipping 41 matching lines @@
   return ViewsDelegate::GetDialogRelatedButtonHorizontalSpacing();
 }
 
 #if !defined(USE_ASH)
 views::Widget::InitParams::WindowOpacity
 ChromeViewsDelegate::GetOpacityForInitParams(
     const views::Widget::InitParams& params) {
   return views::Widget::InitParams::OPAQUE_WINDOW;
 }
 #endif