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

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+442 lines, -608 lines)			Patch
M	Source/bindings/modules/v8/WebGLAny.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/modules/v8/WebGLAny.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/ANGLEInstancedArrays.h	View	1 2 3 4 5 6	2 chunks	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/ANGLEInstancedArrays.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/ANGLEInstancedArrays.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/CHROMIUMSubscribeUniform.h	View	1 2 3 4 5 6	2 chunks	+2 lines, -3 lines	0 comments	Download
M	Source/modules/webgl/CHROMIUMSubscribeUniform.cpp	View		2 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/modules/webgl/CHROMIUMSubscribeUniform.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/CHROMIUMValuebuffer.h	View	1 2 3 4 5 6	2 chunks	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/CHROMIUMValuebuffer.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+3 lines, -9 lines	0 comments	Download
M	Source/modules/webgl/CHROMIUMValuebuffer.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/EXTBlendMinMax.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/EXTBlendMinMax.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/EXTBlendMinMax.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/EXTFragDepth.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/EXTFragDepth.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/EXTFragDepth.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/EXTShaderTextureLOD.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/EXTShaderTextureLOD.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/EXTShaderTextureLOD.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/EXTTextureFilterAnisotropic.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/EXTTextureFilterAnisotropic.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/EXTTextureFilterAnisotropic.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/EXTsRGB.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/EXTsRGB.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/EXTsRGB.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/OESElementIndexUint.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/OESElementIndexUint.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/OESElementIndexUint.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/OESStandardDerivatives.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/OESStandardDerivatives.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/OESStandardDerivatives.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/OESTextureFloat.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/OESTextureFloat.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/OESTextureFloat.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/OESTextureFloatLinear.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/OESTextureFloatLinear.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/OESTextureFloatLinear.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/OESTextureHalfFloat.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/OESTextureHalfFloat.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/OESTextureHalfFloat.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/OESTextureHalfFloatLinear.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/OESTextureHalfFloatLinear.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/OESTextureHalfFloatLinear.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/OESVertexArrayObject.h	View	1 2 3 4 5 6	2 chunks	+2 lines, -3 lines	0 comments	Download
M	Source/modules/webgl/OESVertexArrayObject.cpp	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	Source/modules/webgl/OESVertexArrayObject.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGL2RenderingContext.h	View		1 chunk	+10 lines, -10 lines	0 comments	Download
M	Source/modules/webgl/WebGL2RenderingContextBase.h	View	1 2 3 4 5 6 7 8 9 10 11	6 chunks	+18 lines, -19 lines	0 comments	Download
M	Source/modules/webgl/WebGL2RenderingContextBase.cpp	View	1 2 3 4 5 6 7 8 9 10 11	13 chunks	+35 lines, -35 lines	0 comments	Download
M	Source/modules/webgl/WebGLActiveInfo.h	View	1 2 3 4 5 6	1 chunk	+3 lines, -6 lines	0 comments	Download
M	Source/modules/webgl/WebGLActiveInfo.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLBuffer.h	View	1 2 3 4 5 6	2 chunks	+1 line, -3 lines	0 comments	Download
M	Source/modules/webgl/WebGLBuffer.cpp	View	1 2 3 4 5 6 7 8	2 chunks	+3 lines, -11 lines	0 comments	Download
M	Source/modules/webgl/WebGLBuffer.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLCompressedTextureATC.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLCompressedTextureATC.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLCompressedTextureATC.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLCompressedTextureETC1.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLCompressedTextureETC1.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLCompressedTextureETC1.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLCompressedTexturePVRTC.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLCompressedTexturePVRTC.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLCompressedTexturePVRTC.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLCompressedTextureS3TC.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLCompressedTextureS3TC.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLCompressedTextureS3TC.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLContextGroup.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/modules/webgl/WebGLContextObject.cpp	View	1 2 3 4 5 6 7 8	2 chunks	+1 line, -5 lines	0 comments	Download
M	Source/modules/webgl/WebGLDebugRendererInfo.h	View	1 2 3 4 5 6	2 chunks	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLDebugRendererInfo.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLDebugRendererInfo.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLDebugShaders.h	View	1 2 3 4 5 6	2 chunks	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLDebugShaders.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLDebugShaders.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLDepthTexture.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLDepthTexture.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLDepthTexture.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLDrawBuffers.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLDrawBuffers.cpp	View		2 chunks	+3 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLDrawBuffers.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLExtension.h	View	1 2 3 4 5 6 7 8	1 chunk	+6 lines, -10 lines	0 comments	Download
M	Source/modules/webgl/WebGLExtension.cpp	View	1 2 3 4 5 6 7	1 chunk	+0 lines, -6 lines	0 comments	Download
M	Source/modules/webgl/WebGLFenceSync.h	View	1 2 3 4 5 6	2 chunks	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLFenceSync.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLFramebuffer.h	View	1 2 3 4 5 6	4 chunks	+3 lines, -5 lines	0 comments	Download
M	Source/modules/webgl/WebGLFramebuffer.cpp	View	1 2 3 4 5 6 7 8 9 10 11	8 chunks	+18 lines, -28 lines	0 comments	Download
M	Source/modules/webgl/WebGLFramebuffer.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLLoseContext.h	View	1 2 3 4 5 6	2 chunks	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLLoseContext.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+4 lines, -3 lines	0 comments	Download
M	Source/modules/webgl/WebGLLoseContext.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLObject.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLObject.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+4 lines, -11 lines	0 comments	Download
M	Source/modules/webgl/WebGLProgram.h	View		2 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/modules/webgl/WebGLProgram.cpp	View	1 2 3 4 5 6 7 8	3 chunks	+6 lines, -13 lines	0 comments	Download
M	Source/modules/webgl/WebGLProgram.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLQuery.h	View	1 2 3 4 5 6	2 chunks	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLQuery.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+3 lines, -9 lines	0 comments	Download
M	Source/modules/webgl/WebGLQuery.idl	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLRenderbuffer.h	View	1 2 3 4 5 6	3 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/modules/webgl/WebGLRenderbuffer.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+4 lines, -11 lines	0 comments	Download
M	Source/modules/webgl/WebGLRenderbuffer.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLRenderingContext.h	View		1 chunk	+23 lines, -23 lines	0 comments	Download
M	Source/modules/webgl/WebGLRenderingContextBase.h	View	1 2 3 4 5 6 7 8 9 10 11	13 chunks	+45 lines, -61 lines	0 comments	Download
M	Source/modules/webgl/WebGLRenderingContextBase.cpp	View	1 2 3 4 5 6 7 8 9 10 11	28 chunks	+63 lines, -63 lines	0 comments	Download
M	Source/modules/webgl/WebGLSampler.h	View	1 2 3 4 5 6	2 chunks	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLSampler.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+3 lines, -9 lines	0 comments	Download
M	Source/modules/webgl/WebGLSampler.idl	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLShader.h	View	1 2 3 4 5 6	2 chunks	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLShader.cpp	View	1 2 3 4 5 6 7 8	2 chunks	+3 lines, -9 lines	0 comments	Download
M	Source/modules/webgl/WebGLShader.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLShaderPrecisionFormat.h	View	1 2 3 4 5 6	1 chunk	+2 lines, -4 lines	0 comments	Download
M	Source/modules/webgl/WebGLShaderPrecisionFormat.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLShaderPrecisionFormat.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLSync.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -7 lines	0 comments	Download
M	Source/modules/webgl/WebGLSync.idl	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLTexture.h	View	1 2 3 4 5 6	2 chunks	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLTexture.cpp	View	1 2 3 4 5 6 7 8	2 chunks	+3 lines, -9 lines	0 comments	Download
M	Source/modules/webgl/WebGLTexture.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLTransformFeedback.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLTransformFeedback.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+3 lines, -9 lines	0 comments	Download
M	Source/modules/webgl/WebGLTransformFeedback.idl	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLUniformLocation.h	View	1 2 3 4 5 6	2 chunks	+3 lines, -5 lines	0 comments	Download
M	Source/modules/webgl/WebGLUniformLocation.cpp	View	1 2 3 4 5 6	2 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/modules/webgl/WebGLUniformLocation.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLVertexArrayObject.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLVertexArrayObject.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLVertexArrayObject.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/webgl/WebGLVertexArrayObjectBase.h	View	1 2 3 4 5 6	5 chunks	+8 lines, -11 lines	0 comments	Download
M	Source/modules/webgl/WebGLVertexArrayObjectBase.cpp	View	1 2 3 4 5 6 7 8 9 10 11	6 chunks	+14 lines, -27 lines	0 comments	Download
M	Source/modules/webgl/WebGLVertexArrayObjectOES.h	View	1 2 3 4 5 6	1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLVertexArrayObjectOES.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/webgl/WebGLVertexArrayObjectOES.idl	View		1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 54 (7 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

peria

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

5 years, 4 months ago (2015-07-30 06:33:55 UTC) #4

haraken

On 2015/07/30 06:33:55, peria wrote: > WebGL guys, > I'm glad if you take a ...

5 years, 4 months ago (2015-07-30 06:55:31 UTC) #5

Ken Russell (switch to Gerrit)

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

5 years, 4 months ago (2015-08-01 00:11:56 UTC) #7

+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/W...
File Source/modules/webgl/WebGL2RenderingContext.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/W...
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/W...
File Source/modules/webgl/WebGLContextGroup.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/W...
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/W...
File Source/modules/webgl/WebGLFramebuffer.cpp (left):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/W...
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/W...
File Source/modules/webgl/WebGLRenderingContextBase.cpp (left):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/W...
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/W...
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/100001/Source/modules/webgl/W...
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.

sof

Apologies for the repetitive (and mostly very boring) review :) [No need to individually tick ...

5 years, 4 months ago (2015-08-01 18:49:14 UTC) #9

peria

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 ...

5 years, 4 months ago (2015-08-03 09:00:37 UTC) #10

sof

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 ...

5 years, 4 months ago (2015-08-03 09:12:05 UTC) #11

haraken

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 ...

5 years, 4 months ago (2015-08-04 00:06:53 UTC) #13

Ken Russell (switch to Gerrit)

sof@, haraken@: thank you both for helping review this CL. I don't understand the nuances ...

5 years, 4 months ago (2015-08-04 00:34:11 UTC) #14

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/W...
File Source/modules/webgl/WebGL2RenderingContext.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
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/W...
File Source/modules/webgl/WebGLContextGroup.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
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/W...
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
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/W...
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
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.

haraken

Thanks kbr! peria-san: It seems we have a bunch of items we need to address ...

5 years, 4 months ago (2015-08-04 00:47:05 UTC) #15

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/W...
File Source/modules/webgl/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
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/W...
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:76:
dispatchDetached(context3d);
On 2015/08/04 00:34:11, Ken Russell wrote:
> 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.

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.)

