From 724e0ccfe7866c77be5579d0a682507e307b2e98 Mon Sep 17 00:00:00 2001 From: "plundblad@chromium.org" Date: Tue, 24 Sep 2013 16:44:08 +0000 Subject: [PATCH] Fix a threading bug in the brlapi basec braille controller This CL enables brlapi support by default in builds for chromeos (it's already enabled by default in chromeos itself, so that part is a noop). This revealed a threading bug in the implementation which is also fixed my moving a file_path_watcher from the IO thread to the FILE threadd. BUG=178559 Review URL: https://chromiumcodereview.appspot.com/24325002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@225008 0039d316-1c4b-4281-b951-d872f2087c98 --- build/common.gypi | 5 +++ .../braille_controller_brlapi.cc | 36 ++++++++++++++++------ .../braille_controller_brlapi.h | 10 ++++-- .../braille_display_private_apitest.cc | 4 +-- 4 files changed, 41 insertions(+), 14 deletions(-) diff --git a/build/common.gypi b/build/common.gypi index 08e7198f61a4..b9cfe088568b 100644 --- a/build/common.gypi +++ b/build/common.gypi @@ -1784,6 +1784,11 @@ 'arm_float_abi%': '', 'arm_thumb%': 0, }], + + # Enable brlapi by default for chromeos. + [ 'chromeos==1', { + 'use_brlapi%': 1, + }], ], diff --git a/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc b/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc index 4ab1194194a5..0fe8566b496a 100644 --- a/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc +++ b/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc @@ -134,7 +134,7 @@ void BrailleControllerImpl::SetCreateBrlapiConnectionForTesting( } void BrailleControllerImpl::PokeSocketDirForTesting() { - OnSocketDirChanged(base::FilePath(), false /*error*/); + OnSocketDirChangedOnIOThread(); } void BrailleControllerImpl::StartConnecting() { @@ -146,26 +146,44 @@ void BrailleControllerImpl::StartConnecting() { if (!libbrlapi_loader_.loaded()) { return; } + // One could argue that there is a race condition between starting to + // watch the socket asynchonously and trying to connect. While this is true, + // we are actually retrying to connect for a while, so this doesn't matter + // in practice. + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, base::Bind( + &BrailleControllerImpl::StartWatchingSocketDirOnFileThread, + base::Unretained(this))); + ResetRetryConnectHorizon(); + TryToConnect(); +} + +void BrailleControllerImpl::StartWatchingSocketDirOnFileThread() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); base::FilePath brlapi_dir(BRLAPI_SOCKETPATH); if (!file_path_watcher_.Watch( brlapi_dir, false, base::Bind( - &BrailleControllerImpl::OnSocketDirChanged, + &BrailleControllerImpl::OnSocketDirChangedOnFileThread, base::Unretained(this)))) { LOG(WARNING) << "Couldn't watch brlapi directory " << BRLAPI_SOCKETPATH; - return; } - ResetRetryConnectHorizon(); - TryToConnect(); } -void BrailleControllerImpl::OnSocketDirChanged(const base::FilePath& path, - bool error) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); - DCHECK(libbrlapi_loader_.loaded()); +void BrailleControllerImpl::OnSocketDirChangedOnFileThread( + const base::FilePath& path, bool error) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); if (error) { LOG(ERROR) << "Error watching brlapi directory: " << path.value(); return; } + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, base::Bind( + &BrailleControllerImpl::OnSocketDirChangedOnIOThread, + base::Unretained(this))); +} + +void BrailleControllerImpl::OnSocketDirChangedOnIOThread() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); LOG(INFO) << "BrlAPI directory changed"; // Every directory change resets the max retry time to the appropriate delay // into the future. diff --git a/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.h b/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.h index ff8cf3add787..20fbf9d9aec5 100644 --- a/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.h +++ b/chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.h @@ -52,7 +52,9 @@ class BrailleControllerImpl : public BrailleController { // Tries to connect and starts watching for new brlapi servers. // No-op if already called. void StartConnecting(); - void OnSocketDirChanged(const base::FilePath& path, bool error); + void StartWatchingSocketDirOnFileThread(); + void OnSocketDirChangedOnFileThread(const base::FilePath& path, bool error); + void OnSocketDirChangedOnIOThread(); void TryToConnect(); void ResetRetryConnectHorizon(); void ScheduleTryToConnect(); @@ -63,12 +65,11 @@ class BrailleControllerImpl : public BrailleController { void DispatchKeyEvent(scoped_ptr event); void DispatchOnDisplayStateChanged(scoped_ptr new_state); - LibBrlapiLoader libbrlapi_loader_; CreateBrlapiConnectionFunction create_brlapi_connection_function_; // Manipulated on the IO thread. + LibBrlapiLoader libbrlapi_loader_; scoped_ptr connection_; - base::FilePathWatcher file_path_watcher_; bool started_connecting_; bool connect_scheduled_; base::Time retry_connect_horizon_; @@ -76,6 +77,9 @@ class BrailleControllerImpl : public BrailleController { // Manipulated on the UI thread. ObserverList observers_; + // Manipulated on the FILE thread. + base::FilePathWatcher file_path_watcher_; + friend struct DefaultSingletonTraits; DISALLOW_COPY_AND_ASSIGN(BrailleControllerImpl); diff --git a/chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc b/chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc index 88ce9b67422b..436ea52963b3 100644 --- a/chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc +++ b/chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc @@ -88,7 +88,7 @@ class MockBrlapiConnection : public BrlapiConnection { return true; } - virtual int ReadKey(brlapi_keyCode_t* key_code) { + virtual int ReadKey(brlapi_keyCode_t* key_code) OVERRIDE { if (!data_->pending_keys.empty()) { int queued_key_code = data_->pending_keys.front(); data_->pending_keys.pop_front(); @@ -120,7 +120,7 @@ class MockBrlapiConnection : public BrlapiConnection { class BrailleDisplayPrivateApiTest : public ExtensionApiTest { public: - virtual void SetUpInProcessBrowserTestFixture() { + virtual void SetUpInProcessBrowserTestFixture() OVERRIDE { ExtensionApiTest::SetUpInProcessBrowserTestFixture(); connection_data_.connected = false; connection_data_.display_size = 0; -- 2.11.4.GIT