PPAPI: Make GLES2 calls resilient to bad/dead resources.

PPAPI: Make GLES2 calls resilient to bad/dead resources.

Currently, if PPB_OpenGLES2 is passed a bad resource, or if a bad resource is
set prior to OpenGLES2 calls, our code happily dereferences a NULL pointer.
This CL tries to address that by handling failure in a way that's consistent
with our other Pepper interfaces.

It also appears that the "old" approach to locking would end up locking twice
per call. Once to get the PPB_GLES2Impl, then unlocked, and then locked
again inside the LockingCommandBuffer (meaning that the PPB_GLES2Impl
might not actually be valid at the time we dereference it!!). This change
simplifies that and removes the old/hacky way of locking.

BUG=275815
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226017

[dmichael (off chromium), 2013-09-25 20:05:13]

Antoine: Please focus on gpu/OpenGLES stuff.
Justin: Please focus on Pepper stuff.

https://codereview.chromium.org/24466004/diff/5001/gpu/command_buffer/build_g...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/24466004/diff/5001/gpu/command_buffer/build_g...
gpu/command_buffer/build_gles2_cmd_buffer.py:7699: error_return =
"GL_INVALID_ENUM"
I looked at the documentation of glCheckFramebufferStatus, and I couldn't find
any more appropriate error return than this.

Would it be within spec to return GL_INVALID_OPERATION instead? That's what I
wanted to do, but it isn't listed among possible returns for this function.

https://codereview.chromium.org/24466004/diff/5001/ppapi/proxy/ppapi_command_...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (left):

https://codereview.chromium.org/24466004/diff/5001/ppapi/proxy/ppapi_command_...
ppapi/proxy/ppapi_command_buffer_proxy.cc:78: // calling this function.
I can't find where the GetLastState actually gets called from...  can you help
me understand where, so I can make sure we're doing the right thing? As far as I
can tell, though, we can't get here without acquiring the ProxyLock.

https://codereview.chromium.org/24466004/diff/11001/ppapi/shared_impl/ppb_ope...
File ppapi/shared_impl/ppb_opengles2_shared.cc (left):

https://codereview.chromium.org/24466004/diff/11001/ppapi/shared_impl/ppb_ope...
ppapi/shared_impl/ppb_opengles2_shared.cc:21:
thunk::EnterResource<thunk::PPB_Graphics3D_API> enter_g3d(context, false);
In this code, we would acquire the ProxyLock (because EnterResource does so in
its constructor), and release it upon exiting scope.

https://codereview.chromium.org/24466004/diff/11001/ppapi/shared_impl/ppb_ope...
ppapi/shared_impl/ppb_opengles2_shared.cc:27:
GetGLES(context_id)->ActiveTexture(texture);
So in this case, we're dereferencing a bare GLES2Implementation pointer that
could potentially have been freed on another thread before we get here (or,
could have been NULL if context_id was bad). Then, ActiveTexture re-acquires the
ProxyLock.

[piman, 2013-09-25 22:23:54]

https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:1957: 'shader, count,
const_cast<const char* const*>(str), length'
Ah, sigh. A const got lost in translation and we shipped that...

