WebGL: Transfer ownership of WebGraphicsContext3D from WebGLRenderingContext to DrawingBuffer.

WebGL: Transfer ownership of WebGraphicsContext3D from WebGLRenderingContext to DrawingBuffer.

Currently, the lifecycle of DrawingBuffer is different from it of
WebGraphicsContext3D, so there is so many checks if the context is valid.
This complexity is not necessary because DrawingBuffer does not own any
objects when the context is lost or impossible to be created.

To be clear the lifecycle, this CL makes DrawingBuffer take ownership of WebGraphicsContext3D.

The DrawingBuffer creation or resize can fail if the memory is not enough. This
logic is preserved.
In detail,
1. If it occurs during WebGLRenderingContext creation, we give up creating
WebGLRenderingContext.
2. If it occurs during restoration of lost context, we will try again.
3. If it occurs during resizing DrawingBuffer, we slightly ignore, because we
can reuse different size FBO in this case.

In addition, this CL fixes two bugs.
1. maybeRestoreContext() pretends to success to restore although creating FBO in
DrawingBuffer fails. As mentioned earlier, we will try again.
2. WebGLRenderingContext calls DrawingBuffer::reset() twice during creation,
because the constructor of DrawingBuffer internally calls reset().

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

[dshwang, 2014-02-20 14:49:08]

I change this CL's goal to make more self-sufficient CL.
This CL is preparation of http://crrev.com/169933002

[dshwang, 2014-02-21 07:07:59]

Could you have a chance to review?

[dshwang, 2014-02-24 15:07:18]

On 2014/02/21 07:07:59, dshwang wrote:
> Could you have a chance to review?

[Justin Novosad, 2014-02-24 15:25:56]

https://codereview.chromium.org/163773007/diff/130001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContext.h (right):

https://codereview.chromium.org/163773007/diff/130001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContext.h:392:
blink::WebGraphicsContext3D* m_context;
Having m_context is fragile because future code changes need to remember to keep
it in sync with m_drawingBuffer.  This adds a risk of introducing security
vulnerabilities in the future (memory use after free). We could get rid of
m_context and replace all uses of m_context with m_drawingBuffer->context().

[dshwang, 2014-02-25 20:05:31]

On 2014/02/24 15:25:56, junov wrote:
>
https://codereview.chromium.org/163773007/diff/130001/Source/core/html/canvas...
> File Source/core/html/canvas/WebGLRenderingContext.h (right):
> 
>
https://codereview.chromium.org/163773007/diff/130001/Source/core/html/canvas...
> Source/core/html/canvas/WebGLRenderingContext.h:392:
> blink::WebGraphicsContext3D* m_context;
> Having m_context is fragile because future code changes need to remember to
keep
> it in sync with m_drawingBuffer.  This adds a risk of introducing security
> vulnerabilities in the future (memory use after free). We could get rid of
> m_context and replace all uses of m_context with m_drawingBuffer->context().

Thank you for review. I replaced all uses of m_context with
m_drawingBuffer->context().
Could you review again?

[Justin Novosad, 2014-02-25 21:58:17]

Looks right to me, but I'd rather defer approval to the WebGL experts.

[dshwang, 2014-02-26 15:23:39]

On 2014/02/25 21:58:17, junov wrote:
> Looks right to me, but I'd rather defer approval to the WebGL experts.

junov@, thank you for review.

kbr@, could you have a chance to review?

[dshwang, 2014-02-26 16:53:27]

all layout tests are passed on debug build.

webgl conformance test results are the same on debug build between current and
this CL. conformance/glsl/bug fails on my machine (intel gpu), regardless of
this CL.
https://www.khronos.org/registry/webgl/sdk/tests/webgl-conformance-tests.html

[Ken Russell (switch to Gerrit), 2014-02-27 22:49:09]

I'm sorry for taking so long to review this.

Overall it seems good. A couple of cleanup requests, and one question about a
test which was deleted.

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

https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:501:
ASSERT(context.get());
This can just be ASSERT(context). PassOwnPtr has an implicit conversion to bool.

https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:508:
context.get()->getIntegerv(GL_MAX_VIEWPORT_DIMS, m_maxViewportDims);
This can just be context->getIntegerv(...)

https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:556:
m_drawingBuffer->context()->getIntegerv(GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS,
&numCombinedTextureImageUnits);
m_drawingBuffer->context() is a boilerplate pattern throughout your code. Could
you add a private/protected helper method like "webContext()" or similar which
returns that value? It could be added inline in the header.

