Make ShareGroup thread safe

Make ShareGroup thread safe

In another CL I'll refactor the id handlers AND
make the places sharing resources correctly set things
up.

TEST=none
BUG=120297
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=130034

[greggman, 2012-03-29 20:49:06]

Hey Al, PTAL

I'm open to suggestions for changes. The SetGLES2ImplementationForDestruction
for destruction hack is kind of strange but I don't really know what else to do.
Basically the ShareGroup needs to be able to talk across the command buffer on
destruction and it uses a GLES2Implementation to do this. But the
GLES2Implemenation owns the ShareGroup so I don't know when it will be freed.

Instead, before I release a reference to a ShareGroup I call 
SetGLES2ImplementationForDestruction and then release the reference. If this
happens to be the last reference it will use that GLES2Implementation to free
resources.

I can actually probably remove this when I make the rest of the code use
ShareGroup because freeing the last ShareGroup at that point will also mean the
corresponding ContextGroup should be released on the service, meaning the
ShareGroup doesn't have to free anything on destruction. It only has to do this
now because there's a ShareGroup per GLES2Implementation.

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

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
File gpu/command_buffer/client/atomicops.cc (right):

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
gpu/command_buffer/client/atomicops.cc:39: GPU_DCHECK(!acquired_);
This should not assert if another thread has acquired the lock.

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
gpu/command_buffer/client/atomicops.cc:47: acquired_ = false;
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.

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
gpu/command_buffer/client/atomicops.cc:52: acquired_ =
pthread_mutex_trylock(&mutex_) == 0;
acquired_ should not be set to false if trylock fails. Another thread might have
the lock.

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
gpu/command_buffer/client/atomicops.cc:53: return acquired_;
Another thread might set this to true even though the calling thread did not
successfully get the lock.

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
gpu/command_buffer/client/atomicops.cc:69: struct LockImpl : public base::Lock {
This could be a typedef.

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
File gpu/command_buffer/client/atomicops.h (right):

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
gpu/command_buffer/client/atomicops.h:40: lock_.AssertAcquired();
What if another thread has the lock?

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

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
File gpu/command_buffer/client/atomicops.cc (right):

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
gpu/command_buffer/client/atomicops.cc:39: GPU_DCHECK(!acquired_);
On 2012/03/29 21:03:58, apatrick_chromium wrote:
> This should not assert if another thread has acquired the lock.

Done.

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
gpu/command_buffer/client/atomicops.cc:47: acquired_ = false;
On 2012/03/29 21:03:58, apatrick_chromium wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
gpu/command_buffer/client/atomicops.cc:52: acquired_ =
pthread_mutex_trylock(&mutex_) == 0;
On 2012/03/29 21:03:58, apatrick_chromium wrote:
> acquired_ should not be set to false if trylock fails. Another thread might
have
> the lock.

Done.

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
gpu/command_buffer/client/atomicops.cc:53: return acquired_;
On 2012/03/29 21:03:58, apatrick_chromium wrote:
> Another thread might set this to true even though the calling thread did not
> successfully get the lock.

Done.

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
gpu/command_buffer/client/atomicops.cc:69: struct LockImpl : public base::Lock {
On 2012/03/29 21:03:58, apatrick_chromium wrote:
> This could be a typedef.

Done.

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

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
File gpu/command_buffer/client/atomicops.cc (right):

https://chromiumcodereview.appspot.com/9918027/diff/1/gpu/command_buffer/clie...
gpu/command_buffer/client/atomicops.cc:69: struct LockImpl : public base::Lock {
On 2012/03/29 21:03:58, apatrick_chromium wrote:
> This could be a typedef.

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'

[apatrick_chromium, 2012-03-29 21:36:44]

lgtm

[commit-bot: I haz the power, 2012-03-29 23:19:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/9918027/7001

[commit-bot: I haz the power, 2012-03-30 00:49:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/9918027/15001

[commit-bot: I haz the power, 2012-03-30 05:24:21]

Try job failure for 9918027-15001 (retry) (retry) on win_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-30 08:49:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/9918027/15001

[commit-bot: I haz the power, 2012-03-30 11:32:10]

Try job failure for 9918027-15001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-30 17:49:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/9918027/15001

[commit-bot: I haz the power, 2012-03-30 20:40:25]

Try job failure for 9918027-15001 (retry) on win_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-30 20:52:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/9918027/15001

[commit-bot: I haz the power, 2012-03-30 23:59:54]

Try job failure for 9918027-15001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-31 01:19:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/9918027/15001

[commit-bot: I haz the power, 2012-03-31 03:19:03]

Change committed as 130034