gpu: Raise GL_OUT_OF_MEMORY when BeginQueryEXT fails to allocate.

gpu: Lose context when BeginQueryEXT fails to allocate.

Instead of crashes, raise a GL_OUT_OF_MEMORY error. Since compositor
does not want to deal with these errors and it would leave it in a
bad state, add the ability to lose the context when GL_OUT_OF_MEMORY
occurs.

R=dmichael@chromium.org, jamesr@chromium.org, piman@chromium.org, piman
BUG=351587
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261120

[danakj, 2014-03-13 21:52:43]

MappedMemoryManager has a CommandBufferHelper which is not able to raise errors.
It provides access to CommandBuffer, but SetParseError is not implemented in
CommandBufferProxyImpl.

So I don't think we can raise the error generically when transfer memory
allocation fails?

QuerySyncManager::Alloc does not have access to the GLES2Impl. It is owned by
the GLES2Impl, though, so we could give something to it that lets it raise an
error.

Or, MappedMemoryManager is also owned by the GLES2Impl, so we could give it
something else, or add something to CommandBufferHelper. 

Alternatively, we can raise the error out in GLES2Impl::BeginQueryEXT(). Which
is what I did here.

What would be the best way to go about this? I'm not sure how all these things
are used elsewhere (if at all).

[piman, 2014-03-14 00:45:57]

lgtm

https://codereview.chromium.org/199443004/diff/1/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gles2_implementation.cc (left):

https://codereview.chromium.org/199443004/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:3288: MustBeContextLost();
I think that was the last caller. Can you remove MustBeContextLost altogether?

[danakj, 2014-03-14 01:15:48]

https://codereview.chromium.org/199443004/diff/1/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gles2_implementation.cc (left):

https://codereview.chromium.org/199443004/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:3288: MustBeContextLost();
On 2014/03/14 00:45:57, piman wrote:
> I think that was the last caller. Can you remove MustBeContextLost altogether?

Oh! Done.

[danakj, 2014-03-14 01:15:51]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-14 01:17:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/199443004/20001

[danakj, 2014-03-14 01:20:03]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-03-21 19:25:38]

This patch now does the following:

1. Raise GL_OOM when BeginQuery fails to allocate memory.
2. Pass the WGC3D::Attributes::loseContextWhenOOM attribute to
GLES2Implementation. When an error is set on the client side, it checks for that
attribute and sends LoseContextCHROMIUM to the service side.
3. The attribute is given to the service side, and stored as a bool on
GLES2DecoderImpl. 
4. The ErrorState class now has an ErrorStateClient with OnOutOfMemeoryError()
method. When GL_OOM occurs, it tells the ErrorStateClient.
5. GLES2DecoderImpl is the ErrorStateClient. When GL_OOM occurs, it checks for
the loseContextWhenOOM attribute, and loses itself and all contexts in the group
(same as LoseContextCHROMIUM would do).
6. Add loseContextWhenOOM attribute to all compositor contexts.

[danakj, 2014-03-21 19:28:27]

On Fri, Mar 21, 2014 at 3:25 PM, <danakj@chromium.org> wrote:

> This patch now does the following:
>
> 1. Raise GL_OOM when BeginQuery fails to allocate memory.
> 2. Pass the WGC3D::Attributes::loseContextWhenOOM attribute to
> GLES2Implementation. When an error is set on the client side, it checks
> for that
> attribute and sends LoseContextCHROMIUM to the service side.
> 3. The attribute is given to the service side, and stored as a bool on
> GLES2DecoderImpl.
> 4. The ErrorState class now has an ErrorStateClient with
> OnOutOfMemeoryError()
> method. When GL_OOM occurs, it tells the ErrorStateClient.
> 5. GLES2DecoderImpl is the ErrorStateClient. When GL_OOM occurs, it checks
> for
> the loseContextWhenOOM attribute, and loses itself and all contexts in the
> group
> (same as LoseContextCHROMIUM would do).
> 6. Add loseContextWhenOOM attribute to all compositor contexts.
>

Actually just to the OutputSurface main context, not to the offscreen
contexts, or not consistently. Probably need to improve this and set this
for all offscreen contexts too. I don't think that will interfere with
pepper/nacl?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-03-21 20:31:47]

