Report lost context from GLES2Implementation based on share group state.

Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2.
IPC thread posts to thread B for Context 1.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
IPC thread posts to thread A for Context 2.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/b1f17d6f3f988d7b190cab1147b1e20da0647a44
Cr-Commit-Position: refs/heads/master@{#389947}

Committed: https://crrev.com/90fcc5eaa0a57e02c8be5f97eff8c1f29cad8a76
Cr-Commit-Position: refs/heads/master@{#390591}

[danakj, 2016-04-26 01:19:34]

Description was changed from

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2. Posts to thread B and A
to notify them.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
==========

to

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2. Posts to thread B and A
to notify them.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[danakj, 2016-04-26 01:21:56]

danakj@chromium.org changed reviewers:
+ fsamuel@google.com, sky@chromium.org

[danakj, 2016-04-26 01:21:56]

+sky for mojo
+fsamuel for mus

[danakj, 2016-04-26 01:24:07]

Description was changed from

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2. Posts to thread B and A
to notify them.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2.
IPC thread posts to thread B for Context 1.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
IPC thread posts to thread A for Context 2.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[piman, 2016-04-26 01:58:47]

lgtm

[Fady Samuel, 2016-04-26 02:54:50]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-04-26 02:54:51]

mus lgtm

[sky, 2016-04-26 16:29:37]

LGTM

[danakj, 2016-04-26 19:02:32]

I think the bot failures are me forgetting to init lost_ = false in the
ShareGroup.

[danakj, 2016-04-26 19:03:21]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-04-26 19:03:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, piman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1916303003/#ps20001
(title: "lost-share-group: init")

[commit-bot: I haz the power, 2016-04-26 19:03:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916303003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916303003/20001

[commit-bot: I haz the power, 2016-04-26 19:54:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-26 19:54:02]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2016-04-26 21:18:49]

The cc unittest failure is GLRendererTestSyncPoint.SignalSyncPointOnLostContext:

The test does:

1. LoseContextCHROMIUM()
2. SignalSyncToken()
3. Finish()

Then expects that the sync point callback won't happen since the context was
lost.

However now it does happen.

LoseContextCHROMIUM is in GLES2Implementation. It calls
GLES2CmdHelper::GLES2Implementation.

The cmd helper's implementation is
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...

Which makes a gles2::cmds::LoseContextCHROMIUM for the command buffer to the
service side.

It seems that the sync point gets finished on the client side before the loss
happens in gles2_cmd_decoder.cc. I'm not sure what prevented this before. Or if
this test is really good.

[danakj, 2016-04-26 21:28:05]

I'm feeling kinda skeptical of this test:

- It's in cc_unittests but testing implementation details of
InProcessCommandBuffer.

- It's checking the sync point callback doesn't happen if we Finish, as opposed
to the sync point callback doesn't happen after the lost context callback.

The test was added in https://chromiumcodereview.appspot.com/14126014

[piman, 2016-04-26 21:49:45]

On Tue, Apr 26, 2016 at 2:28 PM, <danakj@chromium.org> wrote:

> I'm feeling kinda skeptical of this test:
>
> - It's in cc_unittests but testing implementation details of
> InProcessCommandBuffer.
>
> - It's checking the sync point callback doesn't happen if we Finish, as
> opposed
> to the sync point callback doesn't happen after the lost context callback.
>
> The test was added in https://chromiumcodereview.appspot.com/14126014
>

That test looks racy. It's hard to make a fully meaningful test... Though I
think if you add a ShallowFlushCHROMIUM between the LoseContextCHROMIUM and
the SignalSyncToken, I think the ordering will make it do what we expect,
though you're right that it's testing an implementation detail.
We could also add a check that the context is not lost in SyncTokenCallback



>
>
> https://codereview.chromium.org/1916303003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-04-26 22:31:30]

OK Ill kill the cc unit test and send another CL for a test in the gpu code.

[danakj, 2016-04-26 22:31:39]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-04-26 22:31:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, piman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1916303003/#ps40001
(title: "lost-share-group: rm-cc-test")

[commit-bot: I haz the power, 2016-04-26 22:32:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916303003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916303003/40001

[commit-bot: I haz the power, 2016-04-27 00:01:01]

Description was changed from

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2.
IPC thread posts to thread B for Context 1.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
IPC thread posts to thread A for Context 2.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2.
IPC thread posts to thread B for Context 1.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
IPC thread posts to thread A for Context 2.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-04-27 00:01:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-27 00:02:17]

Description was changed from

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2.
IPC thread posts to thread B for Context 1.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
IPC thread posts to thread A for Context 2.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2.
IPC thread posts to thread B for Context 1.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
IPC thread posts to thread A for Context 2.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/b1f17d6f3f988d7b190cab1147b1e20da0647a44
Cr-Commit-Position: refs/heads/master@{#389947}
==========

[commit-bot: I haz the power, 2016-04-27 00:02:19]

Patchset 3 (id:??) landed as
https://crrev.com/b1f17d6f3f988d7b190cab1147b1e20da0647a44
Cr-Commit-Position: refs/heads/master@{#389947}

[Matt Giuca, 2016-04-27 03:32:10]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1919203003/ by mgiuca@chromium.org.

The reason for reverting is: Seems to be crashing tests. See two attached bugs:

1. BrowserGpuChannelHostFactoryTest crashes in GPU-related code.
DCHECK(IsContextLost(context_provider->ContextGL())) is failing in the second
stack trace, which leads me to this CL. https://crbug.com/607048.
2. virtual/gpu/fast/canvas/canvas-lost-gpu-context.html LayoutTest is failing
with "Graphics context loss did not destroy canvas contents." The mention of
context loss also leads me to this CL. https://crbug.com/607049.

Speculatively reverting.

BUG=607048,607049.

[danakj, 2016-04-28 01:10:28]

Description was changed from

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2.
IPC thread posts to thread B for Context 1.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
IPC thread posts to thread A for Context 2.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/b1f17d6f3f988d7b190cab1147b1e20da0647a44
Cr-Commit-Position: refs/heads/master@{#389947}
==========

to

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2.
IPC thread posts to thread B for Context 1.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
IPC thread posts to thread A for Context 2.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/b1f17d6f3f988d7b190cab1147b1e20da0647a44
Cr-Commit-Position: refs/heads/master@{#389947}
==========

[danakj, 2016-04-29 01:00:03]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-29 01:00:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916303003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916303003/80001

[danakj, 2016-04-29 01:03:15]

Patchset #5 (id:80001) has been deleted

[danakj, 2016-04-29 01:05:59]

Patchset #5 (id:100001) has been deleted

[danakj, 2016-04-29 01:06:06]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[danakj, 2016-04-29 01:06:10]

Patch set 4 will notify the GLES2Implementation of context loss whenever the
command buffer state becomes an error, which will tell the share group causing
GetGraphicsResetStatusKHR() to return an error, allowing things to see context
losses as the stack unwinds.

What I ended up doing was slightly expanding the "InvalidGpuMessage" and
"InvalidGpuReply" into a set of 4 error functions.

1. OnGpuSyncReplyError() - for replies to sync messages causing errors
2. OnGpuAsyncMessageError(reason, error) - for async error messages from the GPU
process
3. OnGpuStateError() - when we update last_state_ and see an error in it
4. OnClientError(error) - for other cases where things go wrong and we create a
synthetic error as a result

[commit-bot: I haz the power, 2016-04-29 01:06:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916303003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916303003/120001

[danakj, 2016-04-29 01:06:50]

On 2016/04/29 01:06:10, danakj wrote:
> Patch set 4 will notify the GLES2Implementation of context loss whenever the

Patch set 5 I mean of course..

[commit-bot: I haz the power, 2016-04-29 01:21:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-29 01:21:08]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[danakj, 2016-04-29 01:34:22]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-29 01:34:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916303003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916303003/140001

[piman, 2016-04-29 01:35:28]

LGTM+naming nit

https://codereview.chromium.org/1916303003/diff/120001/gpu/ipc/client/command...
File gpu/ipc/client/command_buffer_proxy_impl.h (right):

https://codereview.chromium.org/1916303003/diff/120001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.h:208: void DestroyChannel();
nit: we don't really destroy the channel (it's almost certainly referenced by
something else). Not sure what's a good name. Disconnect?

[danakj, 2016-04-29 02:34:41]

In patch set 8 I used RAII objects to CheckLock() and either verify no error
happened, or call back to GpuControlClient and post task DisconnectChannel() at
the end of the method.

 2 files changed, 115 insertions(+), 96 deletions(-)

Overall it's more code, and less obvious maybe. Let me know what you think.

https://codereview.chromium.org/1916303003/diff/120001/gpu/ipc/client/command...
File gpu/ipc/client/command_buffer_proxy_impl.h (right):

https://codereview.chromium.org/1916303003/diff/120001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.h:208: void DestroyChannel();
On 2016/04/29 01:35:28, piman wrote:
> nit: we don't really destroy the channel (it's almost certainly referenced by
> something else). Not sure what's a good name. Disconnect?

Ah ya, done in Patch set 7.

[danakj, 2016-04-29 02:39:24]

Patchset #8 (id:180001) has been deleted

[piman, 2016-04-29 02:39:38]

Let's go with PS7. Thanks for investigating!

[danakj, 2016-04-29 02:40:06]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-04-29 02:40:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, sky@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1916303003/#ps160001
(title: "lost-share-group: disconnect-channel")

[commit-bot: I haz the power, 2016-04-29 02:40:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916303003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916303003/160001

[commit-bot: I haz the power, 2016-04-29 04:21:10]

Description was changed from

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2.
IPC thread posts to thread B for Context 1.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
IPC thread posts to thread A for Context 2.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/b1f17d6f3f988d7b190cab1147b1e20da0647a44
Cr-Commit-Position: refs/heads/master@{#389947}
==========

to

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2.
IPC thread posts to thread B for Context 1.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
IPC thread posts to thread A for Context 2.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/b1f17d6f3f988d7b190cab1147b1e20da0647a44
Cr-Commit-Position: refs/heads/master@{#389947}
==========

[commit-bot: I haz the power, 2016-04-29 04:21:12]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-04-30 17:24:17]

Patchset 7 (id:??) landed as
https://crrev.com/90fcc5eaa0a57e02c8be5f97eff8c1f29cad8a76
Cr-Commit-Position: refs/heads/master@{#390591}

[commit-bot: I haz the power, 2016-04-30 17:59:21]

Description was changed from

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2.
IPC thread posts to thread B for Context 1.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
IPC thread posts to thread A for Context 2.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/b1f17d6f3f988d7b190cab1147b1e20da0647a44
Cr-Commit-Position: refs/heads/master@{#389947}
==========

to

==========
Report lost context from GLES2Implementation based on share group state.

When any context in a gles2::ShareGroup becomes lost, all contexts in
the same group should report that they are lost immediately. This avoids
races such as the following:

IPC thread hears of the loss of Context 1 and 2.
IPC thread posts to thread B for Context 1.
Context 1 hears it is lost on thread B. Posts to thread A to recreate.
IPC thread posts to thread A for Context 2.
Context 1 is recreated on thread A. Puts itself in same share group as 2.
Context 2 hears it is lost on thread A, makes a new share group.

This can lead to racey cases where share groups get split apart.

Resolve this by storing a |lost_| state on gles2::ShareGroup. When
GLES2Implementation hears of OnGpuControlLostContext, it will set the
lost state on the ShareGroup. Now glGetGraphicsResetStatusKHR() will
return an error if the gles2::ShareGroup has been lost.

We remove the GpuControl::IsChannelLost() method as this is now unused.
The CmdBufferHelper::IsContextLost() is still needed for the
QueryTracker, so insert a comment and a TODO explaining why and how
it could be removed.

R=piman, sievers
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/b1f17d6f3f988d7b190cab1147b1e20da0647a44
Cr-Commit-Position: refs/heads/master@{#389947}

Committed: https://crrev.com/90fcc5eaa0a57e02c8be5f97eff8c1f29cad8a76
Cr-Commit-Position: refs/heads/master@{#390591}
==========