[Oilpan] Migrate classes under module/webgl onto oilpan heap (issue 1234883002 by peria@chromium.org)

pe...@chromium.org

unread,

Jul 23, 2015, 3:32:56 AM7/23/15

to oilpan-...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

Reviewers: oilpan-reviews,

Message:
Oilpan-team members,
PTL from point of Oilpan side.

Description:
[Oilpan] Migrate classes under module/webgl onto oilpan heap

BUG=497662

Please review this at https://codereview.chromium.org/1234883002/

Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Affected files (+765, -799 lines):
M Source/bindings/modules/v8/WebGLAny.h
M Source/bindings/modules/v8/WebGLAny.cpp
M Source/modules/webgl/ANGLEInstancedArrays.h
M Source/modules/webgl/ANGLEInstancedArrays.cpp
M Source/modules/webgl/ANGLEInstancedArrays.idl
M Source/modules/webgl/CHROMIUMSubscribeUniform.h
M Source/modules/webgl/CHROMIUMSubscribeUniform.cpp
M Source/modules/webgl/CHROMIUMSubscribeUniform.idl
M Source/modules/webgl/CHROMIUMValuebuffer.h
M Source/modules/webgl/CHROMIUMValuebuffer.cpp
M Source/modules/webgl/CHROMIUMValuebuffer.idl
M Source/modules/webgl/EXTBlendMinMax.h
M Source/modules/webgl/EXTBlendMinMax.cpp
M Source/modules/webgl/EXTBlendMinMax.idl
M Source/modules/webgl/EXTFragDepth.h
M Source/modules/webgl/EXTFragDepth.cpp
M Source/modules/webgl/EXTFragDepth.idl
M Source/modules/webgl/EXTShaderTextureLOD.h
M Source/modules/webgl/EXTShaderTextureLOD.cpp
M Source/modules/webgl/EXTShaderTextureLOD.idl
M Source/modules/webgl/EXTTextureFilterAnisotropic.h
M Source/modules/webgl/EXTTextureFilterAnisotropic.cpp
M Source/modules/webgl/EXTTextureFilterAnisotropic.idl
M Source/modules/webgl/EXTsRGB.h
M Source/modules/webgl/EXTsRGB.cpp
M Source/modules/webgl/EXTsRGB.idl
M Source/modules/webgl/OESElementIndexUint.h
M Source/modules/webgl/OESElementIndexUint.cpp
M Source/modules/webgl/OESElementIndexUint.idl
M Source/modules/webgl/OESStandardDerivatives.h
M Source/modules/webgl/OESStandardDerivatives.cpp
M Source/modules/webgl/OESStandardDerivatives.idl
M Source/modules/webgl/OESTextureFloat.h
M Source/modules/webgl/OESTextureFloat.cpp
M Source/modules/webgl/OESTextureFloat.idl
M Source/modules/webgl/OESTextureFloatLinear.h
M Source/modules/webgl/OESTextureFloatLinear.cpp
M Source/modules/webgl/OESTextureFloatLinear.idl
M Source/modules/webgl/OESTextureHalfFloat.h
M Source/modules/webgl/OESTextureHalfFloat.cpp
M Source/modules/webgl/OESTextureHalfFloat.idl
M Source/modules/webgl/OESTextureHalfFloatLinear.h
M Source/modules/webgl/OESTextureHalfFloatLinear.cpp
M Source/modules/webgl/OESTextureHalfFloatLinear.idl
M Source/modules/webgl/OESVertexArrayObject.h
M Source/modules/webgl/OESVertexArrayObject.cpp
M Source/modules/webgl/OESVertexArrayObject.idl
M Source/modules/webgl/WebGL2RenderingContext.h
M Source/modules/webgl/WebGL2RenderingContextBase.h
M Source/modules/webgl/WebGL2RenderingContextBase.cpp
M Source/modules/webgl/WebGLActiveInfo.h
M Source/modules/webgl/WebGLActiveInfo.idl
M Source/modules/webgl/WebGLBuffer.h
M Source/modules/webgl/WebGLBuffer.cpp
M Source/modules/webgl/WebGLBuffer.idl
M Source/modules/webgl/WebGLCompressedTextureATC.h
M Source/modules/webgl/WebGLCompressedTextureATC.cpp
M Source/modules/webgl/WebGLCompressedTextureATC.idl
M Source/modules/webgl/WebGLCompressedTextureETC1.h
M Source/modules/webgl/WebGLCompressedTextureETC1.cpp
M Source/modules/webgl/WebGLCompressedTextureETC1.idl
M Source/modules/webgl/WebGLCompressedTexturePVRTC.h
M Source/modules/webgl/WebGLCompressedTexturePVRTC.cpp
M Source/modules/webgl/WebGLCompressedTexturePVRTC.idl
M Source/modules/webgl/WebGLCompressedTextureS3TC.h
M Source/modules/webgl/WebGLCompressedTextureS3TC.cpp
M Source/modules/webgl/WebGLCompressedTextureS3TC.idl
M Source/modules/webgl/WebGLContextGroup.h
M Source/modules/webgl/WebGLContextGroup.cpp
M Source/modules/webgl/WebGLContextObject.cpp
M Source/modules/webgl/WebGLDebugRendererInfo.h
M Source/modules/webgl/WebGLDebugRendererInfo.cpp
M Source/modules/webgl/WebGLDebugRendererInfo.idl
M Source/modules/webgl/WebGLDebugShaders.h
M Source/modules/webgl/WebGLDebugShaders.cpp
M Source/modules/webgl/WebGLDebugShaders.idl
M Source/modules/webgl/WebGLDepthTexture.h
M Source/modules/webgl/WebGLDepthTexture.cpp
M Source/modules/webgl/WebGLDepthTexture.idl
M Source/modules/webgl/WebGLDrawBuffers.h
M Source/modules/webgl/WebGLDrawBuffers.cpp
M Source/modules/webgl/WebGLDrawBuffers.idl
M Source/modules/webgl/WebGLExtension.h
M Source/modules/webgl/WebGLFenceSync.h
M Source/modules/webgl/WebGLFenceSync.cpp
M Source/modules/webgl/WebGLFramebuffer.h
M Source/modules/webgl/WebGLFramebuffer.cpp
M Source/modules/webgl/WebGLFramebuffer.idl
M Source/modules/webgl/WebGLLoseContext.h
M Source/modules/webgl/WebGLLoseContext.cpp
M Source/modules/webgl/WebGLLoseContext.idl
M Source/modules/webgl/WebGLObject.h
M Source/modules/webgl/WebGLProgram.h
M Source/modules/webgl/WebGLProgram.cpp
M Source/modules/webgl/WebGLProgram.idl
M Source/modules/webgl/WebGLQuery.h
M Source/modules/webgl/WebGLQuery.cpp
M Source/modules/webgl/WebGLQuery.idl
M Source/modules/webgl/WebGLRenderbuffer.h
M Source/modules/webgl/WebGLRenderbuffer.cpp
M Source/modules/webgl/WebGLRenderbuffer.idl
M Source/modules/webgl/WebGLRenderingContext.h
M Source/modules/webgl/WebGLRenderingContextBase.h
M Source/modules/webgl/WebGLRenderingContextBase.cpp
M Source/modules/webgl/WebGLSampler.h
M Source/modules/webgl/WebGLSampler.cpp
M Source/modules/webgl/WebGLSampler.idl
M Source/modules/webgl/WebGLShader.h
M Source/modules/webgl/WebGLShader.cpp
M Source/modules/webgl/WebGLShader.idl
M Source/modules/webgl/WebGLShaderPrecisionFormat.h
M Source/modules/webgl/WebGLShaderPrecisionFormat.cpp
M Source/modules/webgl/WebGLShaderPrecisionFormat.idl
M Source/modules/webgl/WebGLSync.idl
M Source/modules/webgl/WebGLTexture.h
M Source/modules/webgl/WebGLTexture.cpp
M Source/modules/webgl/WebGLTexture.idl
M Source/modules/webgl/WebGLTransformFeedback.h
M Source/modules/webgl/WebGLTransformFeedback.cpp
M Source/modules/webgl/WebGLTransformFeedback.idl
M Source/modules/webgl/WebGLUniformLocation.h
M Source/modules/webgl/WebGLUniformLocation.cpp
M Source/modules/webgl/WebGLUniformLocation.idl
M Source/modules/webgl/WebGLVertexArrayObject.h
M Source/modules/webgl/WebGLVertexArrayObject.cpp
M Source/modules/webgl/WebGLVertexArrayObject.idl
M Source/modules/webgl/WebGLVertexArrayObjectBase.h
M Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
M Source/modules/webgl/WebGLVertexArrayObjectOES.h
M Source/modules/webgl/WebGLVertexArrayObjectOES.cpp
M Source/modules/webgl/WebGLVertexArrayObjectOES.idl

pe...@chromium.org

unread,

Jul 30, 2015, 2:33:55 AM7/30/15

to oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

WebGL guys,
I'm glad if you take a look on this CL beside reading the report paper.

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Jul 30, 2015, 2:55:31 AM7/30/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

On 2015/07/30 06:33:55, peria wrote:
> WebGL guys,
> I'm glad if you take a look on this CL beside reading the report paper.

(BTW, I'm happy to review the oilpan details once the webgl guys are happy
about
the performance/memory results.)

https://codereview.chromium.org/1234883002/

k...@chromium.org

unread,

Jul 31, 2015, 8:11:56 PM7/31/15

to pe...@chromium.org, oilpan-...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

+zmo

Thank you for moving this forward. Definitely in favor of moving WebGL to
Oilpan.

Could you please submit multiple tryjobs of this patch (at least 5 total)
to try
to ensure that no intermittent crashes are introduced on Linux, Mac and
Windows?
It would be undesirable if this CL had to be reverted because it made test
steps
like the webgl_conformance_tests flaky.