https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:748: m_colorMask[1] ?
m_clearColor[1] : 0,
The indentation looks strange here.

https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:4165:
blink::WebGraphicsContext3D* WebGLRenderingContextBase::webGraphicsContext3D()
const
Since this helper is here, please use it throughout the code. You could consider
renaming it to a shorter name like "webContext".

https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5349: m_drawingBuffer =
DrawingBuffer::create(context.release(), clampedCanvasSize(), preserve,
contextEvictionManager.release());
Are the offscreen context's resources fully released if DrawingBuffer::create
returns NULL?

https://codereview.chromium.org/163773007/diff/220001/Source/web/tests/Drawin...
File Source/web/tests/DrawingBufferTest.cpp (left):

https://codereview.chromium.org/163773007/diff/220001/Source/web/tests/Drawin...
Source/web/tests/DrawingBufferTest.cpp:144: TEST_F(DrawingBufferTest,
verifyNoNewBuffersAfterContextLostWithMailboxes)
Why was this test removed? Is this just a mistake in rebasing?

[dshwang, 2014-02-28 13:10:18]

On 2014/02/27 22:49:09, Ken Russell wrote:
> I'm sorry for taking so long to review this.
> 
> Overall it seems good. A couple of cleanup requests, and one question about a
> test which was deleted.

Thank you for review!
Done cleanup requests. I removed the test because the test does not make sense
anymore. See below comment.

>
https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
> File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):
> 
>
https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
> Source/core/html/canvas/WebGLRenderingContextBase.cpp:501:
> ASSERT(context.get());
> This can just be ASSERT(context). PassOwnPtr has an implicit conversion to
bool.

Good!. Done.

>
https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
> Source/core/html/canvas/WebGLRenderingContextBase.cpp:508:
> context.get()->getIntegerv(GL_MAX_VIEWPORT_DIMS, m_maxViewportDims);
> This can just be context->getIntegerv(...)

Done.

>
https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
> Source/core/html/canvas/WebGLRenderingContextBase.cpp:556:
> m_drawingBuffer->context()->getIntegerv(GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS,
> &numCombinedTextureImageUnits);
> m_drawingBuffer->context() is a boilerplate pattern throughout your code.
Could
> you add a private/protected helper method like "webContext()" or similar which
> returns that value? It could be added inline in the header.

Done.

>
https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
> Source/core/html/canvas/WebGLRenderingContextBase.cpp:748: m_colorMask[1] ?
> m_clearColor[1] : 0,
> The indentation looks strange here.

Done.

>
https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
> Source/core/html/canvas/WebGLRenderingContextBase.cpp:4165:
> blink::WebGraphicsContext3D* WebGLRenderingContextBase::webGraphicsContext3D()
> const
> Since this helper is here, please use it throughout the code. You could
consider
> renaming it to a shorter name like "webContext".

Done.

>
https://codereview.chromium.org/163773007/diff/220001/Source/core/html/canvas...
> Source/core/html/canvas/WebGLRenderingContextBase.cpp:5349: m_drawingBuffer =
> DrawingBuffer::create(context.release(), clampedCanvasSize(), preserve,
> contextEvictionManager.release());
> Are the offscreen context's resources fully released if DrawingBuffer::create
> returns NULL?

Yes, because DrawingBuffer destructor releases all resources.
DrawingBuffer::~DrawingBuffer()
{
    releaseResources();
}


>
https://codereview.chromium.org/163773007/diff/220001/Source/web/tests/Drawin...
> File Source/web/tests/DrawingBufferTest.cpp (left):
> 
>
https://codereview.chromium.org/163773007/diff/220001/Source/web/tests/Drawin...
> Source/web/tests/DrawingBufferTest.cpp:144: TEST_F(DrawingBufferTest,
> verifyNoNewBuffersAfterContextLostWithMailboxes)
> Why was this test removed? Is this just a mistake in rebasing?

Now DrawingBuffer::releaseResources() is private and only destructor calls it.
So calling prepareMailbox() after calling releaseResources() is impossible.

This test will resurrect in https://codereview.chromium.org/169933002/ because
the CL will define DrawingBuffer::beginDestruction() like releaseResources().

[dshwang, 2014-03-03 11:28:16]

hi, could you have a chance to review again?

