diff options
author | rockot <rockot@chromium.org> | 2014-09-05 01:02:44 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-09-05 08:05:55 +0000 |
commit | 1a51b92b7038b82a90c1a1944104eaf8c51a9e1c (patch) | |
tree | 9dde79f2e12d64897300d2c389725ab4d349f01f | |
parent | 243eec86f415a6624d8d41b0ea0393eda0b3a808 (diff) | |
download | chromium_src-1a51b92b7038b82a90c1a1944104eaf8c51a9e1c.zip chromium_src-1a51b92b7038b82a90c1a1944104eaf8c51a9e1c.tar.gz chromium_src-1a51b92b7038b82a90c1a1944104eaf8c51a9e1c.tar.bz2 |
Fix HidService lifetime issues
This reverts HidService to being a singleton instance for now.
BUG=401234
TBR=rpaquay
Review URL: https://codereview.chromium.org/523743005
Cr-Commit-Position: refs/heads/master@{#293464}
25 files changed, 88 insertions, 110 deletions
diff --git a/chrome/browser/DEPS b/chrome/browser/DEPS index 69b582a..3bd7208 100644 --- a/chrome/browser/DEPS +++ b/chrome/browser/DEPS @@ -79,6 +79,7 @@ include_rules = [ "+courgette", "+device/bluetooth", "+device/core", + "+device/hid", "+device/usb", "+device/media_transfer_protocol", "+extensions/browser", diff --git a/chrome/browser/chrome_device_client.cc b/chrome/browser/chrome_device_client.cc index f36d61f..1eb2572 100644 --- a/chrome/browser/chrome_device_client.cc +++ b/chrome/browser/chrome_device_client.cc @@ -6,6 +6,7 @@ #include "base/logging.h" #include "content/public/browser/browser_thread.h" +#include "device/hid/hid_service.h" #include "device/usb/usb_service.h" ChromeDeviceClient::ChromeDeviceClient() {} @@ -17,3 +18,9 @@ device::UsbService* ChromeDeviceClient::GetUsbService() { content::BrowserThread::GetMessageLoopProxyForThread( content::BrowserThread::UI)); } + +device::HidService* ChromeDeviceClient::GetHidService() { + return device::HidService::GetInstance( + content::BrowserThread::GetMessageLoopProxyForThread( + content::BrowserThread::UI)); +} diff --git a/chrome/browser/chrome_device_client.h b/chrome/browser/chrome_device_client.h index 9f08f96..96d38b9 100644 --- a/chrome/browser/chrome_device_client.h +++ b/chrome/browser/chrome_device_client.h @@ -19,6 +19,7 @@ class ChromeDeviceClient : device::DeviceClient { // device::DeviceClient implementation virtual device::UsbService* GetUsbService() OVERRIDE; + virtual device::HidService* GetHidService() OVERRIDE; private: DISALLOW_COPY_AND_ASSIGN(ChromeDeviceClient); diff --git a/chrome/browser/extensions/api/chrome_extensions_api_client.cc b/chrome/browser/extensions/api/chrome_extensions_api_client.cc index 67db675..08bf08d 100644 --- a/chrome/browser/extensions/api/chrome_extensions_api_client.cc +++ b/chrome/browser/extensions/api/chrome_extensions_api_client.cc @@ -12,7 +12,6 @@ #include "chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/browser_thread.h" -#include "device/hid/hid_service.h" #include "extensions/browser/guest_view/app_view/app_view_guest.h" #include "extensions/browser/guest_view/web_view/web_view_guest.h" #include "extensions/browser/guest_view/web_view/web_view_permission_helper.h" @@ -59,15 +58,6 @@ WebViewPermissionHelperDelegate* ChromeExtensionsAPIClient:: return new ChromeWebViewPermissionHelperDelegate(web_view_permission_helper); } -device::HidService* ChromeExtensionsAPIClient::GetHidService() { - if (!hid_service_) { - hid_service_.reset(device::HidService::Create( - content::BrowserThread::GetMessageLoopProxyForThread( - content::BrowserThread::UI))); - } - return hid_service_.get(); -} - void ChromeExtensionsAPIClient::RegisterGuestViewTypes() { ExtensionOptionsGuest::Register(); } diff --git a/chrome/browser/extensions/api/chrome_extensions_api_client.h b/chrome/browser/extensions/api/chrome_extensions_api_client.h index 881a627..e7bc28c 100644 --- a/chrome/browser/extensions/api/chrome_extensions_api_client.h +++ b/chrome/browser/extensions/api/chrome_extensions_api_client.h @@ -6,7 +6,6 @@ #define CHROME_BROWSER_EXTENSIONS_API_CHROME_EXTENSIONS_API_CLIENT_H_ #include "base/compiler_specific.h" -#include "base/memory/scoped_ptr.h" #include "extensions/browser/api/extensions_api_client.h" namespace extensions { @@ -30,12 +29,9 @@ class ChromeExtensionsAPIClient : public ExtensionsAPIClient { virtual WebViewPermissionHelperDelegate* CreateWebViewPermissionHelperDelegate( WebViewPermissionHelper* web_view_permission_helper) const OVERRIDE; - virtual device::HidService* GetHidService() OVERRIDE; virtual void RegisterGuestViewTypes() OVERRIDE; private: - scoped_ptr<device::HidService> hid_service_; - DISALLOW_COPY_AND_ASSIGN(ChromeExtensionsAPIClient); }; diff --git a/device/core/device_client.cc b/device/core/device_client.cc index f92ec17..5202122 100644 --- a/device/core/device_client.cc +++ b/device/core/device_client.cc @@ -35,4 +35,10 @@ UsbService* DeviceClient::GetUsbService() { return NULL; } +HidService* DeviceClient::GetHidService() { + // This should never be called by clients which do not support the HID API. + NOTREACHED(); + return NULL; +} + } // namespace device diff --git a/device/core/device_client.h b/device/core/device_client.h index 3ca3130..b4a9934 100644 --- a/device/core/device_client.h +++ b/device/core/device_client.h @@ -9,6 +9,7 @@ namespace device { +class HidService; class UsbService; // Interface used by consumers of //device APIs to get pointers to the service @@ -28,6 +29,9 @@ class DeviceClient { // Returns the UsbService instance for this embedder. virtual UsbService* GetUsbService(); + // Returns the HidService instance for this embedder. + virtual HidService* GetHidService(); + private: DISALLOW_COPY_AND_ASSIGN(DeviceClient); }; diff --git a/device/hid/hid_connection_unittest.cc b/device/hid/hid_connection_unittest.cc index aade928..064720f 100644 --- a/device/hid/hid_connection_unittest.cc +++ b/device/hid/hid_connection_unittest.cc @@ -73,7 +73,8 @@ class HidConnectionTest : public testing::Test { if (!UsbTestGadget::IsTestEnabled()) return; message_loop_.reset(new base::MessageLoopForIO()); - service_.reset(HidService::Create(message_loop_->message_loop_proxy())); + service_.reset(HidService::GetInstance( + message_loop_->message_loop_proxy())); ASSERT_TRUE(service_); test_gadget_ = UsbTestGadget::Claim(); diff --git a/device/hid/hid_service.cc b/device/hid/hid_service.cc index 0e2f0d4..c9ab2fc 100644 --- a/device/hid/hid_service.cc +++ b/device/hid/hid_service.cc @@ -7,6 +7,7 @@ #include "base/lazy_instance.h" #include "base/logging.h" #include "base/memory/scoped_vector.h" +#include "base/message_loop/message_loop.h" #include "base/stl_util.h" #include "base/threading/thread_restrictions.h" @@ -20,17 +21,46 @@ namespace device { -HidService* HidService::Create( - scoped_refptr<base::MessageLoopProxy> ui_message_loop) { +namespace { + +HidService* g_service = NULL; + +class HidServiceDestroyer : public base::MessageLoop::DestructionObserver { + public: + explicit HidServiceDestroyer(HidService* hid_service) + : hid_service_(hid_service) {} + virtual ~HidServiceDestroyer() {} + + private: + // base::MessageLoop::DestructionObserver implementation. + virtual void WillDestroyCurrentMessageLoop() OVERRIDE { + base::MessageLoop::current()->RemoveDestructionObserver(this); + delete hid_service_; + delete this; + g_service = NULL; + } + + HidService* hid_service_; +}; + +} // namespace + +HidService* HidService::GetInstance( + scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) { + if (g_service == NULL) { #if defined(OS_LINUX) && defined(USE_UDEV) - return new HidServiceLinux(ui_message_loop); + g_service = new HidServiceLinux(ui_task_runner); #elif defined(OS_MACOSX) - return new HidServiceMac(); + g_service = new HidServiceMac(); #elif defined(OS_WIN) - return new HidServiceWin(); -#else - return NULL; + g_service = new HidServiceWin(); #endif + if (g_service != NULL) { + HidServiceDestroyer* destroyer = new HidServiceDestroyer(g_service); + base::MessageLoop::current()->AddDestructionObserver(destroyer); + } + } + return g_service; } HidService::~HidService() { diff --git a/device/hid/hid_service.h b/device/hid/hid_service.h index c8e4e41..cc7813b 100644 --- a/device/hid/hid_service.h +++ b/device/hid/hid_service.h @@ -10,7 +10,7 @@ #include <vector> #include "base/memory/ref_counted.h" -#include "base/message_loop/message_loop_proxy.h" +#include "base/single_thread_task_runner.h" #include "base/threading/thread_checker.h" #include "device/hid/hid_device_info.h" @@ -20,8 +20,8 @@ class HidConnection; class HidService { public: - static HidService* Create( - scoped_refptr<base::MessageLoopProxy> ui_message_loop); + static HidService* GetInstance( + scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner); virtual ~HidService(); diff --git a/device/hid/hid_service_linux.cc b/device/hid/hid_service_linux.cc index 8146c14..3fb27aa 100644 --- a/device/hid/hid_service_linux.cc +++ b/device/hid/hid_service_linux.cc @@ -41,8 +41,8 @@ const char kHIDUnique[] = "HID_UNIQ"; } // namespace HidServiceLinux::HidServiceLinux( - scoped_refptr<base::MessageLoopProxy> ui_message_loop) - : ui_message_loop_(ui_message_loop), + scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) + : ui_task_runner_(ui_task_runner), weak_factory_(this) { DeviceMonitorLinux* monitor = DeviceMonitorLinux::GetInstance(); monitor->AddObserver(this); @@ -132,7 +132,7 @@ void HidServiceLinux::OnDeviceAdded(udev_device* device) { if (!client) { return; } - ui_message_loop_->PostTask( + ui_task_runner_->PostTask( FROM_HERE, base::Bind(&chromeos::PermissionBrokerClient::RequestPathAccess, base::Unretained(client), diff --git a/device/hid/hid_service_linux.h b/device/hid/hid_service_linux.h index 62bb4c7..7b62fc0 100644 --- a/device/hid/hid_service_linux.h +++ b/device/hid/hid_service_linux.h @@ -21,7 +21,7 @@ class HidConnection; class HidServiceLinux : public HidService, public DeviceMonitorLinux::Observer { public: - HidServiceLinux(scoped_refptr<base::MessageLoopProxy> ui_message_loop); + HidServiceLinux(scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner); virtual scoped_refptr<HidConnection> Connect(const HidDeviceId& device_id) OVERRIDE; @@ -38,7 +38,7 @@ class HidServiceLinux : public HidService, scoped_ptr<HidDeviceInfo> device_info, bool success); - scoped_refptr<base::MessageLoopProxy> ui_message_loop_; + scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_; base::WeakPtrFactory<HidServiceLinux> weak_factory_; diff --git a/device/hid/hid_service_unittest.cc b/device/hid/hid_service_unittest.cc index a807579..2291300 100644 --- a/device/hid/hid_service_unittest.cc +++ b/device/hid/hid_service_unittest.cc @@ -13,8 +13,8 @@ namespace device { TEST(HidServiceTest, Create) { base::MessageLoopForIO message_loop; - scoped_ptr<HidService> service( - HidService::Create(message_loop.message_loop_proxy())); + HidService* service = HidService::GetInstance( + message_loop.message_loop_proxy()); ASSERT_TRUE(service); std::vector<HidDeviceInfo> devices; diff --git a/extensions/browser/api/extensions_api_client.cc b/extensions/browser/api/extensions_api_client.cc index e797212..471e2b1 100644 --- a/extensions/browser/api/extensions_api_client.cc +++ b/extensions/browser/api/extensions_api_client.cc @@ -30,12 +30,6 @@ AppViewGuestDelegate* ExtensionsAPIClient::CreateAppViewGuestDelegate() const { return NULL; } -device::HidService* ExtensionsAPIClient::GetHidService() { - // This should never be called by clients which don't support the HID API. - NOTIMPLEMENTED(); - return NULL; -} - WebViewGuestDelegate* ExtensionsAPIClient::CreateWebViewGuestDelegate( WebViewGuest* web_view_guest) const { return NULL; diff --git a/extensions/browser/api/extensions_api_client.h b/extensions/browser/api/extensions_api_client.h index 5f1a8e3..9332dc6 100644 --- a/extensions/browser/api/extensions_api_client.h +++ b/extensions/browser/api/extensions_api_client.h @@ -19,10 +19,6 @@ namespace content { class BrowserContext; } -namespace device { -class HidService; -} - namespace extensions { class AppViewGuestDelegate; @@ -61,9 +57,6 @@ class ExtensionsAPIClient { // Creates the AppViewGuestDelegate. virtual AppViewGuestDelegate* CreateAppViewGuestDelegate() const; - // Returns the HidService instance for this embedder. - virtual device::HidService* GetHidService(); - // Returns a delegate for some of WebViewGuest's behavior. The caller owns the // returned WebViewGuestDelegate. virtual WebViewGuestDelegate* CreateWebViewGuestDelegate ( diff --git a/extensions/browser/api/hid/DEPS b/extensions/browser/api/hid/DEPS index 76a0f69..7dc8543 100644 --- a/extensions/browser/api/hid/DEPS +++ b/extensions/browser/api/hid/DEPS @@ -1,3 +1,4 @@ include_rules = [ + "+device/core", "+device/hid", ] diff --git a/extensions/browser/api/hid/hid_api.cc b/extensions/browser/api/hid/hid_api.cc index 05f4720..99cf1bd 100644 --- a/extensions/browser/api/hid/hid_api.cc +++ b/extensions/browser/api/hid/hid_api.cc @@ -7,12 +7,12 @@ #include <string> #include <vector> +#include "device/core/device_client.h" #include "device/hid/hid_connection.h" #include "device/hid/hid_device_filter.h" #include "device/hid/hid_device_info.h" #include "device/hid/hid_service.h" #include "extensions/browser/api/api_resource_manager.h" -#include "extensions/browser/api/extensions_api_client.h" #include "extensions/common/api/hid.h" #include "net/base/io_buffer.h" @@ -143,7 +143,7 @@ void HidConnectFunction::AsyncWorkStart() { return; } - HidService* hid_service = ExtensionsAPIClient::Get()->GetHidService(); + HidService* hid_service = device::DeviceClient::Get()->GetHidService(); DCHECK(hid_service); scoped_refptr<HidConnection> connection = hid_service->Connect(device_info.device_id); diff --git a/extensions/browser/api/hid/hid_device_manager.cc b/extensions/browser/api/hid/hid_device_manager.cc index b57e2d9..1c25025 100644 --- a/extensions/browser/api/hid/hid_device_manager.cc +++ b/extensions/browser/api/hid/hid_device_manager.cc @@ -8,6 +8,7 @@ #include <vector> #include "base/lazy_instance.h" +#include "device/core/device_client.h" #include "device/hid/hid_device_filter.h" #include "device/hid/hid_service.h" #include "extensions/browser/api/extensions_api_client.h" @@ -21,7 +22,8 @@ using device::HidUsageAndPage; namespace extensions { HidDeviceManager::HidDeviceManager(content::BrowserContext* context) - : next_resource_id_(0) {} + : next_resource_id_(0) { +} HidDeviceManager::~HidDeviceManager() {} @@ -38,7 +40,7 @@ scoped_ptr<base::ListValue> HidDeviceManager::GetApiDevices( const std::vector<HidDeviceFilter>& filters) { UpdateDevices(); - HidService* hid_service = ExtensionsAPIClient::Get()->GetHidService(); + HidService* hid_service = device::DeviceClient::Get()->GetHidService(); DCHECK(hid_service); base::ListValue* api_devices = new base::ListValue(); for (ResourceIdToDeviceIdMap::const_iterator device_iter = @@ -106,7 +108,7 @@ scoped_ptr<base::ListValue> HidDeviceManager::GetApiDevices( bool HidDeviceManager::GetDeviceInfo(int resource_id, device::HidDeviceInfo* device_info) { UpdateDevices(); - HidService* hid_service = ExtensionsAPIClient::Get()->GetHidService(); + HidService* hid_service = device::DeviceClient::Get()->GetHidService(); DCHECK(hid_service); ResourceIdToDeviceIdMap::const_iterator device_iter = @@ -142,7 +144,7 @@ bool HidDeviceManager::HasPermission(const Extension* extension, void HidDeviceManager::UpdateDevices() { thread_checker_.CalledOnValidThread(); - HidService* hid_service = ExtensionsAPIClient::Get()->GetHidService(); + HidService* hid_service = device::DeviceClient::Get()->GetHidService(); DCHECK(hid_service); std::vector<device::HidDeviceInfo> devices; diff --git a/extensions/browser/api/hid/hid_device_manager.h b/extensions/browser/api/hid/hid_device_manager.h index a258be4..f204269 100644 --- a/extensions/browser/api/hid/hid_device_manager.h +++ b/extensions/browser/api/hid/hid_device_manager.h @@ -10,6 +10,7 @@ #include <map> #include "base/basictypes.h" +#include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" #include "base/threading/thread_checker.h" #include "device/hid/hid_device_info.h" diff --git a/extensions/shell/app_shell.gyp b/extensions/shell/app_shell.gyp index 0eeb392..a1e00fa 100644 --- a/extensions/shell/app_shell.gyp +++ b/extensions/shell/app_shell.gyp @@ -50,8 +50,6 @@ 'app/shell_main_delegate.h', 'browser/api/shell/shell_api.cc', 'browser/api/shell/shell_api.h', - 'browser/api/shell_extensions_api_client.cc', - 'browser/api/shell_extensions_api_client.h', 'browser/default_shell_browser_main_delegate.cc', 'browser/default_shell_browser_main_delegate.h', 'browser/desktop_controller.cc', diff --git a/extensions/shell/browser/api/shell_extensions_api_client.cc b/extensions/shell/browser/api/shell_extensions_api_client.cc deleted file mode 100644 index a8e7d11..0000000 --- a/extensions/shell/browser/api/shell_extensions_api_client.cc +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright 2014 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 "extensions/shell/browser/api/shell_extensions_api_client.h" - -#include "content/public/browser/browser_thread.h" -#include "device/hid/hid_service.h" - -namespace extensions { - -ShellExtensionsAPIClient::ShellExtensionsAPIClient() { -} - -ShellExtensionsAPIClient::~ShellExtensionsAPIClient() { -} - -device::HidService* ShellExtensionsAPIClient::GetHidService() { - if (!hid_service_) { - hid_service_.reset(device::HidService::Create( - content::BrowserThread::GetMessageLoopProxyForThread( - content::BrowserThread::UI))); - } - return hid_service_.get(); -} - -} // namespace extensions diff --git a/extensions/shell/browser/api/shell_extensions_api_client.h b/extensions/shell/browser/api/shell_extensions_api_client.h deleted file mode 100644 index 41f548f..0000000 --- a/extensions/shell/browser/api/shell_extensions_api_client.h +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright 2014 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 EXTENSIONS_SHELL_BROWSER_API_SHELL_EXTENSIONS_API_CLIENT_H_ -#define EXTENSIONS_SHELL_BROWSER_API_SHELL_EXTENSIONS_API_CLIENT_H_ - -#include "base/memory/scoped_ptr.h" -#include "extensions/browser/api/extensions_api_client.h" - -namespace extensions { - -// Extra support for Chrome extensions APIs in app_shell. -class ShellExtensionsAPIClient : public ExtensionsAPIClient { - public: - ShellExtensionsAPIClient(); - virtual ~ShellExtensionsAPIClient(); - - // ExtensionsAPIClient implementation. - virtual device::HidService* GetHidService() OVERRIDE; - - private: - scoped_ptr<device::HidService> hid_service_; -}; - -} // namespace extensions - -#endif // EXTENSIONS_SHELL_BROWSER_API_SHELL_EXTENSIONS_API_CLIENT_H_ diff --git a/extensions/shell/browser/shell_device_client.cc b/extensions/shell/browser/shell_device_client.cc index b3c8168..55b277b 100644 --- a/extensions/shell/browser/shell_device_client.cc +++ b/extensions/shell/browser/shell_device_client.cc @@ -6,6 +6,7 @@ #include "base/logging.h" #include "content/public/browser/browser_thread.h" +#include "device/hid/hid_service.h" #include "device/usb/usb_service.h" namespace extensions { @@ -20,4 +21,10 @@ device::UsbService* ShellDeviceClient::GetUsbService() { content::BrowserThread::UI)); } +device::HidService* ShellDeviceClient::GetHidService() { + return device::HidService::GetInstance( + content::BrowserThread::GetMessageLoopProxyForThread( + content::BrowserThread::UI)); +} + } diff --git a/extensions/shell/browser/shell_device_client.h b/extensions/shell/browser/shell_device_client.h index 862d609..c1ede48 100644 --- a/extensions/shell/browser/shell_device_client.h +++ b/extensions/shell/browser/shell_device_client.h @@ -21,6 +21,7 @@ class ShellDeviceClient : device::DeviceClient { // device::DeviceClient implementation virtual device::UsbService* GetUsbService() OVERRIDE; + virtual device::HidService* GetHidService() OVERRIDE; private: DISALLOW_COPY_AND_ASSIGN(ShellDeviceClient); diff --git a/extensions/shell/browser/shell_extensions_browser_client.cc b/extensions/shell/browser/shell_extensions_browser_client.cc index 84e757c..659f88f 100644 --- a/extensions/shell/browser/shell_extensions_browser_client.cc +++ b/extensions/shell/browser/shell_extensions_browser_client.cc @@ -9,12 +9,12 @@ #include "base/prefs/testing_pref_store.h" #include "components/pref_registry/pref_registry_syncable.h" #include "components/user_prefs/user_prefs.h" +#include "extensions/browser/api/extensions_api_client.h" #include "extensions/browser/api/generated_api_registration.h" #include "extensions/browser/app_sorting.h" #include "extensions/browser/extension_function_registry.h" #include "extensions/browser/extension_prefs.h" #include "extensions/shell/browser/api/generated_api_registration.h" -#include "extensions/shell/browser/api/shell_extensions_api_client.h" #include "extensions/shell/browser/shell_app_sorting.h" #include "extensions/shell/browser/shell_extension_host_delegate.h" #include "extensions/shell/browser/shell_extension_system_factory.h" @@ -34,7 +34,7 @@ void RegisterPrefs(user_prefs::PrefRegistrySyncable* registry) { ShellExtensionsBrowserClient::ShellExtensionsBrowserClient( BrowserContext* context) - : browser_context_(context), api_client_(new ShellExtensionsAPIClient) { + : browser_context_(context), api_client_(new ExtensionsAPIClient) { // Set up the preferences service. base::PrefServiceFactory factory; factory.set_user_prefs(new TestingPrefStore); |