Make ShareGroup thread safe

gpu/command_buffer/client/atomicops.cc

Index: gpu/command_buffer/client/atomicops.cc
diff --git a/gpu/command_buffer/client/atomicops.cc b/gpu/command_buffer/client/atomicops.cc
index 142ab9c6f6448d0532333e247e9629f6d366e0e5..7480286f85ce8160e90b341a91a16b70df49e91b 100644
--- a/gpu/command_buffer/client/atomicops.cc
+++ b/gpu/command_buffer/client/atomicops.cc
@@ -3,9 +3,13 @@
 // found in the LICENSE file.
 
 #include "../client/atomicops.h"
+#include "../common/logging.h"
 
 #if !defined(__native_client__)
 #include "base/atomicops.h"
+#include "base/synchronization/lock.h"
+#else
+#include <pthread.h>
 #endif
 
 namespace gpu {
@@ -18,5 +22,77 @@ void MemoryBarrier() {
 #endif
 }
 
+#if defined(__native_client__)
+
+class LockImpl {
+ public:
+  LockImpl()
+       : acquired_(false) {
+    pthread_mutex_init(&mutex_, NULL);
+  }
+
+  ~LockImpl() {
+    pthread_mutex_destroy(&mutex_);
+  }
+
+  void Acquire() {
+    GPU_DCHECK(!acquired_);

[apatrick_chromium, 2012/03/29 21:03:58]

This should not assert if another thread has acquired the lock.

[greggman, 2012/03/29 21:25:25]

Done.

+    pthread_mutex_lock(&mutex_);
+    acquired_ = true;
+  }
+
+  void Release() {
+    GPU_DCHECK(acquired_);
+    pthread_mutex_unlock(&mutex_);
+    acquired_ = false;

[apatrick_chromium, 2012/03/29 21:03:58]

There's a race here. Once the mutex is unlocked, another thread can lock it and
set acquired_ to true before it is set to false on this line. Then acquired_ is
in an invalid state.

[greggman, 2012/03/29 21:25:25]

Done.

+  }
+
+  bool Try() {
+    GPU_DCHECK(!acquired_);
+    acquired_ = pthread_mutex_trylock(&mutex_) == 0;

[apatrick_chromium, 2012/03/29 21:03:58]

acquired_ should not be set to false if trylock fails. Another thread might have
the lock.

[greggman, 2012/03/29 21:25:25]

Done.

+    return acquired_;

[apatrick_chromium, 2012/03/29 21:03:58]

Another thread might set this to true even though the calling thread did not
successfully get the lock.

[greggman, 2012/03/29 21:25:25]

Done.

+  }
+
+  void AssertAcquired() const {
+    GPU_DCHECK(acquired_);
+  }
+
+ private:
+  bool acquired_;
+  pthread_mutex_t mutex_;
+
+  DISALLOW_COPY_AND_ASSIGN(LockImpl);
+};
+
+#else  // !__native_client__
+
+struct LockImpl : public base::Lock {

[apatrick_chromium, 2012/03/29 21:03:58]

This could be a typedef.

[greggman, 2012/03/29 21:25:25]

Done.

[greggman, 2012/03/29 21:31:32]

I get an error when it's a typedef because of the forward declaration on
atomicops.h. Maybe I don't understand the error

../../gpu/command_buffer/client/atomicops.cc:70: error: conflicting declaration
'typedef class base::Lock gpu::LockImpl'
../../gpu/command_buffer/client/../client/atomicops.h:16: error: 'struct
gpu::LockImpl' has a previous declaration as 'struct gpu::LockImpl'

+};
+
+#endif  // !__native_client__
+
+Lock::Lock()
+    : lock_(new LockImpl()) {
+}
+
+Lock::~Lock() {
+}
+
+void Lock::Acquire() {
+  lock_->Acquire();
+}
+
+void Lock::Release() {
+  lock_->Release();
+}
+
+bool Lock::Try() {
+  return lock_->Try();
+}
+
+void Lock::AssertAcquired() const {
+  return lock_->AssertAcquired();
+}
+
 }  // namespace gpu