gpu: Fix corrupted state due to virtual MakeCurrent race.

Reland "GPU: Reduce MakeCurrent calls to fix Orange San Diego""

This patch now also always calls MakeVirtuallyCurrent during initialize and
MakeCurrent, such that the next context will always restore. The context state is
restored only if the API has changed or the virtually current context has changed,
so we should never skip calling MakeVirtuallyCurrent.

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

[epenner, 2013-05-23 09:09:22]

Ptal.

I just figured out why this didn't happen before. Some real MakeCurrent calls
set the GL-API, which then triggers a state restore.

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

https://codereview.chromium.org/15841002/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gl_context_virtual.cc (right):

https://codereview.chromium.org/15841002/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gl_context_virtual.cc:61:
shared_context_->MakeVirtuallyCurrent(NULL, NULL);
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:01:24]

On 2013/05/23 15:58:36, Daniel Sievers wrote:
>
https://codereview.chromium.org/15841002/diff/1/gpu/command_buffer/service/gl...
> File gpu/command_buffer/service/gl_context_virtual.cc (right):
> 
>
https://codereview.chromium.org/15841002/diff/1/gpu/command_buffer/service/gl...
> gpu/command_buffer/service/gl_context_virtual.cc:61:
> shared_context_->MakeVirtuallyCurrent(NULL, NULL);
> 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().

Also I *think* it's ok that we don't restore the state the first time we make
current before initializing the decoder. I think the decoder might be already
doing it. Gregg can probably clarify. Every flush received after any commands
have been issued would cause a GLContextVirtual::MakeCurrent.

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

https://codereview.chromium.org/15841002/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gl_context_virtual.cc (right):

https://codereview.chromium.org/15841002/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gl_context_virtual.cc:61:
shared_context_->MakeVirtuallyCurrent(NULL, NULL);
On 2013/05/23 15:58:36, Daniel Sievers wrote:
> 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().

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]

https://codereview.chromium.org/15841002/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gl_context_virtual.cc (right):

https://codereview.chromium.org/15841002/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gl_context_virtual.cc:61:
shared_context_->MakeVirtuallyCurrent(NULL, NULL);
On 2013/05/23 16:04:43, Daniel Sievers wrote:
> On 2013/05/23 15:58:36, Daniel Sievers wrote:
> > 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().
> 
> 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 :)

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 17:57:11]

I think you mostly described the issue, but let me rephrase.  The problem is,
for a new virtual context we call:
- Initialize
- MakeCurrent (hits 'else' path)
- Set a bunch of state in decoder

At this point, the virtual API still hasn't been touched, so all we have done is
corrupt the old context. Now there is two possibilities
1.) We call MakeCurrent a second time for the new context and we update the
virtual API.
2.) We call MakeCurrent on the old context. In this case the old context thinks
its still current, but it is corrupted.

I don't actually see the ReleaseCurrent() bug, could you describe?  But in any
case we could have ReleaseVirtuallyCurrent() instead of
MakeVirtuallyCurrent(NULL,NULL).

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

https://codereview.chromium.org/15841002/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gl_context_virtual.cc (right):

https://codereview.chromium.org/15841002/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gl_context_virtual.cc:61:
shared_context_->MakeVirtuallyCurrent(NULL, NULL);
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).

[epenner, 2013-05-23 18:04:20]

Note that previously if we always forwarded to the real MakeCurrent, that would
change the GL-API, which would insure a restore in a round about way.

[epenner, 2013-05-24 00:52:49]

On 2013/05/23 18:04:20, epenner wrote:
> Note that previously if we always forwarded to the real MakeCurrent, that
would
> change the GL-API, which would insure a restore in a round about way.

After Daniel and I discussed, we realized we could use MakeVirtuallyCurrent for
almost all cases if we check for initialization in there.

I'll run against the tests to be sure.

Ptal.

[greggman, 2013-05-24 00:56:02]

If it works great. Tangentially, does

   static GLApi* g_gl;
  
need to be per thread?

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

lgtm

https://codereview.chromium.org/15841002/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gl_context_virtual.cc (right):

https://codereview.chromium.org/15841002/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gl_context_virtual.cc:56: if (decoder_.get() &&
decoder_->initialized())
nit: if there's braces on one if clause there should be braces on both.

[no sievers, 2013-05-24 01:38:22]

On 2013/05/24 00:56:02, greggman wrote:
> If it works great. Tangentially, does
> 
>    static GLApi* g_gl;
>   
> need to be per thread?

I think so.

lgtm. Hopefully we can get rid of ever bypassing MakeVirtuallyCurrent() if we
can reorder tear-down in GpuCommandBufferStub::Destroy(). (You might actually
want to indicate in your TODO comment that we only allow the bypass for exactly
that reason.)

[commit-bot: I haz the power, 2013-05-28 07:50:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/15841002/18002

[commit-bot: I haz the power, 2013-05-28 14:43:57]

Change committed as 202561