[WebGL2] Assert when restoring lost context
Created attachment 405644 [details] Patch
Comment on attachment 405644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405644&action=review > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:215 > + // If we're restoring a lost context, we should delete the old default VAO before creating the new one. > + m_defaultVertexArrayObject = nullptr; This points to a design problem. Decrementing the reference count on a reference counted object is not guaranteed to delete the object. If we need control of timing of object deletion then it’s not good to be using reference counted objects.
Comment on attachment 405644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405644&action=review >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:215 >> + m_defaultVertexArrayObject = nullptr; > > This points to a design problem. Decrementing the reference count on a reference counted object is not guaranteed to delete the object. If we need control of timing of object deletion then it’s not good to be using reference counted objects. I suppose we could just write in a comment here the reason why we are guaranteed there is no one else holding a reference to this. Or we could come up with a way to do the deletion explicitly other than destroying the C++ reference counted thing.
Comment on attachment 405644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405644&action=review >>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:215 >>> + m_defaultVertexArrayObject = nullptr; >> >> This points to a design problem. Decrementing the reference count on a reference counted object is not guaranteed to delete the object. If we need control of timing of object deletion then it’s not good to be using reference counted objects. > > I suppose we could just write in a comment here the reason why we are guaranteed there is no one else holding a reference to this. Or we could come up with a way to do the deletion explicitly other than destroying the C++ reference counted thing. Sorry, "delete" is the wrong word. The object does not need to be deleted, it just needs to be disassociated from the context. I will fix the comment.
Comment on attachment 405644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405644&action=review >>>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:215 >>>> + m_defaultVertexArrayObject = nullptr; >>> >>> This points to a design problem. Decrementing the reference count on a reference counted object is not guaranteed to delete the object. If we need control of timing of object deletion then it’s not good to be using reference counted objects. >> >> I suppose we could just write in a comment here the reason why we are guaranteed there is no one else holding a reference to this. Or we could come up with a way to do the deletion explicitly other than destroying the C++ reference counted thing. > > Sorry, "delete" is the wrong word. The object does not need to be deleted, it just needs to be disassociated from the context. I will fix the comment. I’d like to understand what "disassociated" means. Does it literally just mean that m_defaultVertexArrayObject must be null?
Comment on attachment 405644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405644&action=review >>>>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:215 >>>>> + m_defaultVertexArrayObject = nullptr; >>>> >>>> This points to a design problem. Decrementing the reference count on a reference counted object is not guaranteed to delete the object. If we need control of timing of object deletion then it’s not good to be using reference counted objects. >>> >>> I suppose we could just write in a comment here the reason why we are guaranteed there is no one else holding a reference to this. Or we could come up with a way to do the deletion explicitly other than destroying the C++ reference counted thing. >> >> Sorry, "delete" is the wrong word. The object does not need to be deleted, it just needs to be disassociated from the context. I will fix the comment. > > I’d like to understand what "disassociated" means. Does it literally just mean that m_defaultVertexArrayObject must be null? Could you clarify what the specific issue is? I understand roughly but not fully.
Comment on attachment 405644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405644&action=review >>>>>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:215 >>>>>> + m_defaultVertexArrayObject = nullptr; >>>>> >>>>> This points to a design problem. Decrementing the reference count on a reference counted object is not guaranteed to delete the object. If we need control of timing of object deletion then it’s not good to be using reference counted objects. >>>> >>>> I suppose we could just write in a comment here the reason why we are guaranteed there is no one else holding a reference to this. Or we could come up with a way to do the deletion explicitly other than destroying the C++ reference counted thing. >>> >>> Sorry, "delete" is the wrong word. The object does not need to be deleted, it just needs to be disassociated from the context. I will fix the comment. >> >> I’d like to understand what "disassociated" means. Does it literally just mean that m_defaultVertexArrayObject must be null? > > Could you clarify what the specific issue is? I understand roughly but not fully. There's just an assert in WebGLVertexArrayObject::create that m_defaultVertexArrayObject is null when creating an object with Type::Default, I suppose to prevent people accidentally specifying Type::Default when they didn't mean to. Since that seems unlikely, an alternative approach would be to simply remove the assert.
Created attachment 405763 [details] Patch
Committed r265205: <https://trac.webkit.org/changeset/265205> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405763 [details].
<rdar://problem/66489320>