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

chrome/browser/ui/views/chrome_views_delegate.cc

Index: chrome/browser/ui/views/chrome_views_delegate.cc
diff --git a/chrome/browser/ui/views/chrome_views_delegate.cc b/chrome/browser/ui/views/chrome_views_delegate.cc
index 3d66274647c07cd4dde40679454665a7c93361e0..faabb72558f5ca1d3781b91763283ca0c03431b2 100644
--- a/chrome/browser/ui/views/chrome_views_delegate.cc
+++ b/chrome/browser/ui/views/chrome_views_delegate.cc
@@ -99,14 +99,14 @@ PrefService* GetPrefsForWindow(const views::Widget* window) {
 }
 
 #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
@@ -139,8 +139,28 @@ bool MonitorHasTopmostAutohideTaskbarForEdge(UINT edge, HMONITOR monitor) {
       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
@@ -159,13 +179,13 @@ 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;
 }
@@ -469,6 +489,13 @@ std::string ChromeViewsDelegate::GetApplicationName() {
 #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