WebGL: Restore atomically both WebGraphicsContext3D and DrawingBuffer together.

WebGL: Restore atomically both WebGraphicsContext3D and DrawingBuffer together.

Currently, WebGLRenderingContextBase::maybeRestoreContext() has a potential crash
bug. When WebGraphicsContext3D is created but fbo of DrawingBuffer is not
created, WebGLRenderingContextBase considers it as success, although m_context
is not updated. It means that DrawingBuffer holds a dangling pointer.

This CL changes the success of restoration that completes both
WebGraphicsContext3D creation and fbo creation of DrawingBuffer.

BUG=344393
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170745

[dshwang, 2014-03-10 18:03:26]

Before re-land http://crrev.com/163773007, I want to clarify restoration
process.
http://crrev.com/163773007 slightly changed restoration process and caused
crash. This CL is extracted from the previous CL.

[dshwang, 2014-03-10 18:23:30]

https://codereview.chromium.org/189423003/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (left):

https://codereview.chromium.org/189423003/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5357: return;
Currently, when failing to create fbo, return here. So m_drawingBuffer outlive
with dangling pointer context.get().

[dshwang, 2014-03-11 08:57:06]

The previous patch changed WebGLRenderingContextBase::loseContextImpl() to
destroy WebGraphicsContext3D and fbo even when RealLostContext mode.
That caused crash: [FATAL:webgraphicscontext3d_command_buffer_impl.cc(1375)]
Check failed: host_.get(). 
It's because the callback deletes WebGraphicsContext3D object during
WebGraphicsContext3DCommandBufferImpl::OnGpuChannelLost().
So this patch set rollbacks the wrong change.

[dshwang, 2014-03-12 09:43:25]

could you review?

[Ken Russell (switch to Gerrit), 2014-03-14 01:49:18]

https://codereview.chromium.org/189423003/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/189423003/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5368:
m_drawingBuffer->bind();
If the code path "if (drawingBuffer->isZeroSized())" is taken above, then
m_drawingBuffer will be a NULL pointer here and this will still crash. Correct?

[Ken Russell (switch to Gerrit), 2014-03-14 02:56:46]

Thanks for this patch and sorry about my earlier hasty review.

Do you have a test which exercises this code path? If it's a WebGL conformance
test, could you please submit a pull request against
https://github.com/KhronosGroup/WebGL ?

I'm concerned that we're submitting a lot of patches against this and other code
which are intended to fix bugs, but there are no tests ensuring the bugs have
actually fixed. This means that regressions are likely to happen in the future.

Thanks.

https://codereview.chromium.org/189423003/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/189423003/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5368:
m_drawingBuffer->bind();
On 2014/03/14 01:49:19, Ken Russell wrote:
> If the code path "if (drawingBuffer->isZeroSized())" is taken above, then
> m_drawingBuffer will be a NULL pointer here and this will still crash.
Correct?

Never mind, I'm wrong.

[dshwang, 2014-03-14 07:48:33]

Thank you for review.

> I'm concerned that we're submitting a lot of patches against this and other
code
> which are intended to fix bugs, but there are no tests ensuring the bugs have
> actually fixed. This means that regressions are likely to happen in the
future.

I agree. I feel like that also. but my recent CLs were hard to be covered by
tests; i.e. this CL and mailbox contention. I'll give more effort for test.

https://codereview.chromium.org/189423003/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (left):

https://codereview.chromium.org/189423003/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5360: return;
Unfortunately, this change is very hard to be covered by test, because
1. after fail here, WebGLRenderingContextBase will never use m_drawingBuffer
again.
2. making m_drawingBuffer->isZeroSized() fail is hard. we seem to need internal
test api to change webgl memory budget.

It's kind of logical bug fix not based on real case.

Currently, when drawingBuffer fails to alloc fbo, return here, without
_m_contextLost = false;_ in 5367 line.
It's very weird state. WebGLRenderingContextBase will never use m_drawingBuffer
because isLostContext() is true, but does not emit GL_INVALID_OPERATION
exception or try to restore again.

This CL's goal is to extract logic change from the refactory CL to re-land it;
https://codereview.chromium.org/163773007/

[Ken Russell (switch to Gerrit), 2014-03-15 00:07:26]

I understand.

Let's endeavor to write more tests, even if they require manual failure
injection, and helper methods added to the classes to be tested. See for example
Source/web/tests/DrawingBufferTest.cpp. jamesr@ pushed me to write a test for a
recent bug fix to DrawingBuffer. His insistence was greatly appreciated, and
improved the quality of the code.

LGTM as I can see from code inspection that this is an improvement, but please
still consider whether you can write a test for it.

[dshwang, 2014-03-15 20:07:13]

