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

remoting/host/win/rdp_client_window.cc

Index: remoting/host/win/rdp_client_window.cc
diff --git a/remoting/host/win/rdp_client_window.cc b/remoting/host/win/rdp_client_window.cc
index 47a50b30f92e66fc9ea7fc82ee70d079286b58e1..aab871de4248ce205c10eb16666276d01c7253c4 100644
--- a/remoting/host/win/rdp_client_window.cc
+++ b/remoting/host/win/rdp_client_window.cc
@@ -8,7 +8,9 @@
 
 #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"
@@ -36,6 +38,12 @@ 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";
 
@@ -215,8 +223,14 @@ void RdpClientWindow::InjectSas() {
 }
 
 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() {
@@ -390,6 +404,7 @@ void RdpClientWindow::OnDestroy() {
   client_.Release();
   client_9_.Release();
   client_settings_.Release();
+  apply_resolution_timer_.Stop();
 }
 
 HRESULT RdpClientWindow::OnAuthenticationWarningDisplayed() {
@@ -421,8 +436,14 @@ HRESULT RdpClientWindow::OnLoginComplete() {
 
   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;
 }
@@ -502,9 +523,9 @@ void RdpClientWindow::NotifyDisconnected() {
   }
 }
 
-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
@@ -512,8 +533,7 @@ void RdpClientWindow::UpdateDesktopResolution() {
   // 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(),
@@ -521,9 +541,27 @@ void RdpClientWindow::UpdateDesktopResolution() {
       /*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));
   }
 }