gpu: Fix corrupted state due to virtual MakeCurrent race.

gpu/command_buffer/service/gl_context_virtual.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "gpu/command_buffer/service/gl_context_virtual.h"
 
 #include "gpu/command_buffer/service/gl_state_restorer_impl.h"
 #include "gpu/command_buffer/service/gles2_cmd_decoder.h"
 #include "ui/gl/gl_surface.h"
 
@@ skipping 35 matching lines @@
   return true;
 }
 
 void GLContextVirtual::Destroy() {
   shared_context_->OnDestroyVirtualContext(this);
   shared_context_ = NULL;
   display_ = NULL;
 }
 
 bool GLContextVirtual::MakeCurrent(gfx::GLSurface* surface) {
   if (decoder_.get() && decoder_->initialized())

[greggman, 2013/05/24 00:56:47]

nit: if there's braces on one if clause there should be braces on both.

     shared_context_->MakeVirtuallyCurrent(this, surface);
-  else
-    shared_context_->MakeCurrent(surface);
+  else {
+    // We can't restore our state yet, so unset the current virtual context,
+    // so that the next context (if it isn't this) will still know to restore.
+    shared_context_->MakeVirtuallyCurrent(NULL, NULL);

[no sievers, 2013/05/23 15:58:36]

Hmm I don't understand how current_context_ in the virtual API could have gotten
set to |this| before if we don't have a decoder yet.

Is it maybe a new virtual context getting allocated at the same address?

We do reset current_context_ in OnDestroyVirtualContext(), but I recently
noticed we don't do it for ReleaseCurrent().

[no sievers, 2013/05/23 16:04:43]

Actually I don't understand the comment either. We check for 'virtual_context !=
current_context_' for restoring state. So the 'next context (if it isn't this)'
will have a pointer different from this.

I do believe you that the bug is somewhere here though :)

[no sievers, 2013/05/23 17:04:34]

Ok I think I get it now after reading your log from the bug.
The problem is that we allow the context to be made current bypassing the
virtual GL API here. I had added this for some cleanup calls that might happen
after the stub goes away (TextureImageTransportSurface) and not so much for the
initialization. But now I wonder: Seems like the MakeCurrent before decoder
initialization in GpuCommandBufferStub::OnInitialize would always fall into the
else-path, right?

Since there is also the ReleaseCurrent() bug, would it make sense to add a
ReleaseVirtuallyCurrent() or whatever to GLContext?

[epenner, 2013/05/23 18:00:03]

If it was added for cleanup calls, those clean up calls could also corrupt the
virtually current context (unless we add this code to insure a restore).

+
+    // But still make the real context current, if needed.
+    if (!IsCurrent(surface))
+      shared_context_->MakeCurrent(surface);
+  }
   return true;
 }
 
 void GLContextVirtual::ReleaseCurrent(gfx::GLSurface* surface) {
   if (IsCurrent(surface))
     shared_context_->ReleaseCurrent(surface);
 }
 
 bool GLContextVirtual::IsCurrent(gfx::GLSurface* surface) {
   // If it's a real surface it needs to be current.
@@ skipping 35 matching lines @@
 
 void GLContextVirtual::SetUnbindFboOnMakeCurrent() {
   shared_context_->SetUnbindFboOnMakeCurrent();
 }
 
 GLContextVirtual::~GLContextVirtual() {
   Destroy();
 }
 
 }  // namespace gpu