A few questions and requests. Thanks in advance.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGL2RenderingContext.h
File Source/modules/webgl/WebGL2RenderingContext.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGL2RenderingContext.h#newcode41
Source/modules/webgl/WebGL2RenderingContext.h:41:
PersistentWillBeMember<CHROMIUMSubscribeUniform>
m_chromiumSubscribeUniform;
Could you explain why these (and some other references) aren't being
converted to Members at this point?

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLContextGroup.h
File Source/modules/webgl/WebGLContextGroup.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLContextGroup.h#newcode74
Source/modules/webgl/WebGLContextGroup.h:74:
PersistentHeapHashSet<Member<WebGLSharedObject>> m_groupObjects;
I think we should file a bug about removing these m_groupObjects,
similarly to comment in WebGLRenderingContextBase about removing
m_contextObjects. I think that with sufficient modeling of the OpenGL
state so that live WebGLObjects don't get accidentally GC'd, we ought to
be able to avoid these weak references from the
WebGLRenderingContextBase to the objects it's allocated. Do you agree?

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLFramebuffer.cpp
File Source/modules/webgl/WebGLFramebuffer.cpp (left):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLFramebuffer.cpp#oldcode39
Source/modules/webgl/WebGLFramebuffer.cpp:39: class
WebGLRenderbufferAttachment final : public
WebGLFramebuffer::WebGLAttachment {
The whitespace changes in this file are unfortunate and make it hard to
review. Could you please submit a separate CL first, just removing the
spurious whitespace?

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLRenderingContextBase.cpp
File Source/modules/webgl/WebGLRenderingContextBase.cpp (left):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLRenderingContextBase.cpp#oldcode224
Source/modules/webgl/WebGLRenderingContextBase.cpp:224: //
ScopedDrawingBufferBinder is used for
ReadPixels/CopyTexImage2D/CopySubImage2D to read from
Similar issue with indentation in this block. Could you please submit a
separate CL fixing the indentation first?

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode483
Source/modules/webgl/WebGLRenderingContextBase.h:483:
PersistentHeapHashSetWillBeHeapHashSet<WeakMember<WebGLContextObject>>
m_contextObjects;
Should we file a bug about removing this weak map? In the new
garbage-collected world, it seems to me that the WebGLObjects should
have a strong reference to their WebGLRenderingContextBase, and the
WebGLRenderingContextBase should retain strong references to
WebGLObjects that have been registered as part of the context state. The
cycles are no longer a problem.

https://codereview.chromium.org/1234883002/

sigb...@opera.com

unread,

Aug 1, 2015, 2:49:13 PM8/1/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

pe...@chromium.org

unread,

Aug 3, 2015, 5:00:35 AM8/3/15

to oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/ANGLEInstancedArrays.h
File Source/modules/webgl/ANGLEInstancedArrays.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/ANGLEInstancedArrays.h#newcode34
Source/modules/webgl/ANGLEInstancedArrays.h:34: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:12, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/CHROMIUMSubscribeUniform.h
File Source/modules/webgl/CHROMIUMSubscribeUniform.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/CHROMIUMSubscribeUniform.h#newcode9
Source/modules/webgl/CHROMIUMSubscribeUniform.h:9: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:12, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/CHROMIUMValuebuffer.h
File Source/modules/webgl/CHROMIUMValuebuffer.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/CHROMIUMValuebuffer.h#newcode9
Source/modules/webgl/CHROMIUMValuebuffer.h:9: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:12, sof wrote:
> Can be removed now.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/EXTBlendMinMax.h
File Source/modules/webgl/EXTBlendMinMax.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/EXTBlendMinMax.h#newcode9
Source/modules/webgl/EXTBlendMinMax.h:9: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:12, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/EXTFragDepth.h
File Source/modules/webgl/EXTFragDepth.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/EXTFragDepth.h#newcode30
Source/modules/webgl/EXTFragDepth.h:30: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:12, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/EXTShaderTextureLOD.h
File Source/modules/webgl/EXTShaderTextureLOD.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/EXTShaderTextureLOD.h#newcode9
Source/modules/webgl/EXTShaderTextureLOD.h:9: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:12, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/EXTTextureFilterAnisotropic.h
File Source/modules/webgl/EXTTextureFilterAnisotropic.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/EXTTextureFilterAnisotropic.h#newcode30
Source/modules/webgl/EXTTextureFilterAnisotropic.h:30: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:12, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/EXTsRGB.h
File Source/modules/webgl/EXTsRGB.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/EXTsRGB.h#newcode9
Source/modules/webgl/EXTsRGB.h:9: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:12, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESElementIndexUint.h
File Source/modules/webgl/OESElementIndexUint.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESElementIndexUint.h#newcode30
Source/modules/webgl/OESElementIndexUint.h:30: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:12, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESStandardDerivatives.h
File Source/modules/webgl/OESStandardDerivatives.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESStandardDerivatives.h#newcode30
Source/modules/webgl/OESStandardDerivatives.h:30: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:12, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESTextureFloat.h
File Source/modules/webgl/OESTextureFloat.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESTextureFloat.h#newcode30
Source/modules/webgl/OESTextureFloat.h:30: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESTextureFloatLinear.h
File Source/modules/webgl/OESTextureFloatLinear.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESTextureFloatLinear.h#newcode30
Source/modules/webgl/OESTextureFloatLinear.h:30: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESTextureHalfFloat.h
File Source/modules/webgl/OESTextureHalfFloat.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESTextureHalfFloat.h#newcode30
Source/modules/webgl/OESTextureHalfFloat.h:30: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESTextureHalfFloatLinear.h
File Source/modules/webgl/OESTextureHalfFloatLinear.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESTextureHalfFloatLinear.h#newcode30
Source/modules/webgl/OESTextureHalfFloatLinear.h:30: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESVertexArrayObject.h
File Source/modules/webgl/OESVertexArrayObject.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/OESVertexArrayObject.h#newcode30
Source/modules/webgl/OESVertexArrayObject.h:30: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGL2RenderingContext.h
File Source/modules/webgl/WebGL2RenderingContext.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGL2RenderingContext.h#newcode41
Source/modules/webgl/WebGL2RenderingContext.h:41:
PersistentWillBeMember<CHROMIUMSubscribeUniform>
m_chromiumSubscribeUniform;

On 2015/08/01 00:11:56, Ken Russell wrote:
> Could you explain why these (and some other references) aren't being
converted
> to Members at this point?

Because doing it will effect on too many other classes.

Persistent pointers are used in non-Oilpan objects to figure Oilpan
objects, and WebGL2RenderingContext inherits a non-Oilpan class
CanvasRenderingContext.
To make those member variables to Member<> type values, we have to
migrate CanvasRenderingContext and its inheritances onto Oilpan heap.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGL2RenderingContextBase.h
File Source/modules/webgl/WebGL2RenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGL2RenderingContextBase.h#newcode10
Source/modules/webgl/WebGL2RenderingContextBase.h:10: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLActiveInfo.h
File Source/modules/webgl/WebGLActiveInfo.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLActiveInfo.h#newcode31
Source/modules/webgl/WebGLActiveInfo.h:31: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> wtf/ includes can be pruned.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLBuffer.h
File Source/modules/webgl/WebGLBuffer.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLBuffer.h#newcode30
Source/modules/webgl/WebGLBuffer.h:30: #include "wtf/Forward.h"

On 2015/08/01 18:49:13, sof wrote:
> Can remove the wtf/ includes afaict.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLCompressedTextureATC.h
File Source/modules/webgl/WebGLCompressedTextureATC.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLCompressedTextureATC.h#newcode30
Source/modules/webgl/WebGLCompressedTextureATC.h:30: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be.. removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLCompressedTextureETC1.h
File Source/modules/webgl/WebGLCompressedTextureETC1.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLCompressedTextureETC1.h#newcode9
Source/modules/webgl/WebGLCompressedTextureETC1.h:9: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLCompressedTexturePVRTC.h
File Source/modules/webgl/WebGLCompressedTexturePVRTC.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLCompressedTexturePVRTC.h#newcode30
Source/modules/webgl/WebGLCompressedTexturePVRTC.h:30: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLCompressedTextureS3TC.h
File Source/modules/webgl/WebGLCompressedTextureS3TC.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLCompressedTextureS3TC.h#newcode30
Source/modules/webgl/WebGLCompressedTextureS3TC.h:30: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLContextGroup.h
File Source/modules/webgl/WebGLContextGroup.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLContextGroup.h#newcode74
Source/modules/webgl/WebGLContextGroup.h:74:
PersistentHeapHashSet<Member<WebGLSharedObject>> m_groupObjects;

On 2015/08/01 00:11:56, Ken Russell wrote:
> I think we should file a bug about removing these m_groupObjects,
similarly to
> comment in WebGLRenderingContextBase about removing m_contextObjects.
I think
> that with sufficient modeling of the OpenGL state so that live
WebGLObjects
> don't get accidentally GC'd, we ought to be able to avoid these weak
references
> from the WebGLRenderingContextBase to the objects it's allocated. Do
you agree?

I agree to remove them, but I think it is not good to make those
reference map strong references.
I changed m_groupObjects a weak reference map.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLDebugRendererInfo.h
File Source/modules/webgl/WebGLDebugRendererInfo.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLDebugRendererInfo.h#newcode30
Source/modules/webgl/WebGLDebugRendererInfo.h:30: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLDebugShaders.h
File Source/modules/webgl/WebGLDebugShaders.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLDebugShaders.h#newcode30
Source/modules/webgl/WebGLDebugShaders.h:30: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLDepthTexture.h
File Source/modules/webgl/WebGLDepthTexture.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLDepthTexture.h#newcode30
Source/modules/webgl/WebGLDepthTexture.h:30: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:13, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLExtension.h
File Source/modules/webgl/WebGLExtension.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLExtension.h#newcode34
Source/modules/webgl/WebGLExtension.h:34: #include "wtf/RefCounted.h"

On 2015/08/01 18:49:13, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLExtension.h#newcode38
Source/modules/webgl/WebGLExtension.h:38: class
WebGLExtensionScopedContext : public
GarbageCollectedFinalized<WebGLExtensionScopedContext> {

On 2015/08/01 18:49:13, sof wrote:
> Can't we make this just STACK_ALLOCATED()?

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLExtension.h#newcode41
Source/modules/webgl/WebGLExtension.h:41:
WebGLExtensionScopedContext(WebGLExtension*);

On 2015/08/01 18:49:13, sof wrote:
> add explicit

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLExtension.h#newcode84
Source/modules/webgl/WebGLExtension.h:84:
RefPtrWillBeWeakMember<WebGLRenderingContextBase> m_context;

On 2015/08/01 18:49:13, sof wrote:
> Could you explain why this needs to be a RefPtr<> for the time being
non-Oilpan
> & why that doesn't bring about any ref cycles?

I reverted this change.
I'm not sure why I did this change...

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLFenceSync.h
File Source/modules/webgl/WebGLFenceSync.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLFenceSync.h#newcode9
Source/modules/webgl/WebGLFenceSync.h:9: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLFramebuffer.cpp
File Source/modules/webgl/WebGLFramebuffer.cpp (left):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLFramebuffer.cpp#oldcode39
Source/modules/webgl/WebGLFramebuffer.cpp:39: class
WebGLRenderbufferAttachment final : public
WebGLFramebuffer::WebGLAttachment {

On 2015/08/01 00:11:56, Ken Russell wrote:
> The whitespace changes in this file are unfortunate and make it hard
to review.
> Could you please submit a separate CL first, just removing the
spurious
> whitespace?

Acknowledged.
https://codereview.chromium.org/1267093003/

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLFramebuffer.h
File Source/modules/webgl/WebGLFramebuffer.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLFramebuffer.h#newcode31
Source/modules/webgl/WebGLFramebuffer.h:31: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> The wtf/ includes can now be retired.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLLoseContext.h
File Source/modules/webgl/WebGLLoseContext.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLLoseContext.h#newcode30
Source/modules/webgl/WebGLLoseContext.h:30: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLProgram.cpp
File Source/modules/webgl/WebGLProgram.cpp (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLProgram.cpp#newcode51
Source/modules/webgl/WebGLProgram.cpp:51: #if ENABLE(OILPAN)

On 2015/08/01 18:49:14, sof wrote:
> Could check/clarify why this is still needed? WebGLObject is now
always on the
> heap.

Done.
Thanks, this part seems not necessary.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLProgram.cpp#newcode123
Source/modules/webgl/WebGLProgram.cpp:123: return m_vertexShader.get();

On 2015/08/01 18:49:14, sof wrote:
> redundant get()

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLQuery.h
File Source/modules/webgl/WebGLQuery.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLQuery.h#newcode9
Source/modules/webgl/WebGLQuery.h:9: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLRenderbuffer.h
File Source/modules/webgl/WebGLRenderbuffer.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLRenderbuffer.h#newcode60
Source/modules/webgl/WebGLRenderbuffer.h:60: WebGLRenderbuffer*
emulatedStencilBuffer() const { return m_emulatedStencilBuffer.get(); }

On 2015/08/01 18:49:14, sof wrote:
> redundant get()

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLRenderingContextBase.cpp
File Source/modules/webgl/WebGLRenderingContextBase.cpp (left):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLRenderingContextBase.cpp#oldcode224
Source/modules/webgl/WebGLRenderingContextBase.cpp:224: //
ScopedDrawingBufferBinder is used for
ReadPixels/CopyTexImage2D/CopySubImage2D to read from

On 2015/08/01 00:11:56, Ken Russell wrote:
> Similar issue with indentation in this block. Could you please submit
a separate
> CL fixing the indentation first?

Acknowledged.
https://codereview.chromium.org/1258843005/

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode483
Source/modules/webgl/WebGLRenderingContextBase.h:483:
PersistentHeapHashSetWillBeHeapHashSet<WeakMember<WebGLContextObject>>
m_contextObjects;

On 2015/08/01 00:11:56, Ken Russell wrote:
> Should we file a bug about removing this weak map? In the new
garbage-collected
> world, it seems to me that the WebGLObjects should have a strong
reference to
> their WebGLRenderingContextBase, and the WebGLRenderingContextBase
should retain
> strong references to WebGLObjects that have been registered as part of
the
> context state. The cycles are no longer a problem.

I agree that the reference cycle will not make orphaned leaks, but I
think it better to keep this weak reference map.
If we change it to strong one, we may keep unused WebGLObjects alive in
following situation. We can see it as a leak.

root==>usingWebGLObject==>WebGLContextBase==>unusedWebGLObject

If it is a weak reference map, we can cut this chaining reference and
release the unused object.

root==>usingWebGLOjbect==>WebGLContextBase-(cut)->unusedWebGLObject

WDYT?

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLSampler.h
File Source/modules/webgl/WebGLSampler.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLSampler.h#newcode9
Source/modules/webgl/WebGLSampler.h:9: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLShader.h
File Source/modules/webgl/WebGLShader.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLShader.h#newcode30
Source/modules/webgl/WebGLShader.h:30: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLShaderPrecisionFormat.h
File Source/modules/webgl/WebGLShaderPrecisionFormat.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLShaderPrecisionFormat.h#newcode33
Source/modules/webgl/WebGLShaderPrecisionFormat.h:33: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> Remove the wtf/ includes.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLTexture.h
File Source/modules/webgl/WebGLTexture.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLTexture.h#newcode30
Source/modules/webgl/WebGLTexture.h:30: #include "wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLTransformFeedback.h
File Source/modules/webgl/WebGLTransformFeedback.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLTransformFeedback.h#newcode9
Source/modules/webgl/WebGLTransformFeedback.h:9: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLUniformLocation.cpp
File Source/modules/webgl/WebGLUniformLocation.cpp (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLUniformLocation.cpp#newcode52
Source/modules/webgl/WebGLUniformLocation.cpp:52: return
m_program.get();

On 2015/08/01 18:49:14, sof wrote:
> nit: redundant get()

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLUniformLocation.h
File Source/modules/webgl/WebGLUniformLocation.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLUniformLocation.h#newcode32
Source/modules/webgl/WebGLUniformLocation.h:32: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> Remove the wtf/ includes.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLVertexArrayObject.h
File Source/modules/webgl/WebGLVertexArrayObject.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLVertexArrayObject.h#newcode9
Source/modules/webgl/WebGLVertexArrayObject.h:9: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLVertexArrayObjectBase.h
File Source/modules/webgl/WebGLVertexArrayObjectBase.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLVertexArrayObjectBase.h#newcode11
Source/modules/webgl/WebGLVertexArrayObjectBase.h:11: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLVertexArrayObjectOES.h
File Source/modules/webgl/WebGLVertexArrayObjectOES.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/WebGLVertexArrayObjectOES.h#newcode30
Source/modules/webgl/WebGLVertexArrayObjectOES.h:30: #include
"wtf/PassRefPtr.h"

On 2015/08/01 18:49:14, sof wrote:
> Can be removed.

Done.

https://codereview.chromium.org/1234883002/

sigb...@opera.com

unread,

Aug 3, 2015, 5:12:05 AM8/3/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLExtension.h
File Source/modules/webgl/WebGLExtension.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLExtension.h#newcode37
Source/modules/webgl/WebGLExtension.h:37: class
WebGLExtensionScopedContext {
Add 'final' for maximum tidiness.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLExtension.h#newcode42
Source/modules/webgl/WebGLExtension.h:42: virtual
~WebGLExtensionScopedContext();
No need for virtual, I think. You can arguably just remove the dtor.

(This scope object is a featherlight abstraction.)

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLExtension.h#newcode47
Source/modules/webgl/WebGLExtension.h:47: DECLARE_VIRTUAL_TRACE();
No longer needed.

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 3, 2015, 8:06:52 PM8/3/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

Thanks for working on this! A couple of comments and questions (for
Sigbjorn).

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/CHROMIUMValuebuffer.cpp
File Source/modules/webgl/CHROMIUMValuebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/CHROMIUMValuebuffer.cpp#newcode27
Source/modules/webgl/CHROMIUMValuebuffer.cpp:27:
detachAndDeleteObject();

Sigbjorn: I'm sorry for asking similar questions repeatedly, but help me
understand. detachAndDeleteObject() calls
WebGLObject::getAWebGraphicsContext3D and it touches
WebGLObject::m_context, which is another on-heap object. Why is it safe?

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContext.h
File Source/modules/webgl/WebGL2RenderingContext.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContext.h#newcode50
Source/modules/webgl/WebGL2RenderingContext.h:50:
PersistentWillBeMember<WebGLLoseContext> m_webglLoseContext;

These persistent handles can be removed if you move
CanvasRenderingContext to the oilpan heap at the same time. Is there any
reason that makes it hard to move CanvasRenderingContext to the oilpan
heap?

Either way, I think we can keep the current CL (already huge) as is and
move CanvasRenderingContext in a follow-up CL.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContextBase.h
File Source/modules/webgl/WebGL2RenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContextBase.h#newcode223
Source/modules/webgl/WebGL2RenderingContextBase.h:223:
PersistentHeapVectorWillBeHeapVector<Member<WebGLSampler>>
m_samplerUnits;

Ditto. Let's move CanvasRenderingContext to the oilpan heap and remove
these persistent handles in a follow-up CL.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLContextGroup.h
File Source/modules/webgl/WebGLContextGroup.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLContextGroup.h#newcode74
Source/modules/webgl/WebGLContextGroup.h:74:
PersistentHeapHashSet<WeakMember<WebGLSharedObject>> m_groupObjects;

It looks harmless to make these raw pointers weak, but the handling of
the raw pointers are complicated. Let's keep them as raw pointers in
this CL and address them in a follow-up CL.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLProgram.cpp
File Source/modules/webgl/WebGLProgram.cpp (left):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLProgram.cpp#oldcode57
Source/modules/webgl/WebGLProgram.cpp:57: m_fragmentShader = nullptr;

You shouldn't remove this. This code is necessary for oilpan builds to
avoid the destructor from touching m_vertexShader and m_fragmentShader.
The following detachAndDeleteObject() calls
WebGLProgram::deleteObjectImpl and touches those objects.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLQuery.cpp
File Source/modules/webgl/WebGLQuery.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLQuery.cpp#newcode22
Source/modules/webgl/WebGLQuery.cpp:22: // with or without Oilpan
enabled.

Now that we're shipping Oilpan, remove a comment that refers to 'without
Oilpan'. The same comment for other parts in this CL.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderbuffer.cpp
File Source/modules/webgl/WebGLRenderbuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderbuffer.cpp#newcode41
Source/modules/webgl/WebGLRenderbuffer.cpp:41: #if ENABLE(OILPAN)

You need to remove the #if ENABLE(OILPAN). (Note that you need to keep
m_emulatedStencilBuffer.clear().)

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContext.h
File Source/modules/webgl/WebGLRenderingContext.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContext.h#newcode86
Source/modules/webgl/WebGLRenderingContext.h:86:
PersistentWillBeMember<WebGLDepthTexture> m_webglDepthTexture;

Ditto. Let's move CanvasRenderingContext to the oilpan heap and remove
these persistent handles in a follow-up CL.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.cpp
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5147
Source/modules/webgl/WebGLRenderingContextBase.cpp:5147: auto it =
m_contextObjects.begin();

It would be worth having a comment and mention that the detachContext()
can remove the object from the m_contextObjects (and thus we need to
look up the begin() every time).

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode386
Source/modules/webgl/WebGLRenderingContextBase.h:386:
EAGERLY_FINALIZE();

Sigbjorn: It looks like the destructor of WebGLRenderingContextBase is
touching a lot of on-heap objects (I couldn't verify that they are not
touching eagerly-finalized objects). Maybe it would be safer to make it
a pre-finalizer given that the number of WebGLRenderingContextBase
should be limited.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode544
Source/modules/webgl/WebGLRenderingContextBase.h:544:
GC_PLUGIN_IGNORE("crbug.com/496496")

I think we should remove the GC_PLUGIN_IGNORE by removing the code that
clears m_textureUnits[i] in WebGLRenderingContextBase's destructor.

Let's address it in a follow-up CL.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode549
Source/modules/webgl/WebGLRenderingContextBase.h:549:
PersistentWillBeMember<WebGLTexture> m_blackTextureCubeMap;

Ditto. Let's move CanvasRenderingContext to the oilpan heap and remove
these persistent handles in a follow-up CL.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode696
Source/modules/webgl/WebGLRenderingContextBase.h:696:
PersistentWillBeMember<T>& m_extensionField;

It looks really nasty to keep a reference to a PersistentWillBeMember.
BTW, what is m_extensionField used for? Maybe we can just remove it?

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode34
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:34:
m_destructionInProgress = true;

Instead of introducing another flag like m_destructionInProgress, I'd
prefer just clearing out m_boundElementArrayBuffer and
m_vertexAttribState here.

c.f., see what WebGLRenderbuffer::~WebGLRenderbuffer() is doing.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode49
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:49: if
(m_boundElementArrayBuffer)

If you apply the above change, you'll need to add:

if (!m_boundElementArrayBuffer)
return;
if (!m_vertexAttribState)
return;

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode76
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:76:
dispatchDetached(context3d);

Sigbjorn: Do we still need to call dispatchDetached here? In oilpan, the
lifetime of a WebGL object is explicitly managed by the WebGL object
itself calling WebGL::detachAndDeleteObject(). So I'm wondering if we
can completely remove WebGLObject::m_attachmentCount. If we can remove
it, we can remove WebGLObject::onDetached(). Then we can remove
dispatchDetached().

(Either way, we can handle this in a follow-up CL.)

https://codereview.chromium.org/1234883002/

k...@chromium.org

unread,

Aug 3, 2015, 8:34:11 PM8/3/15

to pe...@chromium.org, oilpan-...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

sof@, haraken@: thank you both for helping review this CL. I don't
understand
the nuances of Oilpan, so your reviews are invaluable. I do think that
moving
this module to Oilpan will significantly reduce maintenance costs going
forward.

I'd like to ask zmo@ and bajones@ to also take a look at this so that we
can all
begin to familiarize ourselves with the new conventions.

Will rubber-stamp this once haraken@ and sof@ are happy with it. A few
comments.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContext.h
File Source/modules/webgl/WebGL2RenderingContext.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContext.h#newcode50
Source/modules/webgl/WebGL2RenderingContext.h:50:
PersistentWillBeMember<WebGLLoseContext> m_webglLoseContext;

On 2015/08/04 00:06:52, haraken wrote:

> These persistent handles can be removed if you move
CanvasRenderingContext to
> the oilpan heap at the same time. Is there any reason that makes it
hard to move
> CanvasRenderingContext to the oilpan heap?

> Either way, I think we can keep the current CL (already huge) as is
and move
> CanvasRenderingContext in a follow-up CL.

Understanding that this is the reason for the persistent handles, let's
definitely leave these as persistent handles rather than weak members.
Moving CanvasRenderingContext into the Oilpan heap at the same time as
the WebGLRenderingContext sounds like too much to do in a single CL. I'm
sure junov@ will agree.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLContextGroup.h
File Source/modules/webgl/WebGLContextGroup.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLContextGroup.h#newcode74
Source/modules/webgl/WebGLContextGroup.h:74:
PersistentHeapHashSet<WeakMember<WebGLSharedObject>> m_groupObjects;

On 2015/08/04 00:06:52, haraken wrote:

> It looks harmless to make these raw pointers weak, but the handling of
the raw
> pointers are complicated. Let's keep them as raw pointers in this CL
and address
> them in a follow-up CL.

+1 for keeping the behavioral changes as simple as possible in this CL.
Appreciate your continued work on this after this CL lands.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode696
Source/modules/webgl/WebGLRenderingContextBase.h:696:
PersistentWillBeMember<T>& m_extensionField;

On 2015/08/04 00:06:52, haraken wrote:

> It looks really nasty to keep a reference to a PersistentWillBeMember.
BTW, what
> is m_extensionField used for? Maybe we can just remove it?

I'm pretty sure that this is intended to keep the JavaScript-side
extension object alive for the lifetime of the WebGLRenderingContext.
The spec says that once you fetch a particular extension for a context
(e.g. gl.getExtension("OES_texture_float"), the same exact object is
returned for all future getExtension calls. There are conformance tests
that verify that user-defined expando properties on these objects
continue to exist over time.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode76
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:76:
dispatchDetached(context3d);

On 2015/08/04 00:06:52, haraken wrote:

> Sigbjorn: Do we still need to call dispatchDetached here? In oilpan,
the
> lifetime of a WebGL object is explicitly managed by the WebGL object
itself
> calling WebGL::detachAndDeleteObject(). So I'm wondering if we can
completely
> remove WebGLObject::m_attachmentCount. If we can remove it, we can
remove
> WebGLObject::onDetached(). Then we can remove dispatchDetached().

> (Either way, we can handle this in a follow-up CL.)

One note about this. In order to maintain uniform behavior across many
vendors' OpenGL drivers, it has been found necessary in the past to
defer destruction of certain OpenGL objects. For example, if a shader is
attached to a program and the user calls deleteShader against it, we
need to delay the destruction of the shader until the program is
deleted. Same for renderbuffers and textures that are attached to
framebuffer objects.

It's possible that we could reevaluate the need to do this work at the
Blink layer -- it may be well handled by Chromium's command buffer at
this point. We should discuss this more. However, in the spirit of
minimizing the amount of changes in this already-large CL, yes, this
should be considered for a follow-on CL.

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 3, 2015, 8:47:06 PM8/3/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

Thanks kbr!

peria-san: It seems we have a bunch of items we need to address in
follow-up CLs
(including kbr's previous comments about the hash map handling). Shall we
file
bugs for all of them in order not to miss them?

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode696
Source/modules/webgl/WebGLRenderingContextBase.h:696:
PersistentWillBeMember<T>& m_extensionField;

On 2015/08/04 00:34:11, Ken Russell wrote:
> On 2015/08/04 00:06:52, haraken wrote:
> >
> > It looks really nasty to keep a reference to a
PersistentWillBeMember. BTW,
> what
> > is m_extensionField used for? Maybe we can just remove it?

> I'm pretty sure that this is intended to keep the JavaScript-side
extension
> object alive for the lifetime of the WebGLRenderingContext. The spec
says that
> once you fetch a particular extension for a context (e.g.
> gl.getExtension("OES_texture_float"), the same exact object is
returned for all
> future getExtension calls. There are conformance tests that verify
that
> user-defined expando properties on these objects continue to exist
over time.

Maybe a better idea would be:

- make TypedExtensionTracker hold a strong reference to
WebGL2RenderingContext.
- call WebGL2RenderingContext::setExtension(extension) when the
TypedExtensionTracker needs to set the extension on the things on the
WebGL2RenderingContext.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode76
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:76:
dispatchDetached(context3d);

Thanks for the context. I now understand why the onDetached calls are
needed.

(Another option might be to remove the onDetached calls and keep the
underlying platform object alive until the WebGLObject gets destructed
(the underlying platform object is destructed in WebGLObject's
destructor). That said, it will prolong the platform object lifetime too
much.)

https://codereview.chromium.org/1234883002/

k...@chromium.org

unread,

Aug 3, 2015, 8:54:19 PM8/3/15

to pe...@chromium.org, oilpan-...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode696
Source/modules/webgl/WebGLRenderingContextBase.h:696:
PersistentWillBeMember<T>& m_extensionField;

I'm not sure I understand the second suggestion.

To be clear: WebGLRenderingContext::getExtension maps strings to
objects. See https://www.khronos.org/registry/webgl/specs/latest/1.0/ .
The invariant is that for a particular extension string, the same exact
extension object must always be returned for the life of the WebGL
context. So long as that invariant is maintained, the structure can be
changed however you like for Oilpan.

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 3, 2015, 8:59:14 PM8/3/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

What we want to do in WebGLRenderingContext::getExtension is to set the
extension on WebGL2RenderingContext::m_chromiumSubscribeUniform etc.
Currently
we do this by registering "the reference of m_chromiumSubscribeUniform" on
the
TypedExtensionTracker. Instead of doing that, I thought we can store "the
strong
pointer to the WebGL2RenderingContext" on the TypedExtensionTracker. Then
the
TypedExtensionTracker can call context->setExtension(...) and ask the
WebGL2RenderingContext to set the extension on m_chromiumSubscribeUniform
etc.

https://codereview.chromium.org/1234883002/

k...@chromium.org

unread,

Aug 3, 2015, 9:16:03 PM8/3/15

to pe...@chromium.org, oilpan-...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

In the pre-Oilpan world that would introduce a reference cycle between the
extension objects and the WebGLRenderingContext/WebGL2RenderingContext
which is
forbidden. That's why the extension objects themselves don't maintain strong
references to the WebGLRenderingContext.

With Oilpan, we could certainly make that change, or a similar one. It
looks to
me like the explicit fields like m_angleInstancedArrays aren't really
needed,
and that all that's needed is some collection of the enabled WebGLExtension
objects in WebGLRenderingContext and WebGL2RenderingContext. Is that right?
Can
DEFINE_TRACE(WebGLRenderingContext) and
DEFINE_TRACE(WebGL2RenderingContext) be
updated to just trace a collection object? If so, let's make the structural
change you suggest. We could add a pure virtual
WebGLRenderingContextBase::addEnabledExtension which would be overridden by
WebGLRenderingContext and WebGL2RenderingContext, and which would just add
the
extension object to the collection.

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 3, 2015, 9:22:02 PM8/3/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

Totally agreed :)

https://codereview.chromium.org/1234883002/

sigb...@opera.com

unread,

Aug 4, 2015, 1:11:56 AM8/4/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

We seem to end up rehashing/repeating Oilpan WebGL object details whenever
that
code is touched; let me follow up on the other issues separately.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode386
Source/modules/webgl/WebGLRenderingContextBase.h:386:
EAGERLY_FINALIZE();

On 2015/08/04 00:06:52, haraken wrote:

> Sigbjorn: It looks like the destructor of WebGLRenderingContextBase is
touching
> a lot of on-heap objects (I couldn't verify that they are not touching
> eagerly-finalized objects). Maybe it would be safer to make it a
pre-finalizer
> given that the number of WebGLRenderingContextBase should be limited.

They're all WebGLObjects (and WebGLSharedObjects), and there's no
conflict/problematic access on finalization. WebGLRenderingContextBase
was one of the motivations for introducing eager finalization. Having it
in place allowed us to remove the involved finalization protocol that
was used prior to that for Oilpan.

https://codereview.chromium.org/1234883002/

sigb...@opera.com

unread,

Aug 4, 2015, 2:20:46 AM8/4/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

I can try to add some comments regarding finalization somewhere near to
WebGLRenderingContextBase, if that would be helpful.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/CHROMIUMValuebuffer.cpp
File Source/modules/webgl/CHROMIUMValuebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/CHROMIUMValuebuffer.cpp#newcode27
Source/modules/webgl/CHROMIUMValuebuffer.cpp:27:
detachAndDeleteObject();

On 2015/08/04 00:06:52, haraken wrote:

> Sigbjorn: I'm sorry for asking similar questions repeatedly, but help
me
> understand. detachAndDeleteObject() calls
WebGLObject::getAWebGraphicsContext3D
> and it touches WebGLObject::m_context, which is another on-heap
object. Why is
> it safe?

It is safe because:

- if m_context (WebGLRenderingContextBase) is finalized at the same
time as this WebGLSharedObject, it (m_context) will have been (eagerly)
finalized first. At which point it will have called
detachAndRemoveAllObjects() to explicitly detach this & other objects.
- if m_context isn't finalized at the same time, detaching this object
needs to happen. Which is safe, as m_context is still alive.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContextBase.h
File Source/modules/webgl/WebGL2RenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContextBase.h#newcode223
Source/modules/webgl/WebGL2RenderingContextBase.h:223:
PersistentHeapVectorWillBeHeapVector<Member<WebGLSampler>>
m_samplerUnits;

On 2015/08/04 00:06:52, haraken wrote:

> Ditto. Let's move CanvasRenderingContext to the oilpan heap and remove
these
> persistent handles in a follow-up CL.

It's an ActiveDOMObject, so it'll have to wait until the big switch.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode76
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:76:
dispatchDetached(context3d);

On 2015/08/04 00:06:52, haraken wrote:

> Sigbjorn: Do we still need to call dispatchDetached here? In oilpan,
the
> lifetime of a WebGL object is explicitly managed by the WebGL object
itself
> calling WebGL::detachAndDeleteObject(). So I'm wondering if we can
completely
> remove WebGLObject::m_attachmentCount. If we can remove it, we can
remove
> WebGLObject::onDetached(). Then we can remove dispatchDetached().

> (Either way, we can handle this in a follow-up CL.)

Good point, I think we can remove the Oilpan specific part (i.e., remove
m_destructionInProgress) due to the partial ordering of finalizers that
we've now got here.

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 4, 2015, 2:35:41 AM8/4/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/CHROMIUMValuebuffer.cpp
File Source/modules/webgl/CHROMIUMValuebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/CHROMIUMValuebuffer.cpp#newcode27
Source/modules/webgl/CHROMIUMValuebuffer.cpp:27:
detachAndDeleteObject();

On 2015/08/04 06:20:45, sof wrote:
> On 2015/08/04 00:06:52, haraken wrote:
> >
> > Sigbjorn: I'm sorry for asking similar questions repeatedly, but
help me
> > understand. detachAndDeleteObject() calls
> WebGLObject::getAWebGraphicsContext3D
> > and it touches WebGLObject::m_context, which is another on-heap
object. Why is
> > it safe?

> It is safe because:

> - if m_context (WebGLRenderingContextBase) is finalized at the same
time as
> this WebGLSharedObject, it (m_context) will have been (eagerly)
finalized first.
> At which point it will have called detachAndRemoveAllObjects() to
explicitly
> detach this & other objects.
> - if m_context isn't finalized at the same time, detaching this
object needs to
> happen. Which is safe, as m_context is still alive.

Thanks, I'm now convinced. I'll try not to forget it :)

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContextBase.h
File Source/modules/webgl/WebGL2RenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContextBase.h#newcode223
Source/modules/webgl/WebGL2RenderingContextBase.h:223:
PersistentHeapVectorWillBeHeapVector<Member<WebGLSampler>>
m_samplerUnits;

On 2015/08/04 06:20:45, sof wrote:
> On 2015/08/04 00:06:52, haraken wrote:
> >
> > Ditto. Let's move CanvasRenderingContext to the oilpan heap and
remove these
> > persistent handles in a follow-up CL.

> It's an ActiveDOMObject, so it'll have to wait until the big switch.

What prevents us from shipping oilpan for ActiveDOMObjects? As far as I
understand, it's safe to ship oilpan for objects that inherit from
ActiveDOMObjects without shipping oilpan for ActiveDOMObjects. In fact,
we did this for MediaDevicesRequest etc.

https://codereview.chromium.org/1234883002/

sigb...@opera.com

unread,

Aug 4, 2015, 2:37:58 AM8/4/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContextBase.h
File Source/modules/webgl/WebGL2RenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContextBase.h#newcode223
Source/modules/webgl/WebGL2RenderingContextBase.h:223:
PersistentHeapVectorWillBeHeapVector<Member<WebGLSampler>>
m_samplerUnits;

On 2015/08/04 06:35:40, haraken wrote:
> On 2015/08/04 06:20:45, sof wrote:
> > On 2015/08/04 00:06:52, haraken wrote:
> > >
> > > Ditto. Let's move CanvasRenderingContext to the oilpan heap and
remove these
> > > persistent handles in a follow-up CL.
> >
> > It's an ActiveDOMObject, so it'll have to wait until the big switch.

> What prevents us from shipping oilpan for ActiveDOMObjects? As far as
I
> understand, it's safe to ship oilpan for objects that inherit from
> ActiveDOMObjects without shipping oilpan for ActiveDOMObjects. In
fact, we did
> this for MediaDevicesRequest etc.

It's associated with an HTMLCanvasElement..

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 4, 2015, 2:42:03 AM8/4/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

ah, makes sense.

https://codereview.chromium.org/1234883002/

pe...@chromium.org

unread,

Aug 4, 2015, 5:15:45 AM8/4/15

to oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContext.h
File Source/modules/webgl/WebGL2RenderingContext.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContext.h#newcode50
Source/modules/webgl/WebGL2RenderingContext.h:50:
PersistentWillBeMember<WebGLLoseContext> m_webglLoseContext;

On 2015/08/04 00:34:10, Ken Russell wrote:
> On 2015/08/04 00:06:52, haraken wrote:
> >

> > These persistent handles can be removed if you move
CanvasRenderingContext to

> > the oilpan heap at the same time. Is there any reason that makes it
hard to

> move

> > CanvasRenderingContext to the oilpan heap?
> >
> > Either way, I think we can keep the current CL (already huge) as is
and move
> > CanvasRenderingContext in a follow-up CL.

> Understanding that this is the reason for the persistent handles,
let's
> definitely leave these as persistent handles rather than weak members.
Moving
> CanvasRenderingContext into the Oilpan heap at the same time as the
> WebGLRenderingContext sounds like too much to do in a single CL. I'm
sure junov@
> will agree.

Acknowledged.
http://crbug.com/516607

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContextBase.h
File Source/modules/webgl/WebGL2RenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGL2RenderingContextBase.h#newcode223
Source/modules/webgl/WebGL2RenderingContextBase.h:223:
PersistentHeapVectorWillBeHeapVector<Member<WebGLSampler>>
m_samplerUnits;

On 2015/08/04 00:06:52, haraken wrote:

> Ditto. Let's move CanvasRenderingContext to the oilpan heap and remove
these
> persistent handles in a follow-up CL.

Acknowledged.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLContextGroup.h
File Source/modules/webgl/WebGLContextGroup.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLContextGroup.h#newcode74
Source/modules/webgl/WebGLContextGroup.h:74:
PersistentHeapHashSet<WeakMember<WebGLSharedObject>> m_groupObjects;

On 2015/08/04 00:34:10, Ken Russell wrote:
> On 2015/08/04 00:06:52, haraken wrote:
> >

> > It looks harmless to make these raw pointers weak, but the handling
of the raw
> > pointers are complicated. Let's keep them as raw pointers in this CL
and
> address
> > them in a follow-up CL.
> >

> +1 for keeping the behavioral changes as simple as possible in this
CL.
> Appreciate your continued work on this after this CL lands.

Done.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLExtension.h
File Source/modules/webgl/WebGLExtension.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLExtension.h#newcode37
Source/modules/webgl/WebGLExtension.h:37: class
WebGLExtensionScopedContext {

On 2015/08/03 09:12:04, sof wrote:
> Add 'final' for maximum tidiness.

Done.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLExtension.h#newcode42
Source/modules/webgl/WebGLExtension.h:42: virtual
~WebGLExtensionScopedContext();

On 2015/08/03 09:12:04, sof wrote:
> No need for virtual, I think. You can arguably just remove the dtor.

> (This scope object is a featherlight abstraction.)

Done.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLExtension.h#newcode47
Source/modules/webgl/WebGLExtension.h:47: DECLARE_VIRTUAL_TRACE();

On 2015/08/03 09:12:04, sof wrote:
> No longer needed.

Done.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLProgram.cpp
File Source/modules/webgl/WebGLProgram.cpp (left):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLProgram.cpp#oldcode57
Source/modules/webgl/WebGLProgram.cpp:57: m_fragmentShader = nullptr;

On 2015/08/04 00:06:52, haraken wrote:

> You shouldn't remove this. This code is necessary for oilpan builds to
avoid the
> destructor from touching m_vertexShader and m_fragmentShader. The
following
> detachAndDeleteObject() calls WebGLProgram::deleteObjectImpl and
touches those
> objects.

Done.
I did not notice they are touched in detachAndDeleteObject().

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLQuery.cpp
File Source/modules/webgl/WebGLQuery.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLQuery.cpp#newcode22
Source/modules/webgl/WebGLQuery.cpp:22: // with or without Oilpan
enabled.

On 2015/08/04 00:06:52, haraken wrote:

> Now that we're shipping Oilpan, remove a comment that refers to
'without
> Oilpan'. The same comment for other parts in this CL.

Done.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderbuffer.cpp
File Source/modules/webgl/WebGLRenderbuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderbuffer.cpp#newcode41
Source/modules/webgl/WebGLRenderbuffer.cpp:41: #if ENABLE(OILPAN)

On 2015/08/04 00:06:52, haraken wrote:

> You need to remove the #if ENABLE(OILPAN). (Note that you need to keep
> m_emulatedStencilBuffer.clear().)

Done.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContext.h
File Source/modules/webgl/WebGLRenderingContext.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContext.h#newcode86
Source/modules/webgl/WebGLRenderingContext.h:86:
PersistentWillBeMember<WebGLDepthTexture> m_webglDepthTexture;

On 2015/08/04 00:06:52, haraken wrote:

> Ditto. Let's move CanvasRenderingContext to the oilpan heap and remove
these
> persistent handles in a follow-up CL.

Acknowledged.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.cpp
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5147
Source/modules/webgl/WebGLRenderingContextBase.cpp:5147: auto it =
m_contextObjects.begin();

On 2015/08/04 00:06:52, haraken wrote:

> It would be worth having a comment and mention that the
detachContext() can
> remove the object from the m_contextObjects (and thus we need to look
up the
> begin() every time).

Done.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode544
Source/modules/webgl/WebGLRenderingContextBase.h:544:
GC_PLUGIN_IGNORE("crbug.com/496496")

On 2015/08/04 00:06:52, haraken wrote:

> I think we should remove the GC_PLUGIN_IGNORE by removing the code
that clears
> m_textureUnits[i] in WebGLRenderingContextBase's destructor.

> Let's address it in a follow-up CL.

Acknowledged.
http://crbug.com/516604

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode549
Source/modules/webgl/WebGLRenderingContextBase.h:549:
PersistentWillBeMember<WebGLTexture> m_blackTextureCubeMap;

On 2015/08/04 00:06:52, haraken wrote:

> Ditto. Let's move CanvasRenderingContext to the oilpan heap and remove
these
> persistent handles in a follow-up CL.

Acknowledged.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode34
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:34:
m_destructionInProgress = true;

On 2015/08/04 00:06:52, haraken wrote:

> Instead of introducing another flag like m_destructionInProgress, I'd
prefer
> just clearing out m_boundElementArrayBuffer and m_vertexAttribState
here.

> c.f., see what WebGLRenderbuffer::~WebGLRenderbuffer() is doing.

will do in another CL.
http://crbug.com/516613

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode49
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:49: if
(m_boundElementArrayBuffer)

On 2015/08/04 00:06:52, haraken wrote:

> If you apply the above change, you'll need to add:

> if (!m_boundElementArrayBuffer)
> return;
> if (!m_vertexAttribState)
> return;

Acknowledged.

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode76
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:76:
dispatchDetached(context3d);

On 2015/08/04 00:06:52, haraken wrote:

> Sigbjorn: Do we still need to call dispatchDetached here? In oilpan,
the
> lifetime of a WebGL object is explicitly managed by the WebGL object
itself
> calling WebGL::detachAndDeleteObject(). So I'm wondering if we can
completely
> remove WebGLObject::m_attachmentCount. If we can remove it, we can
remove
> WebGLObject::onDetached(). Then we can remove dispatchDetached().

> (Either way, we can handle this in a follow-up CL.)

Acknowledged.
http://crbug.com/516613

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 5, 2015, 12:12:19 AM8/5/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

I'll take another look once you fix the layout test failures.

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLBuffer.cpp
File Source/modules/webgl/WebGLBuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLBuffer.cpp#newcode53
Source/modules/webgl/WebGLBuffer.cpp:53: // object is indeed deleted.

Slightly clearer:

There is no guarantee about the order between WebGLContextGroup's
destruction and WebGLBuffer's destruction. So we cannot rely on the
WebGLContextGroup's destructor for deleting the platform objects of the
WebGLBuffers. Hence we delete the platform objects in the WebGLBuffer's
destructor.

Also it would be better to move this comment to
WebGLObject::detachAndDeleteObject().

- Have the comment in WebGLObject::detachAndDeleteObject().
- In each destructor of an object that inherits from WebGLObject, add
"see the comment in WebGLObject::detachAndDeleteObject()".

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLContextObject.cpp
File Source/modules/webgl/WebGLContextObject.cpp (right):

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLContextObject.cpp#newcode44
Source/modules/webgl/WebGLContextObject.cpp:44:
m_context->removeContextObject(this);

You need to remove this because the removal is now handled by the weak
processing.

Make sure that you take care of all #if ENABLE(OILPAN)s and #if
!ENABLE(OILPAN)s in modules/webgl/ in this CL.

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLFramebuffer.cpp
File Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLFramebuffer.cpp#newcode590
Source/modules/webgl/WebGLFramebuffer.cpp:590:
attachment.value->onDetached(context3d);

I guess this is an existing bug of oilpan builds. onDetached()s for the
attachments need to be called if the deleteObjectImpl() is called from
other than WebGLFramebuffer's destructor.

Specifically, we need to change the code to:

if (m_attachments) {
for (const auto& attachment : m_attachments)
attachment.value->onDetached(context3d);
}

without having the #if !ENABLE(OILPAN). And we need to clear
m_attachments in WebGLFramebuffer's destructor.

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLProgram.cpp
File Source/modules/webgl/WebGLProgram.cpp (right):

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLProgram.cpp#newcode51
Source/modules/webgl/WebGLProgram.cpp:51: #if ENABLE(OILPAN)

You need to drop "#if ENABLE(OILPAN)", because m_vertexShader and
m_fragmentShader need to be cleared in both oilpan and non-oilpan.

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLRenderbuffer.cpp
File Source/modules/webgl/WebGLRenderbuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLRenderbuffer.cpp#newcode48
Source/modules/webgl/WebGLRenderbuffer.cpp:48: // objects.

Let's update the comment to "see the comment in
WebGLObject::detachAndDeleteObject()". Ditto for other places that call
detachAndDeleteObject().

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode71
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:71:
dispatchDetached(context3d);

You need the if(m_destructionInProgress) check.

https://codereview.chromium.org/1234883002/

pe...@chromium.org

unread,

Aug 6, 2015, 1:47:00 AM8/6/15

to oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

PTL.
Layout tests should be passed with the change in
WebGLRenderingContextBase.h.

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLBuffer.cpp
File Source/modules/webgl/WebGLBuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLBuffer.cpp#newcode53
Source/modules/webgl/WebGLBuffer.cpp:53: // object is indeed deleted.

On 2015/08/05 04:12:19, haraken wrote:

> Slightly clearer:

> There is no guarantee about the order between WebGLContextGroup's
destruction
> and WebGLBuffer's destruction. So we cannot rely on the
WebGLContextGroup's
> destructor for deleting the platform objects of the WebGLBuffers.
Hence we
> delete the platform objects in the WebGLBuffer's destructor.

> Also it would be better to move this comment to
> WebGLObject::detachAndDeleteObject().

> - Have the comment in WebGLObject::detachAndDeleteObject().
> - In each destructor of an object that inherits from WebGLObject, add
"see the
> comment in WebGLObject::detachAndDeleteObject()".

Done.

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLContextObject.cpp
File Source/modules/webgl/WebGLContextObject.cpp (right):

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLContextObject.cpp#newcode44
Source/modules/webgl/WebGLContextObject.cpp:44:
m_context->removeContextObject(this);

On 2015/08/05 04:12:19, haraken wrote:

> You need to remove this because the removal is now handled by the weak
> processing.

> Make sure that you take care of all #if ENABLE(OILPAN)s and #if
!ENABLE(OILPAN)s
> in modules/webgl/ in this CL.

Done.

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLFramebuffer.cpp
File Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLFramebuffer.cpp#newcode590
Source/modules/webgl/WebGLFramebuffer.cpp:590:
attachment.value->onDetached(context3d);

On 2015/08/05 04:12:19, haraken wrote:

> I guess this is an existing bug of oilpan builds. onDetached()s for
the
> attachments need to be called if the deleteObjectImpl() is called from
other
> than WebGLFramebuffer's destructor.

> Specifically, we need to change the code to:

> if (m_attachments) {
> for (const auto& attachment : m_attachments)
> attachment.value->onDetached(context3d);
> }

> without having the #if !ENABLE(OILPAN). And we need to clear
m_attachments in
> WebGLFramebuffer's destructor.

Done.

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLProgram.cpp
File Source/modules/webgl/WebGLProgram.cpp (right):

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLProgram.cpp#newcode51
Source/modules/webgl/WebGLProgram.cpp:51: #if ENABLE(OILPAN)

On 2015/08/05 04:12:19, haraken wrote:

> You need to drop "#if ENABLE(OILPAN)", because m_vertexShader and
> m_fragmentShader need to be cleared in both oilpan and non-oilpan.

Done.

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLRenderbuffer.cpp
File Source/modules/webgl/WebGLRenderbuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLRenderbuffer.cpp#newcode48
Source/modules/webgl/WebGLRenderbuffer.cpp:48: // objects.

On 2015/08/05 04:12:19, haraken wrote:

> Let's update the comment to "see the comment in
> WebGLObject::detachAndDeleteObject()". Ditto for other places that
call
> detachAndDeleteObject().

Done.

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/140001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode71
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:71:
dispatchDetached(context3d);

On 2015/08/05 04:12:19, haraken wrote:

> You need the if(m_destructionInProgress) check.

Done.

https://codereview.chromium.org/1234883002/

sigb...@opera.com

unread,

Aug 6, 2015, 2:46:30 AM8/6/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLFramebuffer.cpp
File Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLFramebuffer.cpp#newcode343
Source/modules/webgl/WebGLFramebuffer.cpp:343: m_attachments.clear();
Could you add a comment next to this explaining purpose?

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode655
Source/modules/webgl/WebGLRenderingContextBase.h:655: //
WebGLRenderingContextBase becomes a GCed class.
Isn't this always handled by the (eager) finalizer for
WebGLRenderingContextBase already? I don't understand why a change is
needed.

https://codereview.chromium.org/1234883002/

sigb...@opera.com

unread,

Aug 6, 2015, 3:38:39 AM8/6/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode655
Source/modules/webgl/WebGLRenderingContextBase.h:655: //
WebGLRenderingContextBase becomes a GCed class.

On 2015/08/06 06:46:30, sof wrote:
> Isn't this always handled by the (eager) finalizer for
WebGLRenderingContextBase
> already? I don't understand why a change is needed.

If you do need eager finalization in some form here, that annotation
needs to be moved up to ExtensionTracker. But I still don't understand
its purpose.

https://codereview.chromium.org/1234883002/

pe...@chromium.org

unread,

Aug 6, 2015, 3:38:47 AM8/6/15

to oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLFramebuffer.cpp
File Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLFramebuffer.cpp#newcode343
Source/modules/webgl/WebGLFramebuffer.cpp:343: m_attachments.clear();

On 2015/08/06 06:46:30, sof wrote:

> Could you add a comment next to this explaining purpose?

Done.

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode655
Source/modules/webgl/WebGLRenderingContextBase.h:655: //
WebGLRenderingContextBase becomes a GCed class.
On 2015/08/06 06:46:30, sof wrote:
> Isn't this always handled by the (eager) finalizer for
WebGLRenderingContextBase
> already? I don't understand why a change is needed.

On non-Oilpan build, WebGLRendingContextBase's finalizer (dtor)
does not handle lifetime of this class, though it releases
persistent pointers to this class.

https://codereview.chromium.org/1234883002/

sigb...@opera.com

unread,

Aug 6, 2015, 3:43:30 AM8/6/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode655
Source/modules/webgl/WebGLRenderingContextBase.h:655: //
WebGLRenderingContextBase becomes a GCed class.

On 2015/08/06 07:38:46, peria wrote:
> On 2015/08/06 06:46:30, sof wrote:
> > Isn't this always handled by the (eager) finalizer for
> WebGLRenderingContextBase
> > already? I don't understand why a change is needed.

> On non-Oilpan build, WebGLRendingContextBase's finalizer (dtor)
> does not handle lifetime of this class, though it releases
> persistent pointers to this class.

If so, then I think it would be preferable to explicitly dispose the
extensions from its dtor.

https://codereview.chromium.org/1234883002/

pe...@chromium.org

unread,

Aug 6, 2015, 4:04:23 AM8/6/15

to oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/160001/Source/modules/webgl/WebGLRenderingContextBase.h#newcode655
Source/modules/webgl/WebGLRenderingContextBase.h:655: //
WebGLRenderingContextBase becomes a GCed class.

On 2015/08/06 07:43:29, sof wrote:
> On 2015/08/06 07:38:46, peria wrote:
> > On 2015/08/06 06:46:30, sof wrote:
> > > Isn't this always handled by the (eager) finalizer for
> > WebGLRenderingContextBase
> > > already? I don't understand why a change is needed.
> >
> > On non-Oilpan build, WebGLRendingContextBase's finalizer (dtor)
> > does not handle lifetime of this class, though it releases
> > persistent pointers to this class.

> If so, then I think it would be preferable to explicitly dispose the
extensions
> from its dtor.

SGTM!
Done.

https://codereview.chromium.org/1234883002/

sigb...@opera.com

unread,

Aug 6, 2015, 4:13:14 AM8/6/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

lgtm % bots cycling green.

https://codereview.chromium.org/1234883002/

pe...@chromium.org

unread,

Aug 6, 2015, 4:57:11 AM8/6/15

to oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

haraken@
it seems that layout tests are fixed with PS11.
PTAL.

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 6, 2015, 10:01:07 AM8/6/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

On 2015/08/06 08:57:11, peria wrote:
> haraken@
> it seems that layout tests are fixed with PS11.
> PTAL.

I'll take a final look. As kbr@ requested, would you run the chromium bots
to
confirm that there is no behavioral flakiness?

https://codereview.chromium.org/1234883002/

k...@chromium.org

unread,

Aug 7, 2015, 12:42:23 AM8/7/15

to pe...@chromium.org, oilpan-...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

bajones, zmo: would one of you please review this patch? Thanks.

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 7, 2015, 3:43:59 AM8/7/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

LGTM!

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp
File Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp#newcode589
Source/modules/webgl/WebGLFramebuffer.cpp:589: if
(!m_attachments.isEmpty()) {

This check won't be needed, since if the m_attachements is empty, the
following loop does nothing.

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (left):

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLRenderingContextBase.h#oldcode662
Source/modules/webgl/WebGLRenderingContextBase.h:662: {

Maybe can we add:

ASSERT(!m_extension || m_extension->isLost());

to the destructor to make sure that the extension has been lost until we
reach here?

https://codereview.chromium.org/1234883002/

baj...@chromium.org

unread,

Aug 7, 2015, 4:32:17 PM8/7/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

I have no tears to shed for the loss of WillBe types here. :) LGTM %
haraken@'s
comments and one nit.

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode67
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:67: // so they could
have been already finalized.
Minor grammatical nit: This should probably read "since they could
have.."

https://codereview.chromium.org/1234883002/

sigb...@opera.com

unread,

Aug 9, 2015, 4:35:21 AM8/9/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

Looking closer at the change introduced in WebGLFramebuffer..some comments.

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp
File Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp#newcode345
Source/modules/webgl/WebGLFramebuffer.cpp:345: m_attachments.clear();
I don't think is quite correct with Oilpan:

- if the context is swept at the same time, it will take care of
detaching WebGLFramebuffer. Hence, this map will be empty & the dtor a
no-op.
- if the context isn't swept at the same time, we will clear
m_attachments, causing onDetached() not to be called for its elements.

So, there are no guarantees that onDetached() will always be called for
its WebGLAttachments with Oilpan on destruction. I think that implies
that they need to handle detachment on their own (I believe they do
already.) And if they do, the clearing here isn't needed and the
iteration over them in deleteObjectImpl() can be removed.

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp#newcode582
Source/modules/webgl/WebGLFramebuffer.cpp:582: // Both the AttachmentMap
and its WebGLAttachment objects are GCed
The code and the comment is inconsistent now; do these really need to be
notified of onDetached() with Oilpan?

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 9, 2015, 10:16:46 AM8/9/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp
File Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp#newcode345
Source/modules/webgl/WebGLFramebuffer.cpp:345: m_attachments.clear();

On 2015/08/09 08:35:21, sof wrote:
> I don't think is quite correct with Oilpan:

> - if the context is swept at the same time, it will take care of
detaching
> WebGLFramebuffer. Hence, this map will be empty & the dtor a no-op.
> - if the context isn't swept at the same time, we will clear
m_attachments,
> causing onDetached() not to be called for its elements.

> So, there are no guarantees that onDetached() will always be called
for its
> WebGLAttachments with Oilpan on destruction. I think that implies that
they need
> to handle detachment on their own (I believe they do already.) And if
they do,
> the clearing here isn't needed and the iteration over them in
deleteObjectImpl()
> can be removed.

Yeah, you're right -- I think we can just remove the onDetach call
(though I'd prefer doing that in a follow-up CL since this CL is already
complex).

https://codereview.chromium.org/1234883002/

sigb...@opera.com

unread,

Aug 9, 2015, 11:07:38 AM8/9/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp
File Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp#newcode345
Source/modules/webgl/WebGLFramebuffer.cpp:345: m_attachments.clear();

On 2015/08/09 14:16:45, haraken wrote:
> On 2015/08/09 08:35:21, sof wrote:
> > I don't think is quite correct with Oilpan:
> >
> > - if the context is swept at the same time, it will take care of
detaching
> > WebGLFramebuffer. Hence, this map will be empty & the dtor a no-op.
> > - if the context isn't swept at the same time, we will clear
m_attachments,
> > causing onDetached() not to be called for its elements.
> >
> > So, there are no guarantees that onDetached() will always be called
for its
> > WebGLAttachments with Oilpan on destruction. I think that implies
that they
> need
> > to handle detachment on their own (I believe they do already.) And
if they do,
> > the clearing here isn't needed and the iteration over them in
> deleteObjectImpl()
> > can be removed.

> Yeah, you're right -- I think we can just remove the onDetach call
(though I'd
> prefer doing that in a follow-up CL since this CL is already complex).

That's fine, I think. The Oilpan changes made here do no harm, but are
not needed.

https://codereview.chromium.org/1234883002/

pe...@chromium.org

unread,

Aug 9, 2015, 11:15:23 PM8/9/15

to oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp
File Source/modules/webgl/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp#newcode345
Source/modules/webgl/WebGLFramebuffer.cpp:345: m_attachments.clear();

Thanks, I filed an issue to follow it up.
http://crbug.com/518706

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp#newcode582
Source/modules/webgl/WebGLFramebuffer.cpp:582: // Both the AttachmentMap
and its WebGLAttachment objects are GCed

On 2015/08/09 08:35:20, sof wrote:
> The code and the comment is inconsistent now; do these really need to
be
> notified of onDetached() with Oilpan?

Removed latter half comment.
I think it is not needed to call onDetached(), but I will work for it in
a follow-up CL.

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLFramebuffer.cpp#newcode589
Source/modules/webgl/WebGLFramebuffer.cpp:589: if
(!m_attachments.isEmpty()) {

On 2015/08/07 07:43:58, haraken wrote:

> This check won't be needed, since if the m_attachements is empty, the
following
> loop does nothing.

Done.

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLRenderingContextBase.h
File Source/modules/webgl/WebGLRenderingContextBase.h (left):

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLRenderingContextBase.h#oldcode662
Source/modules/webgl/WebGLRenderingContextBase.h:662: {

On 2015/08/07 07:43:58, haraken wrote:

> Maybe can we add:

> ASSERT(!m_extension || m_extension->isLost());

> to the destructor to make sure that the extension has been lost until
we reach
> here?

We can, but it needs EAGER_FINALIZE() to access |m_extension|.

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp#newcode67
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:67: // so they could
have been already finalized.

On 2015/08/07 20:32:17, bajones wrote:
> Minor grammatical nit: This should probably read "since they could
have.."

Done.

https://codereview.chromium.org/1234883002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Aug 11, 2015, 12:09:38 AM8/11/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, commi...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

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

https://codereview.chromium.org/1234883002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Aug 11, 2015, 1:16:23 AM8/11/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, commi...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

Committed patchset #13 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200298

https://codereview.chromium.org/1234883002/

ha...@chromium.org

unread,

Aug 11, 2015, 11:36:43 AM8/11/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

On 2015/08/11 05:16:22, commit-bot: I haz the power wrote:
> Committed patchset #13 (id:240001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=200298

The Win/Clang buildbots went red when this rolled in, e.g.
http://build.chromium.org/p/chromium.fyi/builders/ClangToTWin/builds/2402:

FAILED: ninja -t msvc -e environment.x86 --
"..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m32 /nologo
/showIncludes /FC
@obj\third_party\WebKit\Source\modules\webgl\modules_1.WebGL2RenderingContext.obj.rsp
/c ..\..\third_party\WebKit\Source\modules\webgl\WebGL2RenderingContext.cpp
/Foobj\third_party\WebKit\Source\modules\webgl\modules_1.WebGL2RenderingContext.obj
/Fdobj\third_party\WebKit\Source\modules\modules_1.cc.pdb
In file included from
..\..\third_party\WebKit\Source\modules\webgl\WebGL2RenderingContext.cpp:6:
In file included from
..\..\third_party\WebKit\Source\modules/webgl/WebGL2RenderingContext.h:9:
In file included from
..\..\third_party\WebKit\Source\modules/webgl/WebGL2RenderingContextBase.h:8:
In file included from
..\..\third_party\WebKit\Source\modules/webgl/WebGLExtension.h:32:
..\..\third_party\WebKit\Source\modules/webgl/WebGLRenderingContextBase.h(647,5)
: error: [blink-gc] Class
'TypedExtensionTracker<blink::CHROMIUMSubscribeUniform>' contains GC root in
field 'm_extensionField'.
class TypedExtensionTracker final : public ExtensionTracker {
^
..\..\third_party\WebKit\Source\modules/webgl/WebGLRenderingContextBase.h(696,9)
: note: [blink-gc] Field 'm_extensionField' defining a GC root declared
here:
PersistentWillBeMember<T>& m_extensionField;
^
..\..\third_party\WebKit\Source\platform/heap/Handle.h(806,32) : note:
expanded
from macro 'PersistentWillBeMember'
#define PersistentWillBeMember blink::Persistent
^

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 11, 2015, 11:39:11 AM8/11/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

Sorry for the breakage; I'll fix it shortly.

https://codereview.chromium.org/1234883002/

tha...@chromium.org

unread,

Aug 11, 2015, 11:43:20 AM8/11/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

Please put this new diagnostic behind a flag, it looks like it will make our
next clang roll much harder if the code base regresses so often.

https://codereview.chromium.org/1234883002/

ha...@chromium.org

unread,

Aug 11, 2015, 11:43:30 AM8/11/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/1281953003/ by ha...@chromium.org.

The reason for reverting is: This caused blink-gc plugin errors on all
Win/Clang
builders. See e.g.
http://build.chromium.org/p/chromium.fyi/builders/ClangToTWin/builds/2402.

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 11, 2015, 11:47:02 AM8/11/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

The plugin is detecting a real bug (although in this case, the current code
is
safe by accident for a complicated reason).

I'm wondering why the try bots cannot detect the bug before landing. Maybe
the
gc plugin is not enabled on the try bots?

https://codereview.chromium.org/1234883002/

tha...@chromium.org

unread,

Aug 11, 2015, 11:57:52 AM8/11/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

It's a new diagnostic that was added in
https://codereview.chromium.org/1274403002/ from what I understand. Bots
other
than the clang ToT bots don't have it yet. The next clang roll will bring
it in,
which is why this will make the next clang roll harder if there isn't a
flag to
toggle this warning. With a flag, we can turn off the warning, land the
roll,
and then turn the warning on after the roll stuck.

https://codereview.chromium.org/1234883002/

har...@chromium.org

unread,

Aug 11, 2015, 12:09:45 PM8/11/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

I haven't yet investigated but I don't think
https://codereview.chromium.org/1274403002/ is related to the breakage. My
guess
is that the plugin is not enabled on the try bots.

https://codereview.chromium.org/1234883002/

tha...@chromium.org

unread,

Aug 11, 2015, 1:39:25 PM8/11/15

to pe...@chromium.org, oilpan-...@chromium.org, k...@chromium.org, baj...@chromium.org, ju...@chromium.org, z...@chromium.org, sigb...@opera.com, har...@chromium.org, blink-...@chromium.org, viv...@chromium.org, blink-revie...@chromium.org, vive...@samsung.com

> I haven't yet investigated but I don't think
> https://codereview.chromium.org/1274403002/ is related to the breakage. My
guess
> is that the plugin is not enabled on the try bots.

The plugin is enabled on the trybots.

Oh, I guess this is a Windows-only problem. We don't have any clang/win
trybots.
But the pinned win/clang bots look happy, only the ToT bots are red. So this
might still require some new plugin change.

In any case, I added a section the the clang plugins wiki page to recommend
landing new diags behind a flag:
https://code.google.com/p/chromium/wiki/WritingClangPlugins#Modifying_existing_plugins

https://codereview.chromium.org/1234883002/

Reply all

Reply to author

Forward