diff --git a/printing/printing_context_win_unittest.cc b/printing/printing_context_win_unittest.cc index a9f1dc5cbc48e0..aab83f3e2a1bf0 100644 --- a/printing/printing_context_win_unittest.cc +++ b/printing/printing_context_win_unittest.cc @@ -6,6 +6,8 @@ #include "base/bind.h" #include "base/message_loop/message_loop.h" +#include "base/win/scoped_handle.h" +#include "base/win/scoped_hdc.h" #include "printing/backend/printing_info_win.h" #include "printing/backend/win_helper.h" #include "printing/print_settings.h" @@ -34,6 +36,18 @@ class PrintingContextTest : public PrintingTest, PrintingContext::Result result_; }; +namespace { +struct FreeHandleTraits { + typedef HANDLE Handle; + static void CloseHandle(HANDLE handle) { GlobalFree(handle); } + static bool IsHandleValid(HANDLE handle) { return handle != NULL; } + static HANDLE NullHandle() { return NULL; } +}; +typedef base::win::GenericScopedHandle + ScopedGlobalAlloc; +} + class MockPrintingContextWin : public PrintingContextSytemDialogWin { public: MockPrintingContextWin(Delegate* delegate) @@ -58,41 +72,31 @@ class MockPrintingContextWin : public PrintingContextSytemDialogWin { if (!printer.OpenPrinter(printer_name.c_str())) return E_FAIL; - scoped_ptr buffer; const DEVMODE* dev_mode = NULL; - HRESULT result = S_OK; lppd->hDC = NULL; lppd->hDevMode = NULL; lppd->hDevNames = NULL; PrinterInfo2 info_2; - if (info_2.Init(printer.Get())) { + if (info_2.Init(printer.Get())) dev_mode = info_2.get()->pDevMode; - } - if (!dev_mode) { - result = E_FAIL; - goto Cleanup; - } - - lppd->hDC = CreateDC(L"WINSPOOL", printer_name.c_str(), NULL, dev_mode); - if (!lppd->hDC) { - result = E_FAIL; - goto Cleanup; - } + if (!dev_mode) + return E_FAIL; + + base::win::ScopedCreateDC hdc( + CreateDC(L"WINSPOOL", printer_name.c_str(), NULL, dev_mode)); + if (!hdc.Get()) + return E_FAIL; size_t dev_mode_size = dev_mode->dmSize + dev_mode->dmDriverExtra; - lppd->hDevMode = GlobalAlloc(GMEM_MOVEABLE, dev_mode_size); - if (!lppd->hDevMode) { - result = E_FAIL; - goto Cleanup; - } - void* dev_mode_ptr = GlobalLock(lppd->hDevMode); - if (!dev_mode_ptr) { - result = E_FAIL; - goto Cleanup; - } + ScopedGlobalAlloc dev_mode_mem(GlobalAlloc(GMEM_MOVEABLE, dev_mode_size)); + if (!dev_mode_mem.Get()) + return E_FAIL; + void* dev_mode_ptr = GlobalLock(dev_mode_mem.Get()); + if (!dev_mode_ptr) + return E_FAIL; memcpy(dev_mode_ptr, dev_mode, dev_mode_size); - GlobalUnlock(lppd->hDevMode); + GlobalUnlock(dev_mode_mem.Get()); dev_mode_ptr = NULL; size_t driver_size = @@ -102,16 +106,12 @@ class MockPrintingContextWin : public PrintingContextSytemDialogWin { size_t port_size = 2 + sizeof(wchar_t) * lstrlen(info_2.get()->pPortName); size_t dev_names_size = sizeof(DEVNAMES) + driver_size + printer_size + port_size; - lppd->hDevNames = GlobalAlloc(GHND, dev_names_size); - if (!lppd->hDevNames) { - result = E_FAIL; - goto Cleanup; - } - void* dev_names_ptr = GlobalLock(lppd->hDevNames); - if (!dev_names_ptr) { - result = E_FAIL; - goto Cleanup; - } + ScopedGlobalAlloc dev_names_mem(GlobalAlloc(GHND, dev_names_size)); + if (!dev_names_mem.Get()) + return E_FAIL; + void* dev_names_ptr = GlobalLock(dev_names_mem.Get()); + if (!dev_names_ptr) + return E_FAIL; DEVNAMES* dev_names = reinterpret_cast(dev_names_ptr); dev_names->wDefault = 1; dev_names->wDriverOffset = sizeof(DEVNAMES) / sizeof(wchar_t); @@ -128,27 +128,13 @@ class MockPrintingContextWin : public PrintingContextSytemDialogWin { memcpy(reinterpret_cast(dev_names_ptr) + dev_names->wOutputOffset, info_2.get()->pPortName, port_size); - GlobalUnlock(lppd->hDevNames); + GlobalUnlock(dev_names_mem.Get()); dev_names_ptr = NULL; - Cleanup: - // Note: This section does proper deallocation/free of DC/global handles. We - // did not use ScopedHGlobal or ScopedHandle because they did not - // perform what we need. Goto's are used based on Windows programming - // idiom, to avoid deeply nested if's, and try-catch-finally is not - // allowed in Chromium. - if (FAILED(result)) { - if (lppd->hDC) { - DeleteDC(lppd->hDC); - } - if (lppd->hDevMode) { - GlobalFree(lppd->hDevMode); - } - if (lppd->hDevNames) { - GlobalFree(lppd->hDevNames); - } - } - return result; + lppd->hDC = hdc.Take(); + lppd->hDevMode = dev_mode_mem.Take(); + lppd->hDevNames = dev_names_mem.Take(); + return S_OK; } }; diff --git a/remoting/host/win/rdp_client_window.cc b/remoting/host/win/rdp_client_window.cc index 6ccee3d2e22b06..8516195e2d80bf 100644 --- a/remoting/host/win/rdp_client_window.cc +++ b/remoting/host/win/rdp_client_window.cc @@ -247,10 +247,8 @@ LRESULT RdpClientWindow::OnCreate(CREATESTRUCT* create_struct) { RECT rect = { 0, 0, screen_size_.width(), screen_size_.height() }; activex_window.Create(m_hWnd, rect, nullptr, WS_CHILD | WS_VISIBLE | WS_BORDER); - if (activex_window.m_hWnd == nullptr) { - result = HRESULT_FROM_WIN32(GetLastError()); - goto done; - } + if (activex_window.m_hWnd == nullptr) + return LogOnCreateError(HRESULT_FROM_WIN32(GetLastError())); // Instantiate the RDP ActiveX control. result = activex_window.CreateControlEx( @@ -261,71 +259,71 @@ LRESULT RdpClientWindow::OnCreate(CREATESTRUCT* create_struct) { __uuidof(mstsc::IMsTscAxEvents), reinterpret_cast(static_cast(this))); if (FAILED(result)) - goto done; + return LogOnCreateError(result); result = control.QueryInterface(client_.Receive()); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Use 32-bit color. result = client_->put_ColorDepth(32); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Set dimensions of the remote desktop. result = client_->put_DesktopWidth(screen_size_.width()); if (FAILED(result)) - goto done; + return LogOnCreateError(result); result = client_->put_DesktopHeight(screen_size_.height()); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Set the server name to connect to. result = client_->put_Server(server_name); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Fetch IMsRdpClientAdvancedSettings interface for the client. result = client_->get_AdvancedSettings2(client_settings_.Receive()); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Disable background input mode. result = client_settings_->put_allowBackgroundInput(0); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Do not use bitmap cache. result = client_settings_->put_BitmapPersistence(0); if (SUCCEEDED(result)) result = client_settings_->put_CachePersistenceActive(0); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Do not use compression. result = client_settings_->put_Compress(0); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Enable the Ctrl+Alt+Del screen. result = client_settings_->put_DisableCtrlAltDel(0); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Disable printer and clipboard redirection. result = client_settings_->put_DisableRdpdr(FALSE); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Do not display the connection bar. result = client_settings_->put_DisplayConnectionBar(VARIANT_FALSE); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Do not grab focus on connect. result = client_settings_->put_GrabFocusOnConnect(VARIANT_FALSE); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Enable enhanced graphics, font smoothing and desktop composition. const LONG kDesiredFlags = WTS_PERF_ENABLE_ENHANCED_GRAPHICS | @@ -333,12 +331,12 @@ LRESULT RdpClientWindow::OnCreate(CREATESTRUCT* create_struct) { WTS_PERF_ENABLE_DESKTOP_COMPOSITION; result = client_settings_->put_PerformanceFlags(kDesiredFlags); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Set the port to connect to. result = client_settings_->put_RDPPort(server_endpoint_.port()); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Disable audio in the session. // TODO(alexeypa): re-enable audio redirection when http://crbug.com/242312 is @@ -347,12 +345,12 @@ LRESULT RdpClientWindow::OnCreate(CREATESTRUCT* create_struct) { if (SUCCEEDED(result)) { result = secured_settings2->put_AudioRedirectionMode(kRdpAudioModeNone); if (FAILED(result)) - goto done; + return LogOnCreateError(result); } result = client_->get_SecuredSettings(secured_settings.Receive()); if (FAILED(result)) - goto done; + return LogOnCreateError(result); // Set the terminal ID as the working directory for the initial program. It is // observed that |WorkDir| is used only if an initial program is also @@ -363,21 +361,11 @@ LRESULT RdpClientWindow::OnCreate(CREATESTRUCT* create_struct) { // This code should be in sync with WtsTerminalMonitor::LookupTerminalId(). result = secured_settings->put_WorkDir(terminal_id); if (FAILED(result)) - goto done; + return LogOnCreateError(result); result = client_->Connect(); if (FAILED(result)) - goto done; - -done: - if (FAILED(result)) { - LOG(ERROR) << "RDP: failed to initiate a connection to " - << server_endpoint_.ToString() << ": error=" - << std::hex << result << std::dec; - client_.Release(); - client_settings_.Release(); - return -1; - } + return LogOnCreateError(result); return 0; } @@ -462,6 +450,15 @@ HRESULT RdpClientWindow::OnConfirmClose(VARIANT_BOOL* allow_close) { return S_OK; } +int RdpClientWindow::LogOnCreateError(HRESULT error) { + LOG(ERROR) << "RDP: failed to initiate a connection to " + << server_endpoint_.ToString() << ": error=" + << std::hex << error << std::dec; + client_.Release(); + client_settings_.Release(); + return -1; +} + void RdpClientWindow::NotifyConnected() { if (event_handler_) event_handler_->OnConnected(); diff --git a/remoting/host/win/rdp_client_window.h b/remoting/host/win/rdp_client_window.h index 5298228710c538..4b6ccdae45c054 100644 --- a/remoting/host/win/rdp_client_window.h +++ b/remoting/host/win/rdp_client_window.h @@ -126,6 +126,8 @@ class RdpClientWindow STDMETHOD(OnFatalError)(long error_code); STDMETHOD(OnConfirmClose)(VARIANT_BOOL* allow_close); + int LogOnCreateError(HRESULT error); + // Wrappers for the event handler's methods that make sure that // OnDisconnected() is the last notification delivered and is delevered // only once.