Ken Russell (switch to Gerrit)

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:47:05, haraken wrote: > On ...

5 years, 4 months ago (2015-08-04 00:54:19 UTC) #16

haraken

On 2015/08/04 00:54:19, Ken Russell wrote: > 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 ...

5 years, 4 months ago (2015-08-04 00:59:13 UTC) #17

On 2015/08/04 00:54:19, Ken Russell wrote:
>
https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
> File Source/modules/webgl/WebGLRenderingContextBase.h (right):
> 
>
https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
> Source/modules/webgl/WebGLRenderingContextBase.h:696:
PersistentWillBeMember<T>&
> m_extensionField;
> On 2015/08/04 00:47:05, haraken wrote:
> > 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.
> 
> 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.

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.

Ken Russell (switch to Gerrit)

On 2015/08/04 00:59:13, haraken wrote: > On 2015/08/04 00:54:19, Ken Russell wrote: > > > ...

5 years, 4 months ago (2015-08-04 01:16:02 UTC) #18

On 2015/08/04 00:59:13, haraken wrote:
> On 2015/08/04 00:54:19, Ken Russell wrote:
> >
>
https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
> > File Source/modules/webgl/WebGLRenderingContextBase.h (right):
> > 
> >
>
https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
> > Source/modules/webgl/WebGLRenderingContextBase.h:696:
> PersistentWillBeMember<T>&
> > m_extensionField;
> > On 2015/08/04 00:47:05, haraken wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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.

