[Webkit-unassigned] [Bug 96578] Add support for OES_vertex_array_object in chromium

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=96578


Kenneth Russell <kbr at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #164238|review?                     |review-
               Flag|                            |




--- Comment #6 from Kenneth Russell <kbr at google.com>  2012-09-14 19:19:39 PST ---
(From update of attachment 164238)
View in context: https://bugs.webkit.org/attachment.cgi?id=164238&action=review

This is good work so far but I'm concerned that there is a resource leak introduced with this patch; r-'ing it for that reason rather than leaving the r? bit alone. If it can be proven (and hopefully tested) that I'm wrong about this resource leak and I'm not available to re-review it (traveling on business for two days next week), dino could be a substitute reviewer. gman, benvanik, or zmo could be unofficial reviewers.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4398
> +    m_boundArrayBuffer->onAttached();

It looks to me like these newly introduced onAttached/onDetached calls are unbalanced and will prevent buffer objects from being deleted. In particular, consider the case where a VAO is allocated and bound, a vertexAttribPointer call using it is made, the VAO is unbound, the buffer object is deleted, and the VAO is deleted. My reading of the code is that the buffer object's m_attachmentCount will still be 1 at this point so the deletion will be deferred and never executed. There are missing calls to onDetached() in WebGLVertexArrayObjectOES::deleteObjectImpl. Note that WebGLFramebuffer, which is another type of container object, calls onDetached on all of its attachments in WebGLFramebuffer::deleteObjectImpl.

I hope that it is possible to add a test for this scenario using isBuffer(). After the buffer object is deleted but the VAO is still alive, isBuffer() might still return true -- not sure -- please test this. runDeleteTests() in the new layout test almost tests this, but I would specifically add a new test for it.

One way or another, these onAttached and onDetached calls should be encapsulated within the WebGLVertexArrayObjectOES class or its nested VertexAttribState struct, so that there aren't two distant places in the code which are calling attach and detach.

We should really come up with a better way to manage these lazy deletions via some sort of reference counted pointer (maybe a simple wrapper to RefPtr) but that should wait for another patch.

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.cpp:83
> +        buffer->onAttached();

As far as I can tell, this introduces the same resource leak described above for element array buffers.

> LayoutTests/fast/canvas/webgl/oes-vertex-array-object.html:124
> +    if (gl.getParameter(ext.VERTEX_ARRAY_BINDING_OES) == vao0) {

See comment below about equality comparisons on these kinds of objects.

> LayoutTests/fast/canvas/webgl/oes-vertex-array-object.html:220
> +            if (buffer == state.buffer) {

I am not sure these kinds of object equality comparisons on JavaScript wrapper objects for C++ objects are guaranteed to work, at least not in all JavaScript engines. There was a long discussion about them at one point on the public_webgl mailing list. It would be safer to add an attribute you know about to the buffer objects you create, and do the comparisons on the value of that attribute.

> LayoutTests/fast/canvas/webgl/oes-vertex-array-object.html:246
> +            if (elbuffer == state.elbuffer) {

Same comment about equality comparisons.

> LayoutTests/platform/chromium/TestExpectations:3610
> +BUGWK96578 : platform/chromium/virtual/gpu/fast/canvas/webgl/oes-vertex-array-object.html = PASS TEXT

Please file a new bug referencing this one indicating that these lines should be removed once the Chromium implementation of this extension lands, and use that bug's ID here.

> LayoutTests/platform/efl/Skipped:882
> +# http://webkit.org/b/96828

Please file one or more new bugs referencing this one indicating that this test should be unskipped when the extension is supported on this platform, and reference that bug ID here and in the other ports' Skipped lists.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.