gpu: Fix some context lost marking glitches+leaks and add UMA stats

gpu: Fix some context lost marking glitches+leaks and add UMA stats

* This fixes some glitches in the decoder where we were not ending up
with the correct 'context lost' reason, such as calling things in
the wrong order or calling glGetGraphicsResetStatus() while we might
not have the correct context current.

* Add context lost reasons for when the context group is forcibly lost
when GL_OUT_OF_MEMORY is detected and when MakeCurrent fails.

* Also communicate parse errors to the client.

* Record UMA histograms for each context type
(for example 'browser compositor') with the context lost reason.

* Always lose (client-side) share-group if we force-lose
a context so we don't leak resources.

* Fix a bug where we weren't deleting GL resources if the context was
lost although we might still be able to make it current.

Add a bunch of tests.

BUG=475676
Committed: https://crrev.com/fbaa5dc2fcb2e482c1daec1b9bcf1d577d53ca30
Cr-Commit-Position: refs/heads/master@{#327197}

[no sievers, 2015-04-18 00:31:22]

sievers@chromium.org changed reviewers:
+ kbr@chromium.org, piman@chromium.org

[no sievers, 2015-04-18 00:34:34]

ptal.

It would be nice if we could also query the reset status for the other contexts
in the group once we detect a reset.

I assume you can still make them current and then query the reset status. Maybe
we should do that. But I don't want to mess
with the 'current context' logic too much (at least not in this patch), esp. if
we mark the contexts as lost from the stub.

[piman, 2015-04-18 01:03:18]

I'm super supportive of this. 2 things though.

https://codereview.chromium.org/1095893002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1095893002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3345: if (CheckResetStatus()) {
Do we really want to check this on every MakeCurrent?

https://codereview.chromium.org/1095893002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10315:
MarkContextLost(error::kUnknown);
Here, and other places we call MarkContextLost, we should actually call
group_->LoseContexts() too. This is a requirement from the spec, and actually
otherwise may cause some leaks (context1 is lost, we can't destroy resources on
it. If unrelated-but-shared context2 is not lost, then we will not tear down the
context group and never release resources of context2.).

I think this has been missing for a while and causing some problems.

[Ken Russell (switch to Gerrit), 2015-04-24 02:39:41]

Thanks for working to clean up this code. Your new tests are great.

We should run some of the manual stress tests that cause GPU resets from
https://www.khronos.org/registry/webgl/sdk/tests/extra/ on various platforms
(Windows with ANGLE, Linux, and Windows with --use-gl=desktop and recent
drivers), to make sure that if the driver actually detects the reset that it's
properly reported to the client. Let me know if I can help with this testing.

https://codereview.chromium.org/1095893002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_metrics.h (right):

https://codereview.chromium.org/1095893002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_metrics.h:15: // Don't add new values
before _MAX_ENUM to keep the histogram consistent.
Is this comment correct? Should it read "before" or "after"?

https://codereview.chromium.org/1095893002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_metrics.h:26: // Add new values above
this point.
Above this point or above CONTEXT_TYPE_UNKNOWN?

https://codereview.chromium.org/1095893002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1095893002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3345: if (CheckResetStatus()) {
On 2015/04/18 01:03:18, piman (Very slow to review) wrote:
> Do we really want to check this on every MakeCurrent?

Actually, I think it was already being done in WasContextLost(). It's supposed
to be inexpensive.

https://codereview.chromium.org/1095893002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_lost.cc
(right):

https://codereview.chromium.org/1095893002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_lost.cc:141:
.WillOnce(Return(GL_CONTEXT_LOST_KHR))
As you discovered and pointed out offline, the CONTEXT_LOST_KHR error was only
introduced with GL_KHR_robustness and doesn't exist in the GL_ARB_robustness
extension. We should probably make the command decoder or client manually return
CONTEXT_LOST from GetError() once the context's been discovered to be lost.

[no sievers, 2015-04-24 18:52:47]

https://codereview.chromium.org/1095893002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_metrics.h (right):

https://codereview.chromium.org/1095893002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_metrics.h:26: // Add new values above
this point.
On 2015/04/24 02:39:41, Ken Russell wrote:
> Above this point or above CONTEXT_TYPE_UNKNOWN?

Done.

https://codereview.chromium.org/1095893002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1095893002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3345: if (CheckResetStatus()) {
On 2015/04/24 02:39:41, Ken Russell wrote:
> On 2015/04/18 01:03:18, piman (Very slow to review) wrote:
> > Do we really want to check this on every MakeCurrent?
> 
> Actually, I think it was already being done in WasContextLost(). It's supposed
> to be inexpensive.

We might also want to check it proactively here for EXT_robustness, because
unlike with KHR_robustness we wouldn't get the 'context lost' GL error and never
reach OnContextLostError().

https://codereview.chromium.org/1095893002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_lost.cc
(right):

https://codereview.chromium.org/1095893002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_lost.cc:141:
.WillOnce(Return(GL_CONTEXT_LOST_KHR))
On 2015/04/24 02:39:41, Ken Russell wrote:
> As you discovered and pointed out offline, the CONTEXT_LOST_KHR error was only
> introduced with GL_KHR_robustness and doesn't exist in the GL_ARB_robustness
> extension. We should probably make the command decoder or client manually
return
> CONTEXT_LOST from GetError() once the context's been discovered to be lost.

Good point, the test looks a bit wrong that way. I've changed this class of
tests to init with gles2 and KHR_robustness instead.

We don't expose this error in the command buffer (see error_state.cc) though.
The way the client would see it is through the shared state and context-lost
callback.

[no sievers, 2015-04-24 19:04:16]

sievers@chromium.org changed reviewers:
+ asvitkine@chromium.org, nasko@chromium.org

[no sievers, 2015-04-24 19:04:42]

+asvitkine@chromium.org for histograms.xml

+nasko@chromium.org for gpu_messages.h

[Alexei Svitkine (slow), 2015-04-24 19:13:21]

https://codereview.chromium.org/1095893002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1095893002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:11440: +  <summary>The reason a GPU
command buffer context was lost.</summary>
Please update these descriptions to differentiate them from each other.

https://codereview.chromium.org/1095893002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:11534: +<histogram
name="GPU.WebGLContextLost" enum="ContextLostReason">
Have you considered formulating these histograms in a way that lets you use
histogram_suffixes feature in this file to avoid extra repetition in this file?

for example, the following naming convention would allow this:

GPU.ContextLost.VideoCapture
GPU.ContextLost.WebGL

etc.

In addition to a more concise definition in the XML file, you'll also get better
grouping on the dashboard.

[no sievers, 2015-04-24 19:26:25]

https://codereview.chromium.org/1095893002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1095893002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:11440: +  <summary>The reason a GPU
command buffer context was lost.</summary>
On 2015/04/24 19:13:21, Alexei Svitkine wrote:
> Please update these descriptions to differentiate them from each other.

Done.

https://codereview.chromium.org/1095893002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:11534: +<histogram
name="GPU.WebGLContextLost" enum="ContextLostReason">
On 2015/04/24 19:13:21, Alexei Svitkine wrote:
> Have you considered formulating these histograms in a way that lets you use
> histogram_suffixes feature in this file to avoid extra repetition in this
file?
> 
> for example, the following naming convention would allow this:
> 
> GPU.ContextLost.VideoCapture
> GPU.ContextLost.WebGL
> 
> etc.
> 
> In addition to a more concise definition in the XML file, you'll also get
better
> grouping on the dashboard.

Done. Thanks, that is much better.

[Alexei Svitkine (slow), 2015-04-24 19:31:58]

https://codereview.chromium.org/1095893002/diff/100001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_metrics.cc (right):

https://codereview.chromium.org/1095893002/diff/100001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_metrics.cc:124:
CONTEXT_LOST_REASON_MAX_ENUM);
Unfortunately this won't work.