On 2014/03/21 19:28:27, danakj wrote:
> On Fri, Mar 21, 2014 at 3:25 PM, <mailto:danakj@chromium.org> wrote:
> 
> > This patch now does the following:
> >
> > 1. Raise GL_OOM when BeginQuery fails to allocate memory.
> > 2. Pass the WGC3D::Attributes::loseContextWhenOOM attribute to
> > GLES2Implementation. When an error is set on the client side, it checks
> > for that
> > attribute and sends LoseContextCHROMIUM to the service side.
> > 3. The attribute is given to the service side, and stored as a bool on
> > GLES2DecoderImpl.
> > 4. The ErrorState class now has an ErrorStateClient with
> > OnOutOfMemeoryError()
> > method. When GL_OOM occurs, it tells the ErrorStateClient.
> > 5. GLES2DecoderImpl is the ErrorStateClient. When GL_OOM occurs, it checks
> > for
> > the loseContextWhenOOM attribute, and loses itself and all contexts in the
> > group
> > (same as LoseContextCHROMIUM would do).
> > 6. Add loseContextWhenOOM attribute to all compositor contexts.
> >
> 
> Actually just to the OutputSurface main context, not to the offscreen
> contexts, or not consistently. Probably need to improve this and set this
> for all offscreen contexts too. I don't think that will interfere with
> pepper/nacl?

Added renderer offscreen contexts.

[piman, 2014-03-21 21:34:48]

lgtm

