From 61cd852a6fd0ecc6454a2df1030fa55e580a3d1e Mon Sep 17 00:00:00 2001 From: "dbeam@chromium.org" Date: Fri, 24 May 2013 04:29:15 +0000 Subject: Revert 201955 "Allow multiple base::MessagePumpForUI instances t..." Broke browser_tests on XP Tests (dbg)(4): http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%28dbg%29%284%29/builds/33253 [844:3260:0523/194017:1212046:FATAL:message_window.cc(28)] Check failed: CalledOnValidThread(). Backtrace: base::Histogram::GetCountAndBucketData [0x08408FD1+2262176] base::Histogram::GetCountAndBucketData [0x0827FBBE+651405] base::Histogram::GetCountAndBucketData [0x082E1E60+1053487] base::Histogram::GetCountAndBucketData [0x0825CC46+508181] base::Histogram::GetCountAndBucketData [0x0825BB66+503861] base::Histogram::GetCountAndBucketData [0x0825A534+498179] base::Histogram::GetCountAndBucketData [0x08256C16+483557] base::Histogram::GetCountAndBucketData [0x08256C82+483665] base::Histogram::GetCountAndBucketData [0x08256324+481267] base::Histogram::GetCountAndBucketData [0x081F59AF+85630] base::Histogram::GetCountAndBucketData [0x081F84AC+96635] base::Histogram::GetCountAndBucketData [0x081F5917+85478] base::Histogram::GetCountAndBucketData [0x08249FD3+431266] base::Histogram::GetCountAndBucketData [0x08249255+427812] base::Histogram::GetCountAndBucketData [0x082462CD+415644] base::Histogram::GetCountAndBucketData [0x0831D883+1297746] base::Histogram::GetCountAndBucketData [0x0831D602+1297105] ViewHostMsg_TextInputStateChanged::Read [0x0D3DD6F9+6214944] ViewHostMsg_TextInputStateChanged::Read [0x0D3DD584+6214571] ViewHostMsg_TextInputStateChanged::Read [0x0D33048E+5505717] ViewHostMsg_TextInputStateChanged::Read [0x0D3303FA+5505569] ViewHostMsg_TextInputStateChanged::Read [0x0D32DDEA+5495825] ViewHostMsg_TextInputStateChanged::Read [0x0D32BAC7+5486830] ViewHostMsg_TextInputStateChanged::Read [0x0D3DC9B6+6211549] ViewHostMsg_TextInputStateChanged::Read [0x0D367071+5729944] RelaunchChromeBrowserWithNewCommandLineIfNeeded [0x03E93429+30896697] RelaunchChromeBrowserWithNewCommandLineIfNeeded [0x025BD0DF+4853487] RelaunchChromeBrowserWithNewCommandLineIfNeeded [0x02701C3F+6183503] RelaunchChromeBrowserWithNewCommandLineIfNeeded [0x026EECCB+6105819] RelaunchChromeBrowserWithNewCommandLineIfNeeded [0x026EF6DD+6108397] RelaunchChromeBrowserWithNewCommandLineIfNeeded [0x026EFEAF+6110399] RelaunchChromeBrowserWithNewCommandLineIfNeeded [0x026F673D+6137165] RelaunchChromeBrowserWithNewCommandLineIfNeeded [0x02702637+6186055] RelaunchChromeBrowserWithNewCommandLineIfNeeded [0x026F4FA0+6131120] RelaunchChromeBrowserWithNewCommandLineIfNeeded [0x0262E070+5316224] (No symbol) [0x00632BF5] RelaunchChromeBrowserWithNewCommandLineIfNeeded [0x03E2BD73+30473091] (No symbol) [0x00632975] std::_Init_locks::operator= [0x03F5574F+5791] std::_Init_locks::operator= [0x03F5557F+5327] RegisterWaitForInputIdle [0x7C817077+73] > Allow multiple base::MessagePumpForUI instances to be created simultanenously on Windows. > > The current implementation of base::MessagePumpForUI on Windows registers a window class with a predefined name in order to create a message-only window. The window class is unregistered when base::MessagePumpForUI is deleted. This causes issues when two or more instances of base::MessagePumpForUI are created/destroyed simultanenously on different threads. For instance once thread can unregister the window class right before the other thread is trying to create a window using this class. > > The CL addresses this problem by switching MessageWindow to implement a message-only window. It also moves MessageWindow from remoting/host/win to base/win along with the corresponding unit test. > > MessageWindow registers a uniquely named window class per MessageWindow instance making sure that different MessageWindow objects do not share any resources. In the future this can be optimized further by registering a common window class shared by all MessageWindow objects in a thread-safe manner (by using LazyInstance for example). > > BUG=241939 > > Review URL: https://chromiumcodereview.appspot.com/15261005 TBR=alexeypa@chromium.org Review URL: https://codereview.chromium.org/15973003 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@201974 0039d316-1c4b-4281-b951-d872f2087c98 --- remoting/host/clipboard_win.cc | 10 +-- remoting/host/local_input_monitor_win.cc | 10 +-- remoting/host/win/host_service.cc | 2 +- remoting/host/win/host_service.h | 6 +- remoting/host/win/message_window.cc | 118 +++++++++++++++++++++++++++ remoting/host/win/message_window.h | 67 +++++++++++++++ remoting/host/win/message_window_unittest.cc | 61 ++++++++++++++ remoting/remoting.gyp | 3 + 8 files changed, 263 insertions(+), 14 deletions(-) create mode 100644 remoting/host/win/message_window.cc create mode 100644 remoting/host/win/message_window.h create mode 100644 remoting/host/win/message_window_unittest.cc (limited to 'remoting') diff --git a/remoting/host/clipboard_win.cc b/remoting/host/clipboard_win.cc index fb32dd0..419316a4 100644 --- a/remoting/host/clipboard_win.cc +++ b/remoting/host/clipboard_win.cc @@ -13,12 +13,12 @@ #include "base/string16.h" #include "base/threading/platform_thread.h" #include "base/utf_string_conversions.h" -#include "base/win/message_window.h" #include "base/win/scoped_hglobal.h" #include "base/win/windows_version.h" #include "base/win/wrapped_window_proc.h" #include "remoting/base/constants.h" #include "remoting/base/util.h" +#include "remoting/host/win/message_window.h" #include "remoting/proto/event.pb.h" #include "remoting/protocol/clipboard_stub.h" @@ -101,7 +101,7 @@ typedef BOOL (WINAPI RemoveClipboardFormatListenerFn)(HWND); namespace remoting { class ClipboardWin : public Clipboard, - public base::win::MessageWindow::Delegate { + public win::MessageWindow::Delegate { public: ClipboardWin(); @@ -114,7 +114,7 @@ class ClipboardWin : public Clipboard, private: void OnClipboardUpdate(); - // base::win::MessageWindow::Delegate interface. + // win::MessageWindow::Delegate interface. virtual bool HandleMessage(HWND hwnd, UINT message, WPARAM wparam, @@ -126,7 +126,7 @@ class ClipboardWin : public Clipboard, RemoveClipboardFormatListenerFn* remove_clipboard_format_listener_; // Used to subscribe to WM_CLIPBOARDUPDATE messages. - scoped_ptr window_; + scoped_ptr window_; DISALLOW_COPY_AND_ASSIGN(ClipboardWin); }; @@ -162,7 +162,7 @@ void ClipboardWin::Start( LOG(WARNING) << "AddClipboardFormatListener() is not available."; } - window_.reset(new base::win::MessageWindow()); + window_.reset(new win::MessageWindow()); if (!window_->Create(this)) { LOG(ERROR) << "Couldn't create clipboard window."; window_.reset(); diff --git a/remoting/host/local_input_monitor_win.cc b/remoting/host/local_input_monitor_win.cc index 5d4ce8c..98cd8a4 100644 --- a/remoting/host/local_input_monitor_win.cc +++ b/remoting/host/local_input_monitor_win.cc @@ -11,8 +11,8 @@ #include "base/single_thread_task_runner.h" #include "base/stringprintf.h" #include "base/threading/non_thread_safe.h" -#include "base/win/message_window.h" #include "remoting/host/client_session_control.h" +#include "remoting/host/win/message_window.h" #include "third_party/skia/include/core/SkPoint.h" namespace remoting { @@ -37,7 +37,7 @@ class LocalInputMonitorWin : public base::NonThreadSafe, private: // The actual implementation resides in LocalInputMonitorWin::Core class. class Core : public base::RefCountedThreadSafe, - public base::win::MessageWindow::Delegate { + public win::MessageWindow::Delegate { public: Core(scoped_refptr caller_task_runner, scoped_refptr ui_task_runner, @@ -56,7 +56,7 @@ class LocalInputMonitorWin : public base::NonThreadSafe, // Handles WM_INPUT messages. LRESULT OnInput(HRAWINPUT input_handle); - // base::win::MessageWindow::Delegate interface. + // win::MessageWindow::Delegate interface. virtual bool HandleMessage(HWND hwnd, UINT message, WPARAM wparam, @@ -70,7 +70,7 @@ class LocalInputMonitorWin : public base::NonThreadSafe, scoped_refptr ui_task_runner_; // Used to receive raw input. - scoped_ptr window_; + scoped_ptr window_; // Points to the object receiving mouse event notifications. base::WeakPtr client_session_control_; @@ -127,7 +127,7 @@ LocalInputMonitorWin::Core::~Core() { void LocalInputMonitorWin::Core::StartOnUiThread() { DCHECK(ui_task_runner_->BelongsToCurrentThread()); - window_.reset(new base::win::MessageWindow()); + window_.reset(new win::MessageWindow()); if (!window_->Create(this)) { LOG_GETLASTERROR(ERROR) << "Failed to create the raw input window"; window_.reset(); diff --git a/remoting/host/win/host_service.cc b/remoting/host/win/host_service.cc index 679603a..42a8ed0 100644 --- a/remoting/host/win/host_service.cc +++ b/remoting/host/win/host_service.cc @@ -385,7 +385,7 @@ int HostService::RunInConsole() { } // Create a window for receiving session change notifications. - base::win::MessageWindow window; + win::MessageWindow window; if (!window.Create(this)) { LOG_GETLASTERROR(ERROR) << "Failed to create the session notification window"; diff --git a/remoting/host/win/host_service.h b/remoting/host/win/host_service.h index 20b290f..aa1d9ab 100644 --- a/remoting/host/win/host_service.h +++ b/remoting/host/win/host_service.h @@ -12,8 +12,8 @@ #include "base/memory/ref_counted.h" #include "base/memory/singleton.h" #include "base/synchronization/waitable_event.h" -#include "base/win/message_window.h" #include "net/base/ip_endpoint.h" +#include "remoting/host/win/message_window.h" #include "remoting/host/win/wts_terminal_monitor.h" class CommandLine; @@ -28,7 +28,7 @@ class AutoThreadTaskRunner; class Stoppable; class WtsTerminalObserver; -class HostService : public base::win::MessageWindow::Delegate, +class HostService : public win::MessageWindow::Delegate, public WtsTerminalMonitor { public: static HostService* GetInstance(); @@ -70,7 +70,7 @@ class HostService : public base::win::MessageWindow::Delegate, // console application). int RunInConsole(); - // base::win::MessageWindow::Delegate interface. + // win::MessageWindow::Delegate interface. virtual bool HandleMessage(HWND hwnd, UINT message, WPARAM wparam, diff --git a/remoting/host/win/message_window.cc b/remoting/host/win/message_window.cc new file mode 100644 index 0000000..b36b3f6 --- /dev/null +++ b/remoting/host/win/message_window.cc @@ -0,0 +1,118 @@ +// Copyright 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/message_window.h" + +#include "base/logging.h" +#include "base/process_util.h" +#include "base/stringprintf.h" +#include "base/utf_string_conversions.h" +#include "base/win/wrapped_window_proc.h" + +const char kClassNameFormat[] = "Chromoting_MessageWindow_%p"; + +namespace remoting { +namespace win { + +MessageWindow::MessageWindow() + : atom_(0), + instance_(NULL), + window_(NULL) { + class_name_ = base::StringPrintf(kClassNameFormat, this); + instance_ = base::GetModuleFromAddress(static_cast( + &base::win::WrappedWindowProc)); +} + +MessageWindow::MessageWindow(const std::string& class_name, HINSTANCE instance) + : atom_(0), + class_name_(class_name), + instance_(instance), + window_(NULL) { +} + +MessageWindow::~MessageWindow() { + DCHECK(CalledOnValidThread()); + + if (window_ != NULL) { + DestroyWindow(window_); + window_ = NULL; + } + + if (atom_ != 0) { + UnregisterClass(MAKEINTATOM(atom_), instance_); + atom_ = 0; + } +} + +bool MessageWindow::Create(Delegate* delegate) { + DCHECK(CalledOnValidThread()); + DCHECK(!atom_); + DCHECK(!window_); + + // Register a separate window class for each instance of |MessageWindow|. + string16 class_name = UTF8ToUTF16(class_name_); + WNDCLASSEX window_class; + window_class.cbSize = sizeof(window_class); + window_class.style = 0; + window_class.lpfnWndProc = &base::win::WrappedWindowProc; + window_class.cbClsExtra = 0; + window_class.cbWndExtra = 0; + window_class.hInstance = instance_; + window_class.hIcon = NULL; + window_class.hCursor = NULL; + window_class.hbrBackground = NULL; + window_class.lpszMenuName = NULL; + window_class.lpszClassName = class_name.c_str(); + window_class.hIconSm = NULL; + atom_ = RegisterClassEx(&window_class); + if (atom_ == 0) { + LOG_GETLASTERROR(ERROR) + << "Failed to register the window class '" << class_name_ << "'"; + return false; + } + + window_ = CreateWindow(MAKEINTATOM(atom_), 0, 0, 0, 0, 0, 0, HWND_MESSAGE, 0, + instance_, delegate); + if (!window_) { + LOG_GETLASTERROR(ERROR) << "Failed to create a message-only window"; + return false; + } + + return true; +} + +// static +LRESULT CALLBACK MessageWindow::WindowProc(HWND hwnd, + UINT message, + WPARAM wparam, + LPARAM lparam) { + Delegate* delegate = NULL; + + // Set up the delegate before handling WM_CREATE. + if (message == WM_CREATE) { + CREATESTRUCT* cs = reinterpret_cast(lparam); + delegate = reinterpret_cast(cs->lpCreateParams); + + // Store pointer to the delegate to the window's user data. + SetLastError(ERROR_SUCCESS); + LONG_PTR result = SetWindowLongPtr( + hwnd, GWLP_USERDATA, reinterpret_cast(delegate)); + CHECK(result != 0 || GetLastError() == ERROR_SUCCESS); + } else { + delegate = reinterpret_cast(GetWindowLongPtr(hwnd, + GWLP_USERDATA)); + } + + // Handle the message. + if (delegate) { + LRESULT message_result; + if (delegate->HandleMessage(hwnd, message, wparam, lparam, &message_result)) + return message_result; + } + + return DefWindowProc(hwnd, message, wparam, lparam); +} + +} // namespace win +} // namespace remoting diff --git a/remoting/host/win/message_window.h b/remoting/host/win/message_window.h new file mode 100644 index 0000000..ecd2a36 --- /dev/null +++ b/remoting/host/win/message_window.h @@ -0,0 +1,67 @@ +// Copyright 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. + +#ifndef REMOTING_HOST_WIN_MESSAGE_WINDOW_H_ +#define REMOTING_HOST_WIN_MESSAGE_WINDOW_H_ + +#include + +#include + +#include "base/basictypes.h" +#include "base/compiler_specific.h" +#include "base/threading/non_thread_safe.h" + +namespace remoting { +namespace win { + +// Implements a message-only window. +class MessageWindow : base::NonThreadSafe { + public: + // Handles incoming window messages. + class Delegate { + public: + virtual ~Delegate() {} + + virtual bool HandleMessage(HWND hwnd, + UINT message, + WPARAM wparam, + LPARAM lparam, + LRESULT* result) = 0; + }; + + MessageWindow(); + MessageWindow(const std::string& class_name, HINSTANCE instance); + ~MessageWindow(); + + // Registers the window class and creates the window. The incoming messages + // will be handled by |delegate|. |delegate| must outlive |this|. + bool Create(Delegate* delegate); + + HWND hwnd() const { return window_; } + + private: + // Invoked by the OS to process incoming window messages. + static LRESULT CALLBACK WindowProc(HWND hwnd, UINT message, WPARAM wparam, + LPARAM lparam); + + // Atom representing the registered window class. + ATOM atom_; + + // MessageWindow class name. + std::string class_name_; + + // Instance of the module containing the window procedure. + HINSTANCE instance_; + + // Handle of the input window. + HWND window_; + + DISALLOW_COPY_AND_ASSIGN(MessageWindow); +}; + +} // namespace win +} // namespace remoting + +#endif // REMOTING_HOST_WIN_MESSAGE_WINDOW_H_ diff --git a/remoting/host/win/message_window_unittest.cc b/remoting/host/win/message_window_unittest.cc new file mode 100644 index 0000000..d12af7b --- /dev/null +++ b/remoting/host/win/message_window_unittest.cc @@ -0,0 +1,61 @@ +// 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 "remoting/host/win/message_window.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace remoting { + +namespace { + +class MessageWindowDelegate : public win::MessageWindow::Delegate { + public: + MessageWindowDelegate(); + virtual ~MessageWindowDelegate(); + + // MessageWindow::Delegate interface. + virtual bool HandleMessage(HWND hwnd, + UINT message, + WPARAM wparam, + LPARAM lparam, + LRESULT* result) OVERRIDE; +}; + +MessageWindowDelegate::MessageWindowDelegate() { +} + +MessageWindowDelegate::~MessageWindowDelegate() { +} + +bool MessageWindowDelegate::HandleMessage( + HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam, LRESULT* result) { + // Return |wparam| as the result of WM_USER message. + if (message == WM_USER) { + *result = wparam; + return true; + } + + return false; +} + +} // namespace + +// Checks that a window can be created. +TEST(MessageWindowTest, Create) { + MessageWindowDelegate delegate; + win::MessageWindow window; + EXPECT_TRUE(window.Create(&delegate)); +} + +// Verifies that the created window can receive messages. +TEST(MessageWindowTest, SendMessage) { + MessageWindowDelegate delegate; + win::MessageWindow window; + EXPECT_TRUE(window.Create(&delegate)); + + EXPECT_EQ(SendMessage(window.hwnd(), WM_USER, 100, 0), 100); +} + +} // namespace remoting diff --git a/remoting/remoting.gyp b/remoting/remoting.gyp index 5c6eb3c..b883868 100644 --- a/remoting/remoting.gyp +++ b/remoting/remoting.gyp @@ -464,6 +464,8 @@ 'host/vlog_net_log.h', 'host/win/launch_process_with_token.cc', 'host/win/launch_process_with_token.h', + 'host/win/message_window.cc', + 'host/win/message_window.h', 'host/win/omaha.cc', 'host/win/omaha.h', 'host/win/rdp_client.cc', @@ -2640,6 +2642,7 @@ 'host/setup/pin_validator_unittest.cc', 'host/token_validator_factory_impl_unittest.cc', 'host/video_scheduler_unittest.cc', + 'host/win/message_window_unittest.cc', 'host/win/rdp_client_unittest.cc', 'host/win/worker_process_launcher.cc', 'host/win/worker_process_launcher.h', -- cgit v1.1