214999 2020-07-30 17:41:51 -0700 [WebGL2] Assert when restoring lost context 2020-08-03 13:38:08 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 jdarpinian jdarpinian cdumez changseok darin dino esprehn+autocc ews-watchlist graouts gyuyoung.kim kondapallykalyan webkit-bug-importer oldest_to_newest 1676661 0 jdarpinian 2020-07-30 17:41:51 -0700 [WebGL2] Assert when restoring lost context 1676662 1 405644 jdarpinian 2020-07-30 17:42:34 -0700 Created attachment 405644 Patch 1676683 2 405644 darin 2020-07-30 19:10:09 -0700 Comment on attachment 405644 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. 1676684 3 405644 darin 2020-07-30 19:11:20 -0700 Comment on attachment 405644 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. 1676751 4 405644 jdarpinian 2020-07-31 00:02:36 -0700 Comment on attachment 405644 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. 1676877 5 405644 darin 2020-07-31 11:04:13 -0700 Comment on attachment 405644 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? 1676878 6 405644 darin 2020-07-31 11:04:44 -0700 Comment on attachment 405644 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. 1677017 7 405644 jdarpinian 2020-07-31 15:48:31 -0700 Comment on attachment 405644 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. 1677062 8 405763 jdarpinian 2020-07-31 17:35:46 -0700 Created attachment 405763 Patch 1677348 9 ews-feeder 2020-08-03 10:49:19 -0700 Committed r265205: <https://trac.webkit.org/changeset/265205> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405763. 1677407 10 webkit-bug-importer 2020-08-03 13:38:08 -0700 <rdar://problem/66489320> 405644 2020-07-30 17:42:34 -0700 2020-07-31 17:35:44 -0700 Patch bug-214999-20200730174234.patch text/plain 1712 jdarpinian U3VidmVyc2lvbiBSZXZpc2lvbjogMjY1MDkxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOGY1NzJjYjI4Yjg4MDFj ODViNzk2MDU2MmFhZTZjOGMwY2Q4NDhiYy4uODY0NGEyYzM0NzQ1NzA4ZmMzYzUyODBkMmEzZWM5 ZDViZmVmYmVlNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDIwLTA3LTMwICBKYW1l cyBEYXJwaW5pYW4gIDxqZGFycGluaWFuQGNocm9taXVtLm9yZz4KKworICAgICAgICBbV2ViR0wy XSBBc3NlcnQgd2hlbiByZXN0b3JpbmcgbG9zdCBjb250ZXh0CisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMTQ5OTkKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXZSBuZWVkIHRvIGdldCByaWQgb2YgdGhlIG9s ZCBkZWZhdWx0IFZBTyBiZWZvcmUgY3JlYXRpbmcgdGhlIG5ldyBvbmUuCisKKyAgICAgICAgKiBo dG1sL2NhbnZhcy9XZWJHTDJSZW5kZXJpbmdDb250ZXh0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OldlYkdMMlJlbmRlcmluZ0NvbnRleHQ6OmluaXRpYWxpemVWZXJ0ZXhBcnJheU9iamVjdHMpOgor CiAyMDIwLTA3LTMwICBUaW0gSG9ydG9uICA8dGltb3RoeV9ob3J0b25AYXBwbGUuY29tPgogCiAg ICAgICAgIFdlYiBjb250ZW50IGdldHMgc3R1Y2sgaW4gYW4gaW5hY3RpdmUgc3RhdGUgKG5vIGN1 cnNvciB1cGRhdGVzIG9yIHRleHQgaW5zZXJ0aW9uIGNhcmV0KSB3aGVuIGFjdGl2YXRpbmcgYSB0 YWIgd2l0aCBhIHRodW1ibmFpbCB2aXNpYmxlCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9o dG1sL2NhbnZhcy9XZWJHTDJSZW5kZXJpbmdDb250ZXh0LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2h0 bWwvY2FudmFzL1dlYkdMMlJlbmRlcmluZ0NvbnRleHQuY3BwCmluZGV4IGFlMWZiYjc3ODVhZjdk OTNlYWYxMDI3MmJiNzBmMTE0ZTdiMjJkNDUuLjg3MDc1NjE1NTVhZjc4OTllNzQ5NmZmNDczMzRj ZGYzZTkwNTc2YTYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2h0bWwvY2FudmFzL1dlYkdM MlJlbmRlcmluZ0NvbnRleHQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvY2FudmFzL1dl YkdMMlJlbmRlcmluZ0NvbnRleHQuY3BwCkBAIC0yMTEsNiArMjExLDggQEAgbG9uZyBsb25nIFdl YkdMMlJlbmRlcmluZ0NvbnRleHQ6OmdldEludDY0UGFyYW1ldGVyKEdDR0xlbnVtIHBuYW1lKQog CiB2b2lkIFdlYkdMMlJlbmRlcmluZ0NvbnRleHQ6OmluaXRpYWxpemVWZXJ0ZXhBcnJheU9iamVj dHMoKQogeworICAgIC8vIElmIHdlJ3JlIHJlc3RvcmluZyBhIGxvc3QgY29udGV4dCwgd2Ugc2hv dWxkIGRlbGV0ZSB0aGUgb2xkIGRlZmF1bHQgVkFPIGJlZm9yZSBjcmVhdGluZyB0aGUgbmV3IG9u ZS4KKyAgICBtX2RlZmF1bHRWZXJ0ZXhBcnJheU9iamVjdCA9IG51bGxwdHI7CiAgICAgbV9kZWZh dWx0VmVydGV4QXJyYXlPYmplY3QgPSBXZWJHTFZlcnRleEFycmF5T2JqZWN0OjpjcmVhdGUoKnRo aXMsIFdlYkdMVmVydGV4QXJyYXlPYmplY3Q6OlR5cGU6OkRlZmF1bHQpOwogICAgIGFkZENvbnRl eHRPYmplY3QoKm1fZGVmYXVsdFZlcnRleEFycmF5T2JqZWN0KTsKICNpZiBVU0UoT1BFTkdMX0VT KQo= 405763 2020-07-31 17:35:46 -0700 2020-08-03 10:49:20 -0700 Patch bug-214999-20200731173545.patch text/plain 1560 jdarpinian U3VidmVyc2lvbiBSZXZpc2lvbjogMjY1MDkxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOGY1NzJjYjI4Yjg4MDFj ODViNzk2MDU2MmFhZTZjOGMwY2Q4NDhiYy4uMGE0NTQ4ZDdhZWY2ODZhZTM0MDJkNGI2ZTQ3MTFi ZDU2MjcxNWRiYiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDIwLTA3LTMwICBKYW1l cyBEYXJwaW5pYW4gIDxqZGFycGluaWFuQGNocm9taXVtLm9yZz4KKworICAgICAgICBbV2ViR0wy XSBBc3NlcnQgd2hlbiByZXN0b3JpbmcgbG9zdCBjb250ZXh0CisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMTQ5OTkKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBSZW1vdmUgYW4gYXNzZXJ0IHRoYXQgd2FzIHdy b25nIHdoZW4gcmVzdG9yaW5nIGEgbG9zdCBjb250ZXh0LgorCisgICAgICAgICogaHRtbC9jYW52 YXMvV2ViR0wyUmVuZGVyaW5nQ29udGV4dC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpXZWJHTDJS ZW5kZXJpbmdDb250ZXh0Ojppbml0aWFsaXplVmVydGV4QXJyYXlPYmplY3RzKToKKwogMjAyMC0w Ny0zMCAgVGltIEhvcnRvbiAgPHRpbW90aHlfaG9ydG9uQGFwcGxlLmNvbT4KIAogICAgICAgICBX ZWIgY29udGVudCBnZXRzIHN0dWNrIGluIGFuIGluYWN0aXZlIHN0YXRlIChubyBjdXJzb3IgdXBk YXRlcyBvciB0ZXh0IGluc2VydGlvbiBjYXJldCkgd2hlbiBhY3RpdmF0aW5nIGEgdGFiIHdpdGgg YSB0aHVtYm5haWwgdmlzaWJsZQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvaHRtbC9jYW52 YXMvV2ViR0xWZXJ0ZXhBcnJheU9iamVjdC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9odG1sL2NhbnZh cy9XZWJHTFZlcnRleEFycmF5T2JqZWN0LmNwcAppbmRleCBkZmRhOGQ4MTJjYzdhYmVmNjE0Yjdh ODNhMDVkMzc1MjUxMzNjNzRkLi4wNDBmOGNhZWQzZjhiMDhlZjVjOWEyZGRjNThkMTJlMjExY2Nm ZWYzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9odG1sL2NhbnZhcy9XZWJHTFZlcnRleEFy cmF5T2JqZWN0LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9odG1sL2NhbnZhcy9XZWJHTFZlcnRl eEFycmF5T2JqZWN0LmNwcApAQCAtNDksOCArNDksNiBAQCBXZWJHTFZlcnRleEFycmF5T2JqZWN0 OjpXZWJHTFZlcnRleEFycmF5T2JqZWN0KFdlYkdMUmVuZGVyaW5nQ29udGV4dEJhc2UmIGNvbnRl eAogI2lmIFVTRShPUEVOR0xfRVMpCiAgICAgaWYgKG1fdHlwZSAhPSBUeXBlOjpVc2VyKQogICAg ICAgICByZXR1cm47Ci0jZWxzZQotICAgIEFTU0VSVCh0eXBlICE9IFR5cGU6OkRlZmF1bHQgfHwg ISh0aGlzLT5jb250ZXh0KCktPm1fZGVmYXVsdFZlcnRleEFycmF5T2JqZWN0KSk7CiAjZW5kaWYK ICAgICBzZXRPYmplY3QodGhpcy0+Y29udGV4dCgpLT5ncmFwaGljc0NvbnRleHRHTCgpLT5jcmVh dGVWZXJ0ZXhBcnJheSgpKTsKIH0K