[Ken Russell (switch to Gerrit), 2014-03-04 03:12:06]

I'm sorry again for taking so long to get to this.

Thanks for the contribution. Looks good. LGTM

https://codereview.chromium.org/163773007/diff/240001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/163773007/diff/240001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:129: , m_sampleCount(4)
Out of curiosity, was this change significant?

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

thank you for lgtm :)

https://codereview.chromium.org/163773007/diff/240001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/163773007/diff/240001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:129: , m_sampleCount(4)
On 2014/03/04 03:12:07, Ken Russell wrote:
> Out of curiosity, was this change significant?

DrawingBuffer::initialize() initializes the m_sampleCount like "m_sampleCount =
std::min(4, maxSampleCount);"

So I just add it. It's not big deal, and not related to this CL.

[dshwang, 2014-03-04 08:51:13]

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

[commit-bot: I haz the power, 2014-03-04 08:51:26]

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

[commit-bot: I haz the power, 2014-03-04 09:15:57]

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

[commit-bot: I haz the power, 2014-03-04 09:15:58]

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

[dshwang, 2014-03-04 09:47:58]

On 2014/03/04 09:15:58, I haz the power (commit-bot) wrote:
> Retried try job too often on blink_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

Hi jamesr@, could you review Source/web/tests/DrawingBufferTest.cpp ?

[dshwang, 2014-03-04 09:49:03]

On 2014/03/04 09:47:58, dshwang wrote:
> Hi jamesr@, could you review Source/web/tests/DrawingBufferTest.cpp ?

I remove one test.

Now DrawingBuffer::releaseResources() is private and only destructor calls it.
So calling prepareMailbox() after calling releaseResources() is impossible.

This test will resurrect in https://codereview.chromium.org/169933002/ because
the CL will define DrawingBuffer::beginDestruction() like releaseResources().

[dshwang, 2014-03-04 09:54:28]

https://codereview.chromium.org/163773007/diff/260001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/163773007/diff/260001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:129: , m_sampleCount(0)
I change from m_sampleCount(4) to m_sampleCount(0) for cosmetic.
m_sampleCount is used for only msaa case, which must initialize m_sampleCount =
std::min(4, maxSampleCount).

[Ken Russell (switch to Gerrit), 2014-03-04 12:35:08]

https://codereview.chromium.org/163773007/diff/260001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/163773007/diff/260001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:129: , m_sampleCount(0)
On 2014/03/04 09:54:28, dshwang wrote:
> I change from m_sampleCount(4) to m_sampleCount(0) for cosmetic.
> m_sampleCount is used for only msaa case, which must initialize m_sampleCount
=
> std::min(4, maxSampleCount).

That's fine, and clearer.

[dshwang, 2014-03-04 17:21:57]

On 2014/03/04 12:35:08, Ken Russell wrote:
> > std::min(4, maxSampleCount).
> 
> That's fine, and clearer.

Thank you for confirmation.

On 2014/03/04 09:15:58, I haz the power (commit-bot) wrote:
> Retried try job too often on blink_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

Hi jamesr@, could you review Source/web/tests/DrawingBufferTest.cpp ?
I hope kbr@ become owner of Source/web/tests also :)

[jamesr, 2014-03-04 18:13:13]

/web/tests/ lgtm

The best way to fix the OWNERS awkwardness here is to move the test into the
same directory as the code it tests.  It appears the only dependency this test
has that isn't already in /platform/ is /web/test/MockWebGraphicsContext3D.h,
which should be easy enough to move as well.

[Ken Russell (switch to Gerrit), 2014-03-04 18:21:08]

+a couple more Source/web/ OWNERS

[dshwang, 2014-03-04 18:44:09]

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

[commit-bot: I haz the power, 2014-03-04 18:45:00]

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

[commit-bot: I haz the power, 2014-03-05 09:33:00]

Change committed as 168457

[Ken Russell (switch to Gerrit), 2014-03-05 19:59:17]

Note: this was reverted because of assertion failures on the debug GPU bots. See
Issue 344393.

[dshwang, 2014-03-05 20:20:02]

On 2014/03/05 19:59:17, Ken Russell wrote:
> Note: this was reverted because of assertion failures on the debug GPU bots.
See
> Issue 344393.

Yes, thank you very much. I felt concern about lost context also, so I run
layout tests and webgl conformance test using debug build. It was not enough.
I’m glad that GPU bots catch it. I'll look at it carefully.