https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:7666: file.Write(" 
CHECK(enter->succeeded());\n")
Why CHECK? This is done on every single GL call from the app, which can be a lot
(think 1000s per frame). I don't think we should have correctly written apps
suffer from badly written ones.

https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:7683: file.Write("  if
(enter.succeeded()) {\n")
So this has a non-0 cost. Can we quantify it on some NaCl games?
The crash suggests Flash is obviously doing something wrong, and I don't think
we should have correct apps suffer from that (we should fix Flash). Some
implementations of GL crash if there is no current context, and I think it is
reasonable to do the same in Pepper (no current context == invalid resource), as
it is today.
If this has no measurable cost, then we can keep it.

https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:7699: error_return =
"GL_INVALID_ENUM"
According to
http://www.opengl.org/sdk/docs/man3/xhtml/glCheckFramebufferStatus.xml I would
return 0. Note, this is the return value, not the GL error - the thing you get
with glGetError, which is what the "Errors" section of the man page refers to.
If you don't have a context, you can't get to the GL error anyway (glGetError
should also return 0).

https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:7701: error_return =
"GL_INVALID_OPERATION"
This should most likely be 0 too. If we want to be consistent with the spec, we
should audit each function. I haven't done so, but 0 seems like a reasonable
thing to do.

[dmichael (off chromium), 2013-09-26 19:26:33]

https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:1957: 'shader, count,
const_cast<const char* const*>(str), length'
On 2013/09/25 22:23:55, piman wrote:
> Ah, sigh. A const got lost in translation and we shipped that...
Well, I realized that we don't actually need this cast (it adds a const). It's
still not great, but we can't change the Pepper API without revving, and it
doesn't seem worth dealing with the version skew for that (since we'd have to
support both versions anyway).

https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:7666: file.Write(" 
CHECK(enter->succeeded());\n")
On 2013/09/25 22:23:55, piman wrote:
> Why CHECK? This is done on every single GL call from the app, which can be a
lot
> (think 1000s per frame). I don't think we should have correctly written apps
> suffer from badly written ones.
Yeah, that was probably overkill. The generated code basically guarantees it
doesn't happen. I'll make them DCHECKs.

https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:7683: file.Write("  if
(enter.succeeded()) {\n")
For calls that use the command buffer, this CL is a net win. If you look at the
old code (GetGLES) it does EnterResource, which acquires the Pepper Proxy lock.
Then, it immediately drops it. Later, we had a "LockingCommandBuffer" which
grabbed the lock _again_. We had a race condition in between, and acquired the
lock twice. Now, we acquire the lock once for the duration of the call, so in
those cases we're definitely better off.

Even in the case where the GLES command does not use the command buffer (I don't
know how many of them that is; I poked around the code a little just now, but
it's not obvious to me), this is an inlined function that compares the result to
PP_OK (0). I suspect it's in the noise compared to the lock.

I went and compared with From Dust, and the FPS looks pretty similar with and
without on my HP Z620, FWIW. In Release, both are around 30fps. In Debug, the
new patch is possibly somewhat higher fps in some scenes. Not scientific, or
anything.

And I agree, it looks like Flash is doing something wrong, but I was going for
the tradeoff of greater stability, given that I think it's low/no cost. I
suspect most of the failures are benign. Flash may end up crashing elsewhere in
some of those cases, but it will be more obviously a Flash problem then.

https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:7699: error_return =
"GL_INVALID_ENUM"
On 2013/09/25 22:23:55, piman wrote:
> According to
> http://www.opengl.org/sdk/docs/man3/xhtml/glCheckFramebufferStatus.xml I would
> return 0. Note, this is the return value, not the GL error - the thing you get
> with glGetError, which is what the "Errors" section of the man page refers to.
> If you don't have a context, you can't get to the GL error anyway (glGetError
> should also return 0).
Ah, thanks, I missed that sentence.

https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:7701: error_return =
"GL_INVALID_OPERATION"
On 2013/09/25 22:23:55, piman wrote:
> This should most likely be 0 too. If we want to be consistent with the spec,
we
> should audit each function. I haven't done so, but 0 seems like a reasonable
> thing to do.

Done.

I went ahead and looked at all the functions that return stuff and added a
couple of exceptions to returning 0.

[teravest, 2013-09-26 19:42:59]

It would be nice if there was a debug version of ProxyLock::AssertAcquired that
you could use to ensure that those methods are called with the ProxyLock held.
Is there no good way to do this today?

https://codereview.chromium.org/24466004/diff/48001/gpu/command_buffer/build_...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/24466004/diff/48001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:1219: # pepper_args_to_pass: A
string representing the parameters to pass from the
I don't think this is used. Can you get rid of the pepper_args_to_pass comment?

[piman, 2013-09-26 20:02:26]

On 2013/09/26 19:26:33, dmichael wrote:
>
https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
> File gpu/command_buffer/build_gles2_cmd_buffer.py (right):
> 
>
https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
> gpu/command_buffer/build_gles2_cmd_buffer.py:1957: 'shader, count,
> const_cast<const char* const*>(str), length'
> On 2013/09/25 22:23:55, piman wrote:
> > Ah, sigh. A const got lost in translation and we shipped that...
> Well, I realized that we don't actually need this cast (it adds a const). It's
> still not great, but we can't change the Pepper API without revving, and it
> doesn't seem worth dealing with the version skew for that (since we'd have to
> support both versions anyway).

Yeah, agreed. It's just sad that we missed it...

> 
>
https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
> gpu/command_buffer/build_gles2_cmd_buffer.py:7666: file.Write(" 
> CHECK(enter->succeeded());\n")
> On 2013/09/25 22:23:55, piman wrote:
> > Why CHECK? This is done on every single GL call from the app, which can be a
> lot
> > (think 1000s per frame). I don't think we should have correctly written apps
> > suffer from badly written ones.
> Yeah, that was probably overkill. The generated code basically guarantees it
> doesn't happen. I'll make them DCHECKs.
> 
>
https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
> gpu/command_buffer/build_gles2_cmd_buffer.py:7683: file.Write("  if
> (enter.succeeded()) {\n")
> For calls that use the command buffer, this CL is a net win. If you look at
the
> old code (GetGLES) it does EnterResource, which acquires the Pepper Proxy
lock.
> Then, it immediately drops it. Later, we had a "LockingCommandBuffer" which
> grabbed the lock _again_. We had a race condition in between, and acquired the
> lock twice. Now, we acquire the lock once for the duration of the call, so in
> those cases we're definitely better off.
> 
> Even in the case where the GLES command does not use the command buffer (I
don't
> know how many of them that is; I poked around the code a little just now, but
> it's not obvious to me), this is an inlined function that compares the result
to
> PP_OK (0). I suspect it's in the noise compared to the lock.
> 
> I went and compared with From Dust, and the FPS looks pretty similar with and
> without on my HP Z620, FWIW. In Release, both are around 30fps. In Debug, the
> new patch is possibly somewhat higher fps in some scenes. Not scientific, or
> anything.
> 
> And I agree, it looks like Flash is doing something wrong, but I was going for
> the tradeoff of greater stability, given that I think it's low/no cost. I
> suspect most of the failures are benign. Flash may end up crashing elsewhere
in
> some of those cases, but it will be more obviously a Flash problem then.

Ok, thanks for doing the leg work. I buy your explanation.

> 
>
https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
> gpu/command_buffer/build_gles2_cmd_buffer.py:7699: error_return =
> "GL_INVALID_ENUM"
> On 2013/09/25 22:23:55, piman wrote:
> > According to
> > http://www.opengl.org/sdk/docs/man3/xhtml/glCheckFramebufferStatus.xml I
would
> > return 0. Note, this is the return value, not the GL error - the thing you
get
> > with glGetError, which is what the "Errors" section of the man page refers
to.
> > If you don't have a context, you can't get to the GL error anyway
(glGetError
> > should also return 0).
> Ah, thanks, I missed that sentence.
> 
>
https://codereview.chromium.org/24466004/diff/11001/gpu/command_buffer/build_...
> gpu/command_buffer/build_gles2_cmd_buffer.py:7701: error_return =
> "GL_INVALID_OPERATION"
> On 2013/09/25 22:23:55, piman wrote:
> > This should most likely be 0 too. If we want to be consistent with the spec,
> we
> > should audit each function. I haven't done so, but 0 seems like a reasonable
> > thing to do.
> 
> Done.
> 
> I went ahead and looked at all the functions that return stuff and added a
> couple of exceptions to returning 0.

LGTM. Thanks!

[dmichael (off chromium), 2013-09-26 22:26:38]

https://codereview.chromium.org/24466004/diff/48001/gpu/command_buffer/build_...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/24466004/diff/48001/gpu/command_buffer/build_...
gpu/command_buffer/build_gles2_cmd_buffer.py:1219: # pepper_args_to_pass: A
string representing the parameters to pass from the
On 2013/09/26 19:42:59, teravest wrote:
> I don't think this is used. Can you get rid of the pepper_args_to_pass
comment?
Thanks, done. That was leftover from an earlier patch.

https://codereview.chromium.org/24466004/diff/65001/ppapi/proxy/ppapi_command...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://codereview.chromium.org/24466004/diff/65001/ppapi/proxy/ppapi_command...
ppapi/proxy/ppapi_command_buffer_proxy.cc:78:
ppapi::ProxyLock::AssertAcquiredDebugOnly();
I was able to confirm with From Dust that these are both called, though I don't
know where for the case of GetLastState. CHECK(false) caused a crash.

However, with the AssertAcquiredDebugOnly, no crash. So it appears we *do* have
the lock here now, at no extra charge.

[teravest, 2013-09-26 23:31:14]

lgtm

On Thu, Sep 26, 2013 at 4:26 PM,  <dmichael@chromium.org> wrote:
>
>
https://codereview.chromium.org/24466004/diff/48001/gpu/command_buffer/build_...
> File gpu/command_buffer/build_gles2_cmd_buffer.py (right):
>
>
https://codereview.chromium.org/24466004/diff/48001/gpu/command_buffer/build_...
> gpu/command_buffer/build_gles2_cmd_buffer.py:1219: #
> pepper_args_to_pass: A string representing the parameters to pass from
> the
> On 2013/09/26 19:42:59, teravest wrote:
>>
>> I don't think this is used. Can you get rid of the pepper_args_to_pass
>
> comment?
> Thanks, done. That was leftover from an earlier patch.
>
>
https://codereview.chromium.org/24466004/diff/65001/ppapi/proxy/ppapi_command...
> File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):
>
>
https://codereview.chromium.org/24466004/diff/65001/ppapi/proxy/ppapi_command...
> ppapi/proxy/ppapi_command_buffer_proxy.cc:78:
> ppapi::ProxyLock::AssertAcquiredDebugOnly();
> I was able to confirm with From Dust that these are both called, though
> I don't know where for the case of GetLastState. CHECK(false) caused a
> crash.
>
> However, with the AssertAcquiredDebugOnly, no crash. So it appears we
> *do* have the lock here now, at no extra charge.
>
> https://codereview.chromium.org/24466004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-09-27 16:18:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/24466004/77001

[commit-bot: I haz the power, 2013-09-27 16:18:27]

Failed to apply patch for ppapi/proxy/ppb_graphics_3d_proxy.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ppapi/proxy/ppb_graphics_3d_proxy.cc
  Hunk #4 FAILED at 114.
  Hunk #5 succeeded at 135 (offset 4 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
ppapi/proxy/ppb_graphics_3d_proxy.cc.rej

Patch:       ppapi/proxy/ppb_graphics_3d_proxy.cc
Index: ppapi/proxy/ppb_graphics_3d_proxy.cc
diff --git a/ppapi/proxy/ppb_graphics_3d_proxy.cc
b/ppapi/proxy/ppb_graphics_3d_proxy.cc
index
204466baf62bb482d4c00c184efad5e87442f4b5..9c50243dbf8fda2823bf2a4c25153137d23f220c
100644
--- a/ppapi/proxy/ppb_graphics_3d_proxy.cc
+++ b/ppapi/proxy/ppb_graphics_3d_proxy.cc
@@ -12,7 +12,6 @@
 #include "ppapi/proxy/ppapi_command_buffer_proxy.h"
 #include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/shared_impl/ppapi_globals.h"
-#include "ppapi/shared_impl/proxy_lock.h"
 #include "ppapi/thunk/enter.h"
 #include "ppapi/thunk/resource_creation_api.h"
 #include "ppapi/thunk/thunk.h"
@@ -53,132 +52,8 @@ gpu::CommandBuffer::State GetErrorState() {
 
 }  // namespace
 
-// This class just wraps a CommandBuffer and optionally locks around every
-// method. This is used to ensure that we have the Proxy lock any time we enter
-// PpapiCommandBufferProxy.
-//
-// Note, for performance reasons, most of this code is not truly thread
-// safe in the sense of multiple threads concurrently rendering to the same
-// Graphics3D context; this isn't allowed, and will likely either crash or
-// result in undefined behavior.  It is assumed that the thread which creates
-// the Graphics3D context will be the thread on which subsequent gl rendering
-// will be done. This is why it is okay to read need_to_lock_ without the lock;
-// it should only ever be read and written on the same thread where the context
-// was created.
-//
-// TODO(nfullagar): At some point, allow multiple threads to concurrently
render
-// each to its own context.  First step is to allow a single thread (either
main
-// thread or background thread) to render to a single Graphics3D context.
-class Graphics3D::LockingCommandBuffer : public gpu::CommandBuffer {
- public:
-  explicit LockingCommandBuffer(gpu::CommandBuffer* gpu_command_buffer)
-      : gpu_command_buffer_(gpu_command_buffer), need_to_lock_(true) {
-  }
-  virtual ~LockingCommandBuffer() {
-  }
-  void set_need_to_lock(bool need_to_lock) { need_to_lock_ = need_to_lock; }
-  bool need_to_lock() const { return need_to_lock_; }
-
- private:
-  // MaybeLock acquires the proxy lock on construction if and only if
-  // need_to_lock is true. If it acquired the lock, it releases it on
-  // destruction. If need_to_lock is false, then the lock must already be held.
-  struct MaybeLock {
-    explicit MaybeLock(bool need_to_lock) : locked_(need_to_lock) {
-      if (need_to_lock)
-        ppapi::ProxyLock::Acquire();
-      else
-        ppapi::ProxyLock::AssertAcquired();
-    }
-    ~MaybeLock() {
-      if (locked_)
-        ppapi::ProxyLock::Release();
-    }
-   private:
-    bool locked_;
-  };
-
-  // gpu::CommandBuffer implementation:
-  virtual bool Initialize() OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    return gpu_command_buffer_->Initialize();
-  }
-  virtual State GetState() OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    return gpu_command_buffer_->GetState();
-  }
-  virtual State GetLastState() OVERRIDE {
-    // During a normal scene, the vast majority of calls are to GetLastState().
-    // We don't allow multi-threaded rendering on the same contex, so for
-    // performance reasons, avoid the global lock for this entry point.  We can
-    // get away with this here because the underlying implementation of
-    // GetLastState() is trivial and does not involve global or shared state
-    // between other contexts.
-    // TODO(nfullagar): We can probably skip MaybeLock for other methods, but
-    // the performance gain may not be worth it.
-    //
-    // MaybeLock lock(need_to_lock_);
-    return gpu_command_buffer_->GetLastState();
-  }
-  virtual int32 GetLastToken() OVERRIDE {
-    return GetLastState().token;
-  }
-  virtual void Flush(int32 put_offset) OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    gpu_command_buffer_->Flush(put_offset);
-  }
-  virtual State FlushSync(int32 put_offset, int32 last_known_get) OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    return gpu_command_buffer_->FlushSync(put_offset, last_known_get);
-  }
-  virtual void SetGetBuffer(int32 transfer_buffer_id) OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    gpu_command_buffer_->SetGetBuffer(transfer_buffer_id);
-  }
-  virtual void SetGetOffset(int32 get_offset) OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    gpu_command_buffer_->SetGetOffset(get_offset);
-  }
-  virtual gpu::Buffer CreateTransferBuffer(size_t size,
-                                           int32* id) OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    return gpu_command_buffer_->CreateTransferBuffer(size, id);
-  }
-  virtual void DestroyTransferBuffer(int32 id) OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    gpu_command_buffer_->DestroyTransferBuffer(id);
-  }
-  virtual gpu::Buffer GetTransferBuffer(int32 id) OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    return gpu_command_buffer_->GetTransferBuffer(id);
-  }
-  virtual void SetToken(int32 token) OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    gpu_command_buffer_->SetToken(token);
-  }
-  virtual void SetParseError(gpu::error::Error error) OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    gpu_command_buffer_->SetParseError(error);
-  }
-  virtual void SetContextLostReason(
-      gpu::error::ContextLostReason reason) OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    gpu_command_buffer_->SetContextLostReason(reason);
-  }
-  virtual uint32 InsertSyncPoint() OVERRIDE {
-    MaybeLock lock(need_to_lock_);
-    return gpu_command_buffer_->InsertSyncPoint();
-  }
-
-  // Weak pointer - see class Graphics3D for the scopted_ptr.
-  gpu::CommandBuffer* gpu_command_buffer_;
-
-  bool need_to_lock_;
-};
-
 Graphics3D::Graphics3D(const HostResource& resource)
