gpu: Fix corrupted state due to virtual MakeCurrent race.

gpu/command_buffer/service/gl_context_virtual.cc

Index: gpu/command_buffer/service/gl_context_virtual.cc
diff --git a/gpu/command_buffer/service/gl_context_virtual.cc b/gpu/command_buffer/service/gl_context_virtual.cc
index d142e01fb13a567d945674d6a5980d97636d8c54..6a3dabe560f9edbd7ecc0dd2dc94fa7ce9b7ebc2 100644
--- a/gpu/command_buffer/service/gl_context_virtual.cc
+++ b/gpu/command_buffer/service/gl_context_virtual.cc
@@ -55,8 +55,15 @@ void GLContextVirtual::Destroy() {
 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;
 }