haraken

On 2015/08/04 01:16:02, Ken Russell wrote: > On 2015/08/04 00:59:13, haraken wrote: > > On ...

5 years, 4 months ago (2015-08-04 01:22:01 UTC) #19

On 2015/08/04 01:16:02, Ken Russell wrote:
> On 2015/08/04 00:59:13, haraken wrote:
> > On 2015/08/04 00:54:19, Ken Russell wrote:
> > >
> >
>
https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
> > > File Source/modules/webgl/WebGLRenderingContextBase.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
> > > Source/modules/webgl/WebGLRenderingContextBase.h:696:
> > PersistentWillBeMember<T>&
> > > m_extensionField;
> > > On 2015/08/04 00:47:05, haraken wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

Totally agreed :)

sof

We seem to end up rehashing/repeating Oilpan WebGL object details whenever that code is touched; ...

5 years, 4 months ago (2015-08-04 05:11:56 UTC) #20

sof

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

5 years, 4 months ago (2015-08-04 06:20:45 UTC) #21

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/C...
File Source/modules/webgl/CHROMIUMValuebuffer.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/C...
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/W...
File Source/modules/webgl/WebGL2RenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
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/W...
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
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.

haraken

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 ...

5 years, 4 months ago (2015-08-04 06:35:40 UTC) #22

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

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/C...
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/W...
File Source/modules/webgl/WebGL2RenderingContextBase.h (right):

https://codereview.chromium.org/1234883002/diff/120001/Source/modules/webgl/W...
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.

sof

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 ...

5 years, 4 months ago (2015-08-04 06:37:58 UTC) #23

haraken

On 2015/08/04 06:37:58, sof wrote: > 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 > ...

5 years, 4 months ago (2015-08-04 06:42:02 UTC) #24

peria

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: > ...

5 years, 4 months ago (2015-08-04 09:10:26 UTC) #25

haraken

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): ...

5 years, 4 months ago (2015-08-05 04:12:19 UTC) #26

peria

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): ...

5 years, 4 months ago (2015-08-06 05:47:00 UTC) #27

sof

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 ...

5 years, 4 months ago (2015-08-06 06:46:30 UTC) #28

sof

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, ...

5 years, 4 months ago (2015-08-06 07:38:39 UTC) #29

peria

5 years, 4 months ago (2015-08-06 07:38:46 UTC) #30

sof

5 years, 4 months ago (2015-08-06 07:43:30 UTC) #31

peria

5 years, 4 months ago (2015-08-06 08:04:22 UTC) #32

peria

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

5 years, 4 months ago (2015-08-06 08:57:11 UTC) #34

haraken

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

5 years, 4 months ago (2015-08-06 14:01:06 UTC) #35

Ken Russell (switch to Gerrit)

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

5 years, 4 months ago (2015-08-07 04:42:22 UTC) #36

haraken

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, ...

5 years, 4 months ago (2015-08-07 07:43:58 UTC) #37

bajones

I have no tears to shed for the loss of WillBe types here. :) LGTM ...

5 years, 4 months ago (2015-08-07 20:32:17 UTC) #38

sof

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: ...

5 years, 4 months ago (2015-08-09 08:35:21 UTC) #39

haraken

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 ...

5 years, 4 months ago (2015-08-09 14:16:46 UTC) #40

sof

5 years, 4 months ago (2015-08-09 15:07:37 UTC) #41

peria

5 years, 4 months ago (2015-08-10 03:15:23 UTC) #42

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

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/W...
Source/modules/webgl/WebGLFramebuffer.cpp:345: m_attachments.clear();
On 2015/08/09 15:07:37, sof wrote:
> 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.

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

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/W...
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/W...
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/W...
File Source/modules/webgl/WebGLRenderingContextBase.h (left):

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/W...
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/W...
File Source/modules/webgl/WebGLVertexArrayObjectBase.cpp (right):

https://codereview.chromium.org/1234883002/diff/200001/Source/modules/webgl/W...
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.

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-11 04:09:37 UTC) #45

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-11 05:16:22 UTC) #46

hans

On 2015/08/11 05:16:22, commit-bot: I haz the power wrote: > Committed patchset #13 (id:240001) as ...

5 years, 4 months ago (2015-08-11 15:36:43 UTC) #47

haraken

On 2015/08/11 15:36:43, hans wrote: > On 2015/08/11 05:16:22, commit-bot: I haz the power wrote: ...

5 years, 4 months ago (2015-08-11 15:39:11 UTC) #48

Message was sent while issue was closed.

On 2015/08/11 15:36:43, hans wrote:
> 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
>                                ^

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

Nico

On 2015/08/11 15:39:11, haraken wrote: > On 2015/08/11 15:36:43, hans wrote: > > On 2015/08/11 ...

5 years, 4 months ago (2015-08-11 15:43:19 UTC) #49

Message was sent while issue was closed.

On 2015/08/11 15:39:11, haraken wrote:
> On 2015/08/11 15:36:43, hans wrote:
> > 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
> >                                ^
> 
> Sorry for the breakage; I'll fix it shortly.

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.

hans

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

5 years, 4 months ago (2015-08-11 15:43:29 UTC) #50

haraken

On 2015/08/11 15:43:19, Nico (hiding) wrote: > On 2015/08/11 15:39:11, haraken wrote: > > On ...

5 years, 4 months ago (2015-08-11 15:47:02 UTC) #51

Message was sent while issue was closed.

On 2015/08/11 15:43:19, Nico (hiding) wrote:
> On 2015/08/11 15:39:11, haraken wrote:
> > On 2015/08/11 15:36:43, hans wrote:
> > > 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
> > >                                ^
> > 
> > Sorry for the breakage; I'll fix it shortly.
> 
> 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.

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?

Nico

On 2015/08/11 15:47:02, haraken wrote: > On 2015/08/11 15:43:19, Nico (hiding) wrote: > > On ...

5 years, 4 months ago (2015-08-11 15:57:51 UTC) #52

Message was sent while issue was closed.

On 2015/08/11 15:47:02, haraken wrote:
> On 2015/08/11 15:43:19, Nico (hiding) wrote:
> > On 2015/08/11 15:39:11, haraken wrote:
> > > On 2015/08/11 15:36:43, hans wrote:
> > > > 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
> > > >                                ^
> > > 
> > > Sorry for the breakage; I'll fix it shortly.
> > 
> > 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.
> 
> 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?

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.

haraken

On 2015/08/11 15:57:51, Nico (hiding) wrote: > On 2015/08/11 15:47:02, haraken wrote: > > On ...

5 years, 4 months ago (2015-08-11 16:09:45 UTC) #53

Message was sent while issue was closed.

On 2015/08/11 15:57:51, Nico (hiding) wrote:
> On 2015/08/11 15:47:02, haraken wrote:
> > On 2015/08/11 15:43:19, Nico (hiding) wrote:
> > > On 2015/08/11 15:39:11, haraken wrote:
> > > > On 2015/08/11 15:36:43, hans wrote:
> > > > > 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
> > > > >                                ^
> > > > 
> > > > Sorry for the breakage; I'll fix it shortly.
> > > 
> > > 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.
> > 
> > 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?
> 
> 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.

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.

Nico

5 years, 4 months ago (2015-08-11 17:39:24 UTC) #54

Message was sent while issue was closed.

> 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_existin...

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Issue 1234883002: [Oilpan] Migrate classes under module/webgl onto oilpan heap (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fix layout test #

Patch Set 3 : Fix compile #

Patch Set 4 : Rebase #

Patch Set 5 : Rebase #

Patch Set 6 : Rebase #

Patch Set 7 : Work for comments #

Patch Set 8 : Rebase & work for some comments #

Patch Set 9 : Rebase & fix webkit test #

Patch Set 10 : Fix webkit tests #

Patch Set 11 : Detach extensions in dtor of RenderingContextBase #

Patch Set 12 : Rebase and work for comments #

Patch Set 13 : Rebase #

Messages