-    : PPB_Graphics3D_Shared(resource),
-      num_already_locked_calls_(0) {
+    : PPB_Graphics3D_Shared(resource) {
 }
 
 Graphics3D::~Graphics3D() {
@@ -193,10 +68,7 @@ bool Graphics3D::Init(gpu::gles2::GLES2Implementation*
share_gles2) {
 
   command_buffer_.reset(
       new PpapiCommandBufferProxy(host_resource(), dispatcher));
-  locking_command_buffer_.reset(
-      new LockingCommandBuffer(command_buffer_.get()));
 
-  ScopedNoLocking already_locked(this);
   return CreateGLES2Impl(kCommandBufferSize, kTransferBufferSize,
                          share_gles2);
 }
@@ -242,14 +114,10 @@ uint32_t Graphics3D::InsertSyncPoint() {
 }
 
 gpu::CommandBuffer* Graphics3D::GetCommandBuffer() {
-  return locking_command_buffer_.get();
+  return command_buffer_.get();
 }
 
 int32 Graphics3D::DoSwapBuffers() {
-  // gles2_impl()->SwapBuffers() results in CommandBuffer calls, and we already
-  // have the proxy lock.
-  ScopedNoLocking already_locked(this);
-
   gles2_impl()->SwapBuffers();
   IPC::Message* msg = new PpapiHostMsg_PPBGraphics3D_SwapBuffers(
       API_ID_PPB_GRAPHICS_3D, host_resource());
@@ -259,31 +127,6 @@ int32 Graphics3D::DoSwapBuffers() {
   return PP_OK_COMPLETIONPENDING;
 }
 
-void Graphics3D::PushAlreadyLocked() {
-  ppapi::ProxyLock::AssertAcquired();
-  if (!locking_command_buffer_) {
-    NOTREACHED();
-    return;
-  }
-  if (num_already_locked_calls_ == 0)
-    locking_command_buffer_->set_need_to_lock(false);
-  ++num_already_locked_calls_;
-}
-
-void Graphics3D::PopAlreadyLocked() {
-  // We must have Pushed before we can Pop.
-  DCHECK(!locking_command_buffer_->need_to_lock());
-  DCHECK_GT(num_already_locked_calls_, 0);
-  ppapi::ProxyLock::AssertAcquired();
-  if (!locking_command_buffer_) {
-    NOTREACHED();
-    return;
-  }
-  --num_already_locked_calls_;
-  if (num_already_locked_calls_ == 0)
-    locking_command_buffer_->set_need_to_lock(true);
-}
-
 PPB_Graphics3D_Proxy::PPB_Graphics3D_Proxy(Dispatcher* dispatcher)
     : InterfaceProxy(dispatcher),
       callback_factory_(this) {

[commit-bot: I haz the power, 2013-09-27 16:52:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/24466004/81001

[commit-bot: I haz the power, 2013-09-27 20:33:17]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-27 21:35:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/24466004/77002

[commit-bot: I haz the power, 2013-09-28 01:50:59]

Retried try job too often on linux_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-30 16:29:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/24466004/77002

[commit-bot: I haz the power, 2013-09-30 18:36:04]

Change committed as 226017