generic-library/webrtc
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Posix Thread: Removes the setting of the run function to NULL which could cause data race.

Browse Source 
BUG=http://code.google.com/p/chromium/issues/detail?id=103711
TESTED=Code analysis (no tools)

Review URL: https://webrtc-codereview.appspot.com/1008006

git-svn-id: http://webrtc.googlecode.com/svn/trunk@3388 4adac7df-926f-26a2-2b94-8c16560cd09d
This commit is contained in:
henrike@webrtc.org 2013-01-18 16:39:21 +00:00
parent 4ad64458cb
commit a3e6bec23a
3 changed files with 17 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
webrtc/system_wrappers/interface/thread_wrapper.h
View File

@ -52,8 +52,8 @@ class ThreadWrapper {
// prio Thread priority. May require root/admin rights. // prio Thread priority. May require root/admin rights.
// thread_name NULL terminated thread name, will be visable in the Windows // thread_name NULL terminated thread name, will be visable in the Windows
// debugger. // debugger.
static ThreadWrapper* CreateThread(ThreadRunFunction func = 0, static ThreadWrapper* CreateThread(ThreadRunFunction func,
ThreadObj obj = 0, ThreadObj obj,
ThreadPriority prio = kNormalPriority, ThreadPriority prio = kNormalPriority,
const char* thread_name = 0); const char* thread_name = 0);

38
webrtc/system_wrappers/source/thread_posix.cc
View File

@ -97,8 +97,7 @@ extern "C"
} }
} }
ThreadWrapper* ThreadPosix::Create(ThreadRunFunction func, ThreadWrapper* ThreadPosix::Create(ThreadRunFunction func, ThreadObj obj,
ThreadObj obj,
ThreadPriority prio, ThreadPriority prio,
const char* thread_name) { const char* thread_name) {
ThreadPosix* ptr = new ThreadPosix(func, obj, prio, thread_name); ThreadPosix* ptr = new ThreadPosix(func, obj, prio, thread_name);
@ -174,9 +173,6 @@ ThreadPosix::~ThreadPosix() {
bool ThreadPosix::Start(unsigned int& thread_id) bool ThreadPosix::Start(unsigned int& thread_id)
{ {
if (!run_function_) {
return false;
}
int result = pthread_attr_setdetachstate(&attr_, PTHREAD_CREATE_DETACHED); int result = pthread_attr_setdetachstate(&attr_, PTHREAD_CREATE_DETACHED);
// Set the stack stack size to 1M. // Set the stack stack size to 1M.
result |= pthread_attr_setstacksize(&attr_, 1024 * 1024); result |= pthread_attr_setstacksize(&attr_, 1024 * 1024);
@ -195,15 +191,17 @@ bool ThreadPosix::Start(unsigned int& thread_id)
if (result != 0) { if (result != 0) {
return false; return false;
} }
{
CriticalSectionScoped cs(crit_state_);
dead_ = false;
}
// Wait up to 10 seconds for the OS to call the callback function. Prevents // Wait up to 10 seconds for the OS to call the callback function. Prevents
// race condition if Stop() is called too quickly after start. // race condition if Stop() is called too quickly after start.
if (kEventSignaled != event_->Wait(WEBRTC_EVENT_10_SEC)) { if (kEventSignaled != event_->Wait(WEBRTC_EVENT_10_SEC)) {
WEBRTC_TRACE(kTraceError, kTraceUtility, -1, WEBRTC_TRACE(kTraceError, kTraceUtility, -1,
"posix thread event never triggered"); "posix thread event never triggered");
// Timed out. Something went wrong. // Timed out. Something went wrong.
run_function_ = NULL;
return true; return true;
} }
@ -291,7 +289,7 @@ bool ThreadPosix::Stop() {
// TODO(hellner) why not use an event here? // TODO(hellner) why not use an event here?
// Wait up to 10 seconds for the thread to terminate // Wait up to 10 seconds for the thread to terminate
for (int i = 0; i < 1000 && !dead; i++) { for (int i = 0; i < 1000 && !dead; ++i) {
timespec t; timespec t;
t.tv_sec = 0; t.tv_sec = 0;
t.tv_nsec = 10 * 1000 * 1000; t.tv_nsec = 10 * 1000 * 1000;
@ -312,7 +310,6 @@ void ThreadPosix::Run() {
{ {
CriticalSectionScoped cs(crit_state_); CriticalSectionScoped cs(crit_state_);
alive_ = true; alive_ = true;
dead_ = false;
} }
#if (defined(WEBRTC_LINUX) || defined(WEBRTC_ANDROID)) #if (defined(WEBRTC_LINUX) || defined(WEBRTC_ANDROID))
pid_ = GetThreadId(); pid_ = GetThreadId();
@ -331,22 +328,15 @@ void ThreadPosix::Run() {
"Thread without name started"); "Thread without name started");
} }
bool alive = true; bool alive = true;
do { bool run = true;
if (run_function_) { while (alive) {
if (!run_function_(obj_)) { run = run_function_(obj_);
alive = false; CriticalSectionScoped cs(crit_state_);
} if (!run) {
} else { alive_ = false;
alive = false;
} }
{ alive = alive_;
CriticalSectionScoped cs(crit_state_); }
if (!alive) {
alive_ = false;
}
alive = alive_;
}
} while (alive);
if (set_thread_name_) { if (set_thread_name_) {
// Don't set the name for the trace thread because it may cause a // Don't set the name for the trace thread because it may cause a

9
webrtc/system_wrappers/source/thread_unittest.cc
View File

@ -15,20 +15,13 @@
namespace webrtc { namespace webrtc {
TEST(ThreadTest, NullFunctionPointer) {
webrtc::scoped_ptr<ThreadWrapper> thread(
webrtc::ThreadWrapper::CreateThread());
unsigned int id = 42;
EXPECT_FALSE(thread->Start(id));
}
// Function that does nothing, and reports success. // Function that does nothing, and reports success.
bool NullRunFunction(void* obj) { bool NullRunFunction(void* obj) {
return true; return true;
} }
TEST(ThreadTest, StartStop) { TEST(ThreadTest, StartStop) {
ThreadWrapper* thread = ThreadWrapper::CreateThread(&NullRunFunction); ThreadWrapper* thread = ThreadWrapper::CreateThread(&NullRunFunction, NULL);
unsigned int id = 42; unsigned int id = 42;
ASSERT_TRUE(thread->Start(id)); ASSERT_TRUE(thread->Start(id));
EXPECT_TRUE(thread->Stop()); EXPECT_TRUE(thread->Stop());