On 2014/03/15 00:07:26, Ken Russell wrote:
> I understand.
> 
> Let's endeavor to write more tests, even if they require manual failure
> injection, and helper methods added to the classes to be tested. See for
example
> Source/web/tests/DrawingBufferTest.cpp. jamesr@ pushed me to write a test for
a
> recent bug fix to DrawingBuffer. His insistence was greatly appreciated, and
> improved the quality of the code.
> 
> LGTM as I can see from code inspection that this is an improvement, but please
> still consider whether you can write a test for it.

Thank you for lgtm.
I guess unittests can be possible not like layout tests. Let me investigate.

[Ken Russell (switch to Gerrit), 2014-03-27 18:59:57]

Ping. Do you plan to land this CL? It would be better to get it in rather than
hold it up, I think.

[dshwang, 2014-03-27 19:11:35]

On 2014/03/27 18:59:57, Ken Russell wrote:
> Ping. Do you plan to land this CL? It would be better to get it in rather than
> hold it up, I think.

Hi, I'm investigating how to make unit tests to land this CL.

To do that, I moved DrawingBufferTest.cpp to platform/;
https://codereview.chromium.org/203513002/
In addition, I need to do some more clean-up before creating
WebGLRenderingContextBastTest.cpp
The clean-up is to move DrawingBufferTest and MockWebGraphicsContext3D from
legacy platform_web_unittest_files to platform_test_files because I don't want
to add a legacy unit test.

         # NOTE: these are legacy unit tests, do not add more!
 914     'platform_web_unittest_files': [
 915       'animation/TimingFunctionTest.cpp',
 916       'graphics/Canvas2DLayerBridgeTest.cpp',
 917       'graphics/Canvas2DLayerManagerTest.cpp',
 916       'graphics/DeferredImageDecoderTest.cpp',
 917       'graphics/ImageDecodingStoreTest.cpp',
 918       'graphics/ImageFrameGeneratorTest.cpp',
 921       'graphics/gpu/DrawingBufferTest.cpp',
 919       'graphics/test/MockImageDecoder.h',	
 923       'graphics/test/MockWebGraphicsContext3D.h',

[dshwang, 2014-04-02 11:52:48]

On 2014/03/27 18:59:57, Ken Russell wrote:
> Ping. Do you plan to land this CL? It would be better to get it in rather than
> hold it up, I think.

Hi, could I land this CL without unit tests?

I tried to make unit tests but it seems to be impossible.
The main reason is that WebGLRenderingContextBase requires HTMLCanvasElement
object and Document object. Other unittests have not ever create those kind of
objects.
WebGLRenderingContextBase::WebGLRenderingContextBase(HTMLCanvasElement*
passedCanvas, PassOwnPtr<blink::WebGraphicsContext3D> context,
WebGLContextAttributes* requestedAttributes)
    : CanvasRenderingContext(passedCanvas)
    , ActiveDOMObject(&passedCanvas->document())

[Ken Russell (switch to Gerrit), 2014-04-02 21:05:17]

Yes, let's land this even without unit tests. From code inspection it's clear
that this is an improvement.

LGTM

I noticed that your linux_gpu, win_gpu and mac_gpu try jobs never started. How
did you initiate them? From the UI here in Rietveld, from the command line (git
cl try), etc.?

[dshwang, 2014-04-03 05:40:59]

On 2014/04/02 21:05:17, Ken Russell wrote:
> Yes, let's land this even without unit tests. From code inspection it's clear
> that this is an improvement.
> 
> LGTM
> 
> I noticed that your linux_gpu, win_gpu and mac_gpu try jobs never started. How
> did you initiate them? From the UI here in Rietveld, from the command line
(git
> cl try), etc.?

Thank you for lgtm. I delay to reply due to timezone.
I used 'git cl try -b linux_gpu ...'

[dshwang, 2014-04-03 05:43:34]

On 2014/04/03 05:40:59, dshwang wrote:
> On 2014/04/02 21:05:17, Ken Russell wrote:
> > Yes, let's land this even without unit tests. From code inspection it's
clear
> > that this is an improvement.
> > 
> > LGTM
> > 
> > I noticed that your linux_gpu, win_gpu and mac_gpu try jobs never started.
How
> > did you initiate them? From the UI here in Rietveld, from the command line
> (git
> > cl try), etc.?
> 
> Thank you for lgtm. I delay to reply due to timezone.
> I used 'git cl try -b linux_gpu ...'

Trybot status is interesting. There is green light linux_gpu, win_gpu and
mac_gpu try jobs also.
I did run only once. 'git cl try -m chromium.trybot(?) -b linux_gpu -b win_gpu
-b mac_gpu'

[dshwang, 2014-04-03 05:43:46]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-04-03 05:43:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/189423003/12...

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

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

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

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[dshwang, 2014-04-03 06:08:05]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-04-03 06:08:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/189423003/12...

[commit-bot: I haz the power, 2014-04-03 07:18:58]

Change committed as 170745