https://codereview.chromium.org/199443004/diff/130001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/199443004/diff/130001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:490:
helper_->LoseContextCHROMIUM(GL_UNKNOWN_CONTEXT_RESET_ARB,
nit: we're the guilty one.

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

https://codereview.chromium.org/199443004/diff/130001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10571:
LoseContext(GL_UNKNOWN_CONTEXT_RESET_ARB);
nit: we're guilty.

[danakj, 2014-03-21 21:36:01]

https://codereview.chromium.org/199443004/diff/130001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/199443004/diff/130001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:490:
helper_->LoseContextCHROMIUM(GL_UNKNOWN_CONTEXT_RESET_ARB,
On 2014/03/21 21:34:48, piman wrote:
> nit: we're the guilty one.

Should I tell the others they're innocent also? I was like "What if another
context is hogging all the memory, so we fail alloc as a result?"

[piman, 2014-03-21 22:10:25]

On Fri, Mar 21, 2014 at 2:36 PM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.org/199443004/diff/130001/gpu/
> command_buffer/client/gles2_implementation.cc
> File gpu/command_buffer/client/gles2_implementation.cc (right):
>
> https://codereview.chromium.org/199443004/diff/130001/gpu/
> command_buffer/client/gles2_implementation.cc#newcode490
> gpu/command_buffer/client/gles2_implementation.cc:490:
> helper_->LoseContextCHROMIUM(GL_UNKNOWN_CONTEXT_RESET_ARB,
> On 2014/03/21 21:34:48, piman wrote:
>
>> nit: we're the guilty one.
>>
>
> Should I tell the others they're innocent also? I was like "What if
> another context is hogging all the memory, so we fail alloc as a
> result?"
>

Good question. I don't think we use that info for anything today, though I
think we were planning to be less lenient with "guilty" contexts wrt
allowing them to start again. I don't know if we would ever use INNOCENT vs
UNKNOWN.
UNKNOWN is probably ok for now.


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

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-03-21 22:53:50]

https://codereview.chromium.org/199443004/diff/130001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/199443004/diff/130001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:490:
helper_->LoseContextCHROMIUM(GL_UNKNOWN_CONTEXT_RESET_ARB,
On 2014/03/21 21:36:02, danakj wrote:
> On 2014/03/21 21:34:48, piman wrote:
> > nit: we're the guilty one.
> 
> Should I tell the others they're innocent also? I was like "What if another
> context is hogging all the memory, so we fail alloc as a result?"

Done.

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

https://codereview.chromium.org/199443004/diff/130001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10571:
LoseContext(GL_UNKNOWN_CONTEXT_RESET_ARB);
On 2014/03/21 21:34:48, piman wrote:
> nit: we're guilty.

Done.

[danakj, 2014-03-21 22:56:03]

+dmichael for ppapi/ : passing false to GLES2Implementation to keep existing
behaviour for pepper contexts.

[dmichael (off chromium), 2014-03-24 17:12:07]

ppapi lgtm

[danakj, 2014-03-24 18:03:58]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-24 18:05:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/199443004/170001

[commit-bot: I haz the power, 2014-03-24 18:28:11]

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

[commit-bot: I haz the power, 2014-03-24 18:28:11]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[danakj, 2014-03-25 22:35:10]

PTAL. Updated the CL.

For in process contexts, we leave the attribute on
gpu::GLInProcessContextAttribs. Added a bool to the WGC3DInProcessCBImpl class
instead of WGC3D::Attributes, that gets added to the GLInProcessContext
attributes.

For out of process contexts, we add a bool to WGC3DCommandBufferImpl constructor
instead of using the WGC3D::Attributes struct. This bool translates into an
attribute that we send to the service side still and is passed to
GLES2Implementation directly.

[danakj, 2014-03-25 22:51:36]

+jamesr for mojo/ OWNERS

[dmichael (off chromium), 2014-03-25 23:38:51]

ppapi still lgtm

[jamesr, 2014-03-26 00:53:05]

lgtm

[piman, 2014-03-26 23:34:31]

lgtm

[danakj, 2014-03-26 23:35:25]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-26 23:37:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/199443004/250001

[commit-bot: I haz the power, 2014-03-26 23:37:45]

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

[commit-bot: I haz the power, 2014-03-26 23:37:46]

Failed to apply patch for content/renderer/render_widget.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/render_widget.cc
  Hunk #1 FAILED at 890.
  Hunk #2 succeeded at 2815 (offset 34 lines).
  Hunk #3 succeeded at 2830 (offset 34 lines).
  Hunk #4 succeeded at 2863 (offset 34 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
content/renderer/render_widget.cc.rej

Patch:       content/renderer/render_widget.cc
Index: content/renderer/render_widget.cc
diff --git a/content/renderer/render_widget.cc
b/content/renderer/render_widget.cc
index
22343690fa852c52261e23b6b746e1d8c017f6bd..aa48cfcde704798174008228e5b188d928a0e582
100644
--- a/content/renderer/render_widget.cc
+++ b/content/renderer/render_widget.cc
@@ -890,19 +890,12 @@ scoped_ptr<cc::OutputSurface>
RenderWidget::CreateOutputSurface(bool fallback) {
   // be optimized to resolve directly into the IOSurface shared between the
   // GPU and browser processes. For these reasons and to avoid platform
   // disparities we explicitly disable antialiasing.
-  blink::WebGraphicsContext3D::Attributes attributes;
-  attributes.antialias = false;
-  attributes.shareResources = true;
-  attributes.noAutomaticFlushes = true;
-  attributes.depth = false;
-  attributes.stencil = false;
 
   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
   scoped_refptr<ContextProviderCommandBuffer> context_provider;
   if (!fallback) {
     context_provider = ContextProviderCommandBuffer::Create(
-        CreateGraphicsContext3D(attributes),
-        "RenderCompositor");
+        CreateGraphicsContext3D(), "RenderCompositor");
   }
 
   uint32 output_surface_id = next_output_surface_id_++;
@@ -2781,8 +2774,7 @@ bool RenderWidget::HasTouchEventHandlersAt(const
gfx::Point& point) const {
 }
 
 scoped_ptr<WebGraphicsContext3DCommandBufferImpl>
-RenderWidget::CreateGraphicsContext3D(
-    const blink::WebGraphicsContext3D::Attributes& attributes) {
+RenderWidget::CreateGraphicsContext3D() {
   if (!webwidget_)
     return scoped_ptr<WebGraphicsContext3DCommandBufferImpl>();
   if (CommandLine::ForCurrentProcess()->HasSwitch(
@@ -2797,6 +2789,14 @@ RenderWidget::CreateGraphicsContext3D(
   if (!gpu_channel_host)
     return scoped_ptr<WebGraphicsContext3DCommandBufferImpl>();
 
+  blink::WebGraphicsContext3D::Attributes attributes;
+  attributes.antialias = false;
+  attributes.shareResources = true;
+  attributes.noAutomaticFlushes = true;
+  attributes.depth = false;
+  attributes.stencil = false;
+  bool bind_generates_resources = false;
+  bool lose_context_when_out_of_memory = true;
   WebGraphicsContext3DCommandBufferImpl::SharedMemoryLimits limits;
 #if defined(OS_ANDROID)
   // If we raster too fast we become upload bound, and pending
@@ -2822,14 +2822,14 @@ RenderWidget::CreateGraphicsContext3D(
 #endif
 
   scoped_ptr<WebGraphicsContext3DCommandBufferImpl> context(
-      new WebGraphicsContext3DCommandBufferImpl(
-          surface_id(),
-          GetURLForGraphicsContext3D(),
-          gpu_channel_host.get(),
-          attributes,
-          false /* bind generates resources */,
-          limits,
-          NULL));
+      new WebGraphicsContext3DCommandBufferImpl(surface_id(),
+                                                GetURLForGraphicsContext3D(),
+                                                gpu_channel_host.get(),
+                                                attributes,
+                                                bind_generates_resources,
+                                               
lose_context_when_out_of_memory,
+                                                limits,
+                                                NULL));
   return context.Pass();
 }

[danakj, 2014-03-26 23:43:46]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-26 23:45:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/199443004/310001

[commit-bot: I haz the power, 2014-03-27 00:18:36]

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

[commit-bot: I haz the power, 2014-03-27 00:18:36]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[danakj, 2014-03-27 00:23:36]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-27 00:33:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/199443004/330001

[commit-bot: I haz the power, 2014-03-27 01:17:00]

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

[commit-bot: I haz the power, 2014-03-27 01:17:01]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[danakj, 2014-03-31 14:37:52]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-31 14:38:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/199443004/350001

[commit-bot: I haz the power, 2014-03-31 15:01:10]

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

[commit-bot: I haz the power, 2014-03-31 15:01:10]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[danakj, 2014-03-31 23:17:59]

I've added a test for the GLES2Decoder, need to figure out one for the
GLES2Implmentation still.

[danakj, 2014-04-01 20:50:45]

I've added GLES2Implementation unittests now too.

piman@ can you please have a look at the new tests
(gpu/command_buffer/client/gles2_implementation_unittest.cc and
gpu/command_buffer/service/gles2_cmd_decoder_unittest{,_base}.{cc,h} and make
sure they look right, I hope they do.

Also the unit test refactoring the GLES2CmdDecoder unittests since adding
YetAnotherBool to the function-called-everywhere was not very palatable.

Thanks!

[danakj, 2014-04-01 20:52:45]

(
I also updated the android callsites:

content/browser/renderer_host/compositor_impl_android.cc
content/browser/renderer_host/image_transport_factory_android.cc
)

[danakj, 2014-04-01 20:56:19]

https://codereview.chromium.org/199443004/diff/480001/mojo/gles2/gles2_contex...
File mojo/gles2/gles2_context.cc (right):

https://codereview.chromium.org/199443004/diff/480001/mojo/gles2/gles2_contex...
mojo/gles2/gles2_context.cc:44: bool lose_context_when_out_of_memory = false;
Oh, and one more, over here I set this false, as I don't see a really reasonable
way to plumb this in here atm. Idk if this would be used for the compositor
output surface, in which case it should be true, but it surely is used for other
things as well, where it should be false. So yeh.

[piman, 2014-04-01 22:13:03]

lgtm

https://codereview.chromium.org/199443004/diff/480001/mojo/gles2/gles2_contex...
File mojo/gles2/gles2_context.cc (right):

https://codereview.chromium.org/199443004/diff/480001/mojo/gles2/gles2_contex...
mojo/gles2/gles2_context.cc:44: bool lose_context_when_out_of_memory = false;
On 2014/04/01 20:56:20, danakj wrote:
> Oh, and one more, over here I set this false, as I don't see a really
reasonable
> way to plumb this in here atm. Idk if this would be used for the compositor
> output surface, in which case it should be true, but it surely is used for
other
> things as well, where it should be false. So yeh.

Yeah, right now, this is common code. We'll have to plumb this sort of things
from the client API.
Maybe add a TODO(piman) there.

[danakj, 2014-04-01 22:16:13]

https://codereview.chromium.org/199443004/diff/480001/mojo/gles2/gles2_contex...
File mojo/gles2/gles2_context.cc (right):

https://codereview.chromium.org/199443004/diff/480001/mojo/gles2/gles2_contex...
mojo/gles2/gles2_context.cc:44: bool lose_context_when_out_of_memory = false;
On 2014/04/01 22:13:04, piman wrote:
> On 2014/04/01 20:56:20, danakj wrote:
> > Oh, and one more, over here I set this false, as I don't see a really
> reasonable
> > way to plumb this in here atm. Idk if this would be used for the compositor
> > output surface, in which case it should be true, but it surely is used for
> other
> > things as well, where it should be false. So yeh.
> 
> Yeah, right now, this is common code. We'll have to plumb this sort of things
> from the client API.
> Maybe add a TODO(piman) there.

Thanks! Done.

[danakj, 2014-04-01 22:16:24]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-04-01 22:31:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/199443004/410002

[commit-bot: I haz the power, 2014-04-02 02:02:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/199443004/410002

[commit-bot: I haz the power, 2014-04-02 04:16:33]

Change committed as 261064

[pfeldman, 2014-04-02 04:54:36]

A revert of this CL has been created in
https://codereview.chromium.org/221783002/ by pfeldman@chromium.org.

The reason for reverting is: Build failure in
content/common/gpu/client/gl_helper_benchmark.cc. 

http://build.chromium.org/p/chromium/builders/Linux%20x64/builds/63241/steps/....

[danakj, 2014-04-02 14:43:22]

Committed patchset #24 manually as r261120 (presubmit successful).