Per the section of the WebGL spec on Resource Restrictions (currently Section 4.2), the contents of renderbuffers must be initialized before their first use, so that garbage can not be read back from them. To implement this, when a renderbuffer is first attached to a complete FBO, or when an FBO becomes complete and it has uninitialized renderbuffers attached, the appropriate planes of the FBO must be cleared.
Created attachment 56665 [details] patch
Comment on attachment 56665 [details] patch This looks good overall. A few comments, one affecting correctness. WebCore/html/canvas/WebGLFramebuffer.cpp:83 + // lifespan of CanvanObject changes. CanvanObject -> CanvasObject WebCore/html/canvas/WebGLFramebuffer.cpp:196 + } This can be written more concisely: if (initDepth && initStencil && m_depthStencilAttachment) { (reinterpret_cast<WebGLRenderbuffer*>(m_depthStencilAttachment))->setInitialized(); } else { if (initDepth) (reinterpret_cast<WebGLRenderbuffer*>(m_depthAttachment))->setInitialized(); if (initStencil) (reinterpret_cast<WebGLRenderbuffer*>(m_stencilAttachment))->setInitialized(); } WebCore/html/canvas/WebGLFramebuffer.h:73 + CanvasObject* m_depthStencilAttachment; These need to be RefPtr<CanvasObject*>, or they can refer to deleted renderbuffers.
(In reply to comment #2) > (From update of attachment 56665 [details]) > This looks good overall. A few comments, one affecting correctness. > > WebCore/html/canvas/WebGLFramebuffer.cpp:83 > + // lifespan of CanvanObject changes. > CanvanObject -> CanvasObject > > > WebCore/html/canvas/WebGLFramebuffer.cpp:196 > + } > This can be written more concisely: > > if (initDepth && initStencil && m_depthStencilAttachment) { > (reinterpret_cast<WebGLRenderbuffer*>(m_depthStencilAttachment))->setInitialized(); > } else { > if (initDepth) > (reinterpret_cast<WebGLRenderbuffer*>(m_depthAttachment))->setInitialized(); > if (initStencil) > (reinterpret_cast<WebGLRenderbuffer*>(m_stencilAttachment))->setInitialized(); > } > > > WebCore/html/canvas/WebGLFramebuffer.h:73 > + CanvasObject* m_depthStencilAttachment; > These need to be RefPtr<CanvasObject*>, or they can refer to deleted renderbuffers. In the logic we check if attachment->object()==0, which guard again referencing deleted CanvasObjects.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 56665 [details] [details]) > > This looks good overall. A few comments, one affecting correctness. > > > > WebCore/html/canvas/WebGLFramebuffer.cpp:83 > > + // lifespan of CanvanObject changes. > > CanvanObject -> CanvasObject > > > > > > WebCore/html/canvas/WebGLFramebuffer.cpp:196 > > + } > > This can be written more concisely: > > > > if (initDepth && initStencil && m_depthStencilAttachment) { > > (reinterpret_cast<WebGLRenderbuffer*>(m_depthStencilAttachment))->setInitialized(); > > } else { > > if (initDepth) > > (reinterpret_cast<WebGLRenderbuffer*>(m_depthAttachment))->setInitialized(); > > if (initStencil) > > (reinterpret_cast<WebGLRenderbuffer*>(m_stencilAttachment))->setInitialized(); > > } > > > > > > WebCore/html/canvas/WebGLFramebuffer.h:73 > > + CanvasObject* m_depthStencilAttachment; > > These need to be RefPtr<CanvasObject*>, or they can refer to deleted renderbuffers. > > In the logic we check if attachment->object()==0, which guard again referencing deleted CanvasObjects. I don't believe this is sufficient. Consider this sequence of operations: 1. A renderbuffer is attached to a framebuffer. A pointer to the WebGLRenderbuffer is stored in the WebGLFramebuffer object. For the purposes of example, say that the framebuffer is still incomplete at this point. 2. All JavaScript references to the renderbuffer, but not the framebuffer, are dropped. The WebGLRenderbuffer's reference count goes to 0 and the C++ object is deleted. The WebGLFramebuffer now points to deleted storage. 3. A different renderbuffer is attached to the WebGLFramebuffer. The old, deleted pointer, is queried.
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 56665 [details] [details] [details]) > > > This looks good overall. A few comments, one affecting correctness. > > > > > > WebCore/html/canvas/WebGLFramebuffer.cpp:83 > > > + // lifespan of CanvanObject changes. > > > CanvanObject -> CanvasObject > > > > > > > > > WebCore/html/canvas/WebGLFramebuffer.cpp:196 > > > + } > > > This can be written more concisely: > > > > > > if (initDepth && initStencil && m_depthStencilAttachment) { > > > (reinterpret_cast<WebGLRenderbuffer*>(m_depthStencilAttachment))->setInitialized(); > > > } else { > > > if (initDepth) > > > (reinterpret_cast<WebGLRenderbuffer*>(m_depthAttachment))->setInitialized(); > > > if (initStencil) > > > (reinterpret_cast<WebGLRenderbuffer*>(m_stencilAttachment))->setInitialized(); > > > } > > > > > > > > > WebCore/html/canvas/WebGLFramebuffer.h:73 > > > + CanvasObject* m_depthStencilAttachment; > > > These need to be RefPtr<CanvasObject*>, or they can refer to deleted renderbuffers. > > > > In the logic we check if attachment->object()==0, which guard again referencing deleted CanvasObjects. > > I don't believe this is sufficient. Consider this sequence of operations: > > 1. A renderbuffer is attached to a framebuffer. A pointer to the WebGLRenderbuffer is stored in the WebGLFramebuffer object. For the purposes of example, say that the framebuffer is still incomplete at this point. > 2. All JavaScript references to the renderbuffer, but not the framebuffer, are dropped. The WebGLRenderbuffer's reference count goes to 0 and the C++ object is deleted. The WebGLFramebuffer now points to deleted storage. > 3. A different renderbuffer is attached to the WebGLFramebuffer. The old, deleted pointer, is queried. That's correct. But once an CanvasObject is created, a RefPtr is added in the object list in WebGLRenderingContext, and is never deleted until the destruction of WebGLRenderingContext. So in this case, we don't need to worry about the renference count goes to 0.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > (From update of attachment 56665 [details] [details] [details] [details]) > > > > This looks good overall. A few comments, one affecting correctness. > > > > > > > > WebCore/html/canvas/WebGLFramebuffer.cpp:83 > > > > + // lifespan of CanvanObject changes. > > > > CanvanObject -> CanvasObject > > > > > > > > > > > > WebCore/html/canvas/WebGLFramebuffer.cpp:196 > > > > + } > > > > This can be written more concisely: > > > > > > > > if (initDepth && initStencil && m_depthStencilAttachment) { > > > > (reinterpret_cast<WebGLRenderbuffer*>(m_depthStencilAttachment))->setInitialized(); > > > > } else { > > > > if (initDepth) > > > > (reinterpret_cast<WebGLRenderbuffer*>(m_depthAttachment))->setInitialized(); > > > > if (initStencil) > > > > (reinterpret_cast<WebGLRenderbuffer*>(m_stencilAttachment))->setInitialized(); > > > > } > > > > > > > > > > > > WebCore/html/canvas/WebGLFramebuffer.h:73 > > > > + CanvasObject* m_depthStencilAttachment; > > > > These need to be RefPtr<CanvasObject*>, or they can refer to deleted renderbuffers. > > > > > > In the logic we check if attachment->object()==0, which guard again referencing deleted CanvasObjects. > > > > I don't believe this is sufficient. Consider this sequence of operations: > > > > 1. A renderbuffer is attached to a framebuffer. A pointer to the WebGLRenderbuffer is stored in the WebGLFramebuffer object. For the purposes of example, say that the framebuffer is still incomplete at this point. > > 2. All JavaScript references to the renderbuffer, but not the framebuffer, are dropped. The WebGLRenderbuffer's reference count goes to 0 and the C++ object is deleted. The WebGLFramebuffer now points to deleted storage. > > 3. A different renderbuffer is attached to the WebGLFramebuffer. The old, deleted pointer, is queried. > > That's correct. But once an CanvasObject is created, a RefPtr is added in the object list in WebGLRenderingContext, and is never deleted until the destruction of WebGLRenderingContext. So in this case, we don't need to worry about the renference count goes to 0. Right, I'd forgotten about that object list. Could you instead add a comment above these fields in WebGLFramebuffer indicating that these objects are kept alive by the global table in WebGLRenderingContext?
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (In reply to comment #3) > > > > (In reply to comment #2) > > > > > (From update of attachment 56665 [details] [details] [details] [details] [details]) > > > > > This looks good overall. A few comments, one affecting correctness. > > > > > > > > > > WebCore/html/canvas/WebGLFramebuffer.cpp:83 > > > > > + // lifespan of CanvanObject changes. > > > > > CanvanObject -> CanvasObject > > > > > > > > > > > > > > > WebCore/html/canvas/WebGLFramebuffer.cpp:196 > > > > > + } > > > > > This can be written more concisely: > > > > > > > > > > if (initDepth && initStencil && m_depthStencilAttachment) { > > > > > (reinterpret_cast<WebGLRenderbuffer*>(m_depthStencilAttachment))->setInitialized(); > > > > > } else { > > > > > if (initDepth) > > > > > (reinterpret_cast<WebGLRenderbuffer*>(m_depthAttachment))->setInitialized(); > > > > > if (initStencil) > > > > > (reinterpret_cast<WebGLRenderbuffer*>(m_stencilAttachment))->setInitialized(); > > > > > } > > > > > > > > > > > > > > > WebCore/html/canvas/WebGLFramebuffer.h:73 > > > > > + CanvasObject* m_depthStencilAttachment; > > > > > These need to be RefPtr<CanvasObject*>, or they can refer to deleted renderbuffers. > > > > > > > > In the logic we check if attachment->object()==0, which guard again referencing deleted CanvasObjects. > > > > > > I don't believe this is sufficient. Consider this sequence of operations: > > > > > > 1. A renderbuffer is attached to a framebuffer. A pointer to the WebGLRenderbuffer is stored in the WebGLFramebuffer object. For the purposes of example, say that the framebuffer is still incomplete at this point. > > > 2. All JavaScript references to the renderbuffer, but not the framebuffer, are dropped. The WebGLRenderbuffer's reference count goes to 0 and the C++ object is deleted. The WebGLFramebuffer now points to deleted storage. > > > 3. A different renderbuffer is attached to the WebGLFramebuffer. The old, deleted pointer, is queried. > > > > That's correct. But once an CanvasObject is created, a RefPtr is added in the object list in WebGLRenderingContext, and is never deleted until the destruction of WebGLRenderingContext. So in this case, we don't need to worry about the renference count goes to 0. > > Right, I'd forgotten about that object list. Could you instead add a comment above these fields in WebGLFramebuffer indicating that these objects are kept alive by the global table in WebGLRenderingContext? Will do.
Created attachment 56944 [details] revised patch : responding to kbr's review
Comment on attachment 56944 [details] revised patch : responding to kbr's review Looks good to me. Nice work, in particular on handling the case where a color attachment becomes valid because of a call to texImage2D or copyTexImage2D.
Comment on attachment 56944 [details] revised patch : responding to kbr's review Just a few typos: WebCore/ChangeLog:5 + Implement lazy clearing of renderbuffers Typo: implement. WebCore/html/canvas/WebGLFramebuffer.cpp:134 + bool isSissorEnabled = false; Typo: Scissor WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp:332 + GLboolean isSissorEnabled = GL_FALSE; Ditto. Another thing -- this is just a clarity suggestion. Would it be more descriptive to define a simple Attachment abstraction, which holds CanvasObject* and manages all casting and validity checking?
(In reply to comment #10) > (From update of attachment 56944 [details]) > Just a few typos: > > WebCore/ChangeLog:5 > + Implement lazy clearing of renderbuffers > Typo: implement. > The original spelling is correct. > WebCore/html/canvas/WebGLFramebuffer.cpp:134 > + bool isSissorEnabled = false; > Typo: Scissor > Fixed. > WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp:332 > + GLboolean isSissorEnabled = GL_FALSE; > Ditto. > Fixed. > > Another thing -- this is just a clarity suggestion. Would it be more descriptive to define a simple Attachment abstraction, which holds CanvasObject* and manages all casting and validity checking? I added two helper functions to deal with casting and validity checking. Now the code is much cleaner. Thanks for the suggestion.
Created attachment 57029 [details] revised patch : responding to Dimitri Glazkov's review
Comment on attachment 57029 [details] revised patch : responding to Dimitri Glazkov's review ok. isInitialized/isUninitialized could've been just local functions, but this works too.
Comment on attachment 57029 [details] revised patch : responding to Dimitri Glazkov's review Clearing flags on attachment: 57029 Committed r60290: <http://trac.webkit.org/changeset/60290>
All reviewed patches have been landed. Closing bug.