Improve WebGL object lifetime management in WebGLRenderingContext

[Zhenyao Mo]

At the moment, all objects are held by WebGLRenderingContext even after the underlying gl object is explicitly deleted.

It seems like we don't need to keep this object table (m_canvasObjects) in WebGLRenderingContext.  For all the objects that can be queried (for example, current program), we already hold them besides this object table (for example, m_currentProgram).

Currently the only usage of this object table is that when we query an object, we get an integer from underlying GL, and we go through this table to find the corresponding WebGL object.  If we remove this table, we can just return the held WebGL object instead of querying the underlying GL and then find the WebGLObject matching it.

[Zhenyao Mo]

Created attachment 99321 [details]
Patch

[Zhenyao Mo]

The tests are fixed on the khronos and synced here.

Also, I manually verified that now garbage collection does collect the WebGL objects that are no longer referenced.

Please review.

[Kenneth Russell]

Comment on attachment 99321 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99321&action=review

Nice work; generally looks good except for one cleanup issue in detachAndRemoveAllObjects.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2033
> +        m_context->synthesizeGLError(GraphicsContext3D::INVALID_ENUM);

This is correct, but it might be worth a note here that OpenGL ES 2.0 specifies INVALID_ENUM in this case, while desktop GL specifies INVALID_OPERATION.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2043
> +            return WebGLGetInfo(PassRefPtr<WebGLTexture>(reinterpret_cast<WebGLTexture*>(object)));

I think you should be using adoptRef rather than calling the PassRefPtr constructor explicitly here.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2061
> +            return WebGLGetInfo(PassRefPtr<WebGLRenderbuffer>(reinterpret_cast<WebGLRenderbuffer*>(object)));

Same thing about using adoptRef here.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4001
>      }

This looks very complicated and flaky. If you're worried about WebGLObject::detachContext() removing the object from the table and changing the iteration order, copy the HashSet<WebGLObject*> into a Vector<WebGLObject*> and iterate down the vector.

Also, you might want to consider having WebGLObject::detachContext() remove the object from this table rather than the destructor of WebGLObject. Right now calling detachContext() on the WebGLObject prevents the WebGLObject from removing itself from this table.

[Zhenyao Mo]

(In reply to comment #3)
> (From update of attachment 99321 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99321&action=review
> 
> Nice work; generally looks good except for one cleanup issue in detachAndRemoveAllObjects.
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2033
> > +        m_context->synthesizeGLError(GraphicsContext3D::INVALID_ENUM);
> 
> This is correct, but it might be worth a note here that OpenGL ES 2.0 specifies INVALID_ENUM in this case, while desktop GL specifies INVALID_OPERATION.

Done.

> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2043
> > +            return WebGLGetInfo(PassRefPtr<WebGLTexture>(reinterpret_cast<WebGLTexture*>(object)));
> 
> I think you should be using adoptRef rather than calling the PassRefPtr constructor explicitly here.

There is a difference between adoptRef and explicit constructor.  Explicit constructor will call ref() if the pointer is non-null.  If we use adoptRef here, several tests crash.

> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2061
> > +            return WebGLGetInfo(PassRefPtr<WebGLRenderbuffer>(reinterpret_cast<WebGLRenderbuffer*>(object)));
> 
> Same thing about using adoptRef here.
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4001
> >      }
> 
> This looks very complicated and flaky. If you're worried about WebGLObject::detachContext() removing the object from the table and changing the iteration order, copy the HashSet<WebGLObject*> into a Vector<WebGLObject*> and iterate down the vector.
> 
> Also, you might want to consider having WebGLObject::detachContext() remove the object from this table rather than the destructor of WebGLObject. Right now calling detachContext() on the WebGLObject prevents the WebGLObject from removing itself from this table.

Done.

[Zhenyao Mo]

Created attachment 99378 [details]
Patch

[Kenneth Russell]

Comment on attachment 99378 [details]
Patch

Looks better to me. Nice work.

[Zhenyao Mo]

Committed r90180: <http://trac.webkit.org/changeset/90180>