Adding retry logic for applying desktop resolution changes to RDP session.

remoting/host/win/rdp_client_window.cc

 // Copyright (c) 2013 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 "remoting/host/win/rdp_client_window.h"
 
 #include <wtsdefs.h>
 
 #include <list>
 
+#include "base/bind.h"
 #include "base/lazy_instance.h"
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_local.h"
 #include "base/win/scoped_bstr.h"
 
 namespace remoting {
 
 namespace {
 
 // RDP session disconnect reason codes that should not be interpreted as errors.
 constexpr long kDisconnectReasonNoInfo = 0;
 constexpr long kDisconnectReasonLocalNotError = 1;
 constexpr long kDisconnectReasonRemoteByUser = 2;
 constexpr long kDisconnectReasonByServer = 3;
 
 // Maximum length of a window class name including the terminating nullptr.
 constexpr int kMaxWindowClassLength = 256;
 
 // Each member of the array returned by GetKeyboardState() contains status data
 // for a virtual key. If the high-order bit is 1, the key is down; otherwise, it
 // is up.
 constexpr BYTE kKeyPressedFlag = 0x80;
 
 constexpr int kKeyboardStateLength = 256;
 
+constexpr base::TimeDelta kReapplyResolutionPeriod =
+    base::TimeDelta::FromMilliseconds(250);
+
+// We want to try to reapply resolution changes for ~5 seconds (20 * 250ms).
+constexpr int kMaxResolutionReapplyAttempts = 20;
+
 // The RDP control creates 'IHWindowClass' window to handle keyboard input.
 constexpr wchar_t kRdpInputWindowClass[] = L"IHWindowClass";
 
 enum RdpAudioMode {
   // Redirect sounds to the client. This is the default value.
   kRdpAudioModeRedirect = 0,
 
   // Play sounds at the remote computer. Equivalent to |kRdpAudioModeNone| if
   // the remote computer is running a server SKU.
   kRdpAudioModePlayOnServer = 1,
@@ skipping 159 matching lines @@
   SendMessage(input_window, WM_KEYUP, VK_MENU, MAKELPARAM(1, alt | up));
 
   // Release 'Ctrl'.
   keyboard_state[VK_CONTROL] &= ~kKeyPressedFlag;
   keyboard_state[VK_LCONTROL] &= ~kKeyPressedFlag;
   CHECK(SetKeyboardState(keyboard_state));
   SendMessage(input_window, WM_KEYUP, VK_CONTROL, MAKELPARAM(1, control | up));
 }
 
 void RdpClientWindow::ChangeResolution(const ScreenResolution& resolution) {
+  // Stop any pending resolution changes.
+  apply_resolution_timer_.Stop();
   screen_resolution_ = resolution;
-  UpdateDesktopResolution();
+  HRESULT result = UpdateDesktopResolution();
+  if (FAILED(result)) {
+    LOG(ERROR) << "UpdateSessionDisplaySettings() failed: 0x" << std::hex
+               << result << std::dec;

[Sergey Ulanov, 2016/12/27 21:16:13]

I don't think you need std::dec here. Each LOG() creates a separate stream, so
std::hex won't be applied to other messages.

[joedow, 2017/01/04 15:47:44]

Done.

+  }
 }
 
 void RdpClientWindow::OnClose() {
   if (!client_.get()) {
     NotifyDisconnected();
     return;
   }
 
   // Request a graceful shutdown.
   mstsc::ControlCloseStatus close_status;
@@ skipping 153 matching lines @@
   if (FAILED(result))
     return LogOnCreateError(result);
 
   return 0;
 }
 
 void RdpClientWindow::OnDestroy() {
   client_.Release();
   client_9_.Release();
   client_settings_.Release();
+  apply_resolution_timer_.Stop();
 }
 
 HRESULT RdpClientWindow::OnAuthenticationWarningDisplayed() {
   LOG(WARNING) << "RDP: authentication warning is about to be shown.";
 
   // Hook window activation to cancel any modal UI shown by the RDP control.
   // This does not affect creation of other instances of the RDP control on this
   // thread because the RDP control's window is hidden and is not activated.
   window_activate_hook_ = WindowHook::Create();
   return S_OK;
@@ skipping 11 matching lines @@
 
   NotifyConnected();
   return S_OK;
 }
 
 HRESULT RdpClientWindow::OnLoginComplete() {
   VLOG(1) << "RDP: user successfully logged in.";
 
   user_logged_in_ = true;
 
-  // Apply pending screen size changes to the desktop.
-  UpdateDesktopResolution();
+  // Set up a timer to apply pending screen size changes to the desktop after a
+  // brief timeout.  Attempting to set the resolution now seems to fail
+  // consistently, but succeeds after a brief timeout.
+  if (client_9_) {
+    apply_resolution_timer_.Start(FROM_HERE,
+        kReapplyResolutionPeriod,
+        base::Bind(&RdpClientWindow::ReapplyDesktopResolution, this, 0));
+  }
 
   return S_OK;
 }
 
 HRESULT RdpClientWindow::OnDisconnected(long reason) {
   if (reason == kDisconnectReasonNoInfo ||
       reason == kDisconnectReasonLocalNotError ||
       reason == kDisconnectReasonRemoteByUser ||
       reason == kDisconnectReasonByServer) {
     VLOG(1) << "RDP: disconnected from " << server_endpoint_.ToString()
@@ skipping 59 matching lines @@
 }
 
 void RdpClientWindow::NotifyDisconnected() {
   if (event_handler_) {
     EventHandler* event_handler = event_handler_;
     event_handler_ = nullptr;
     event_handler->OnDisconnected();
   }
 }
 
-void RdpClientWindow::UpdateDesktopResolution() {
+HRESULT RdpClientWindow::UpdateDesktopResolution() {
   if (!client_9_ || !user_logged_in_) {
-    return;
+    return S_FALSE;
   }
 
   // UpdateSessionDisplaySettings() is poorly documented in MSDN and has a few
   // quirks that should be noted.
   // 1.) This method will only work when the user is logged into their session.
   // 2.) The method may return E_UNEXPECTED until some amount of time (seconds)
   //     have elapsed after logging in to the user's session.
-  // TODO(joedow): Investigate adding retry logic for applying the settings.
-  HRESULT result = client_9_->UpdateSessionDisplaySettings(
+  return client_9_->UpdateSessionDisplaySettings(
       screen_resolution_.dimensions().width(),
       screen_resolution_.dimensions().height(),
       screen_resolution_.dimensions().width(),
       screen_resolution_.dimensions().height(),
       /*ulOrientation=*/0,
       screen_resolution_.dpi().x(),
       screen_resolution_.dpi().y());
+}
+
+void RdpClientWindow::ReapplyDesktopResolution(int attempt_count) {
+  if (attempt_count > kMaxResolutionReapplyAttempts) {

[Sergey Ulanov, 2016/12/27 21:16:13]

Maybe just don't post the task after kMaxResolutionReapplyAttempts attempts?
That way attempt_count would have to be checked only in one place

[joedow, 2017/01/04 15:47:44]

Done.

+    LOG(WARNING) << "Unable to apply desktop resolution changes.";
+    return;
+  }
+
+  HRESULT result = UpdateDesktopResolution();
   if (FAILED(result)) {
-    LOG(ERROR) << "UpdateSessionDisplaySettings() failed: 0x" << std::hex
-               << result << std::dec;
+    // Only log errors on the last attempt to reduce log spam since a few errors
+    // can be expected and don't signal an actual failure.
+    if (attempt_count == kMaxResolutionReapplyAttempts) {
+      LOG(ERROR) << "UpdateSessionDisplaySettings() failed: 0x" << std::hex
+                 << result << std::dec;
+    }
+
+    apply_resolution_timer_.Start(FROM_HERE,

[Sergey Ulanov, 2016/12/27 21:16:13]

It may be easier to start a repeating timer and then stop it as soon as the
operation succeeds or 20 failed attempts.

[joedow, 2017/01/04 15:47:44]

I think that's a good call.  I've updated it to use a repeating timer.

+        kReapplyResolutionPeriod,
+        base::Bind(&RdpClientWindow::ReapplyDesktopResolution, this,
+                   attempt_count + 1));
   }
 }
 
 scoped_refptr<RdpClientWindow::WindowHook>
 RdpClientWindow::WindowHook::Create() {
   scoped_refptr<WindowHook> window_hook = g_window_hook.Pointer()->Get();
 
   if (!window_hook.get()) {
     window_hook = new WindowHook();
   }
@@ skipping 38 matching lines @@
   }
 
   // Close the window once all pending window messages are processed.
   HWND window = reinterpret_cast<HWND>(wparam);
   LOG(WARNING) << "RDP: closing a window: " << std::hex << window << std::dec;
   ::PostMessage(window, WM_CLOSE, 0, 0);
   return 0;
 }
 
 }  // namespace remoting