Bug 214999 – [WebGL2] Assert when restoring lost context

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Darpinian]

[WebGL2] Assert when restoring lost context

[James Darpinian]

Created attachment 405644 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[James Darpinian]

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.

[Darin Adler]

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?

[Darin Adler]

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.

[James Darpinian]

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.

[James Darpinian]

Created attachment 405763 [details]
Patch

[EWS]

Committed r265205: <https://trac.webkit.org/changeset/265205>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405763 [details].

[Radar WebKit Bug Importer]

<rdar://problem/66489320>