The UMA histogram macro caches the histogram object after the first call. So if
this is hit twice with different histogram_names, the second time will log under
the wrong histogram.

You need to either have a separate UMA_HISTOGRAM_ENUMERATION macro for each
string literal - OR to unwind the macro and expand to the underlying API calls
directly without any caching.

Same thing below.

https://codereview.chromium.org/1095893002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1095893002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:11441: +<histogram
name="GPU.ContextLost.BrowserCompositor" enum="ContextLostReason">
Now that you're using this syntax, you can make this more concise, using
<histogram_suffixes> construct. See the top of the file for some documentation
and some examples within the code.

e.g. you only need on top-level <histogram entry for "GPU.ContextLost" and a
<histogram_suffixes> element that lists all the possible suffixes.

[no sievers, 2015-04-24 19:56:43]

https://codereview.chromium.org/1095893002/diff/100001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_metrics.cc (right):

https://codereview.chromium.org/1095893002/diff/100001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_metrics.cc:124:
CONTEXT_LOST_REASON_MAX_ENUM);
On 2015/04/24 19:31:58, Alexei Svitkine wrote:
> Unfortunately this won't work.
> 
> The UMA histogram macro caches the histogram object after the first call. So
if
> this is hit twice with different histogram_names, the second time will log
under
> the wrong histogram.
> 
> You need to either have a separate UMA_HISTOGRAM_ENUMERATION macro for each
> string literal - OR to unwind the macro and expand to the underlying API calls
> directly without any caching.
> 
> Same thing below.

Thanks for catching that. Fixed.

https://codereview.chromium.org/1095893002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1095893002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:11441: +<histogram
name="GPU.ContextLost.BrowserCompositor" enum="ContextLostReason">
On 2015/04/24 19:31:58, Alexei Svitkine wrote:
> Now that you're using this syntax, you can make this more concise, using
> <histogram_suffixes> construct. See the top of the file for some documentation
> and some examples within the code.
> 
> e.g. you only need on top-level <histogram entry for "GPU.ContextLost" and a
> <histogram_suffixes> element that lists all the possible suffixes.

Like this?

[Alexei Svitkine (slow), 2015-04-24 20:01:32]

https://codereview.chromium.org/1095893002/diff/120001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_metrics.cc (right):

https://codereview.chromium.org/1095893002/diff/120001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_metrics.cc:150: switch (type) {
To avoid having the switches in two places, you can make a helper function with
a single switch that takes CommandBufferContextLostReason and |type| as params.

https://codereview.chromium.org/1095893002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1095893002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:64712: +<histogram_suffixes
name="ContextType">
I think you need to add separator="." here.

[no sievers, 2015-04-24 20:12:50]

https://codereview.chromium.org/1095893002/diff/120001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_metrics.cc (right):

https://codereview.chromium.org/1095893002/diff/120001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_metrics.cc:150: switch (type) {
On 2015/04/24 20:01:31, Alexei Svitkine wrote:
> To avoid having the switches in two places, you can make a helper function
with
> a single switch that takes CommandBufferContextLostReason and |type| as
params.

Done.

https://codereview.chromium.org/1095893002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1095893002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:64712: +<histogram_suffixes
name="ContextType">
On 2015/04/24 20:01:31, Alexei Svitkine wrote:
> I think you need to add separator="." here.

Done. It seems to default to "." though, because I was looking at
about:histograms where it looked as expected.

[Alexei Svitkine (slow), 2015-04-24 20:18:10]

lgtm

https://codereview.chromium.org/1095893002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1095893002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:64712: +<histogram_suffixes
name="ContextType">
On 2015/04/24 20:12:50, sievers wrote:
> On 2015/04/24 20:01:31, Alexei Svitkine wrote:
> > I think you need to add separator="." here.
> 
> Done. It seems to default to "." though, because I was looking at
> about:histograms where it looked as expected.

about:histograms shows what the code logs, not what histograms.xml contains.

[no sievers, 2015-04-24 21:00:26]

https://codereview.chromium.org/1095893002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1095893002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3335: if (WasContextLost()) {
There is actually a bug here (even before my patch I think):
GpuCommandBufferStub::Destroy() does this:

  have_context = decoder_->MakeCurrent();
  if (decoder_) {
    decoder_->Destroy(have_context);

So we will leak resources if we make MakeCurrent() fail here unnecessarily.

I could fix it in GpuCommandBufferStub by just calling
context_->MakeCurrent(surface_) instead.

[nasko, 2015-04-27 14:07:33]

IPC LGTM

[no sievers, 2015-04-27 21:53:33]

I think this is good to go.

There are more bugs I noticed - and in fact recovery from a real reset on Linux
does not work even before my change. But this patch fixes some of the issues
involved, though I might not have figured out all of them yet. It works with
'workarounds().exit_on_context_lost'. I'll keep investigating, but don't want to
pile too many changes into this patch.

[Ken Russell (switch to Gerrit), 2015-04-27 22:35:34]

LGTM overall. Thanks again for looking at and improving this code. Couple of
comments and questions.

https://codereview.chromium.org/1095893002/diff/180001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1095893002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:426: // don't leak resources.
Presumably if the driver has actually lost the context then the resources have
all been invalidated / reclaimed. This change seems OK though.

https://codereview.chromium.org/1095893002/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1095893002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3350: return false;
Per piman's earlier comment, were you going to broadcast this to other contexts
via group_->LoseContexts()?

https://codereview.chromium.org/1095893002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10383:
group_->LoseContexts(error::kUnknown);
Per our offline conversation, one comment about broadcasting LoseContext
notifications.

I'm not sure how likely it is that guilty context notifications will be reported
promptly. Even if the driver properly assigns GPU resets to the right context,
if enough buffering of commands happens in the driver, the guilty notification
might not be discovered until the next time the guilty context is made current.
In the interim, "innocent" or "unknown" notifications may be delivered to other
contexts. Since we lose all other contexts in the share group immediately with
unknown status as soon as we get any reset notification, we may squelch guilty
notifications.

I don't think this is a big deal, at least right now. WebGL's DoS defenses
aren't predicated on receiving guilty notifications. It's something to keep in
mind though.

https://codereview.chromium.org/1095893002/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_lost.cc
(right):

https://codereview.chromium.org/1095893002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_lost.cc:173:
ClearCurrentDecoderError();
Will this happen in the normal operation of the code, so that decoders will
clean themselves up properly?

[piman, 2015-04-27 23:17:40]

lgtm

[no sievers, 2015-04-27 23:24:42]

https://codereview.chromium.org/1095893002/diff/180001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1095893002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:426: // don't leak resources.
On 2015/04/27 22:35:33, Ken Russell wrote:
> Presumably if the driver has actually lost the context then the resources have
> all been invalidated / reclaimed. This change seems OK though.

But it's a problem if we forced the context to be lost for GL_OUT_OF_MEMORY or
something else.

https://codereview.chromium.org/1095893002/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1095893002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3350: return false;
On 2015/04/27 22:35:33, Ken Russell wrote:
> Per piman's earlier comment, were you going to broadcast this to other
contexts
> via group_->LoseContexts()?

Good catch. Missed this one since it didn't call MarkContextLost() directly.

https://codereview.chromium.org/1095893002/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_lost.cc
(right):

https://codereview.chromium.org/1095893002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_context_lost.cc:173:
ClearCurrentDecoderError();
On 2015/04/27 22:35:33, Ken Russell wrote:
> Will this happen in the normal operation of the code, so that decoders will
> clean themselves up properly?

FWIU there are multiple ways we detect a decoder error.
Generally the decoder only has a concept of the |current_decoder_error_| and
will only return it once from DoCommands() and then reset its error internally
(so you can actually keep reusing it), which is what happens here. (Really I had
to do this because there are some expectations in TearDown() that want kNoError
to be returned.)

However, in production we would not process more commands through the decoder.
The error is either seen by GpuScheduler (which looks for both parse errors to
be returned from ProcessCommands but also checks for other context-lost reasons
afterwards) or GpuCommandBufferStub (which then manually sets a parse error),
which should then always cause the parse error callback and the 'context lost'
callback in the client. Once the CommandBuffer is lost, that's permanent.

[no sievers, 2015-04-27 23:41:15]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2015-04-27 23:41:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, nasko@chromium.org, kbr@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1095893002/#ps220001
(title: "kbr's comment")

[commit-bot: I haz the power, 2015-04-27 23:42:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1095893002/220001

[commit-bot: I haz the power, 2015-04-28 00:45:59]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-04-28 00:46:52]

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