[webkit-reviews] review denied: [Bug 96578] Add support for OES_vertex_array_object in chromium : [Attachment 164238] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Brandon Jones
<bajones at chromium.org>'s request for review:
Bug 96578: Add support for OES_vertex_array_object in chromium
https://bugs.webkit.org/show_bug.cgi?id=96578

Attachment 164238: Patch
https://bugs.webkit.org/attachment.cgi?id=164238&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
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.