237948 2022-03-16 01:38:34 -0700 [GTK][WPE] Debug build assertion, TextureMapperPlatformLayerProxyGL asserts when going back in the minibrowser 2022-03-19 14:51:34 -0700 1 1 1 Unclassified WebKit ANGLE WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=237949 InRadar P2 Normal --- 1 alex alex bugs-noreply cgarcia clord cmarcelo dino ews-watchlist kbr kkinnunen kondapallykalyan luiz mcatanzaro webkit-bug-importer zdobersek oldest_to_newest 1851686 0 alex 2022-03-16 01:38:34 -0700 We are not initializing m_compositorThread properly with the invalidation and the ThreadedCompositor is destroyed after a navigation, and when going back the browser asserts. 1851689 1 alex 2022-03-16 01:45:23 -0700 Added a new bug for a follow up patch to create a test for the ThreadedCompositor destroyed situation. 1851690 2 alex 2022-03-16 01:46:37 -0700 I forgot to say it is bug 237949. 1851692 3 454811 alex 2022-03-16 01:47:27 -0700 Created attachment 454811 Patch 1851761 4 ews-feeder 2022-03-16 06:04:55 -0700 Committed r291342 (248481@main): <https://commits.webkit.org/248481@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 454811. 1851762 5 webkit-bug-importer 2022-03-16 06:05:18 -0700 <rdar://problem/90365573> 1851802 6 454811 mcatanzaro 2022-03-16 08:06:05 -0700 Comment on attachment 454811 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454811&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL.cpp:96 > +#ifndef NDEBUG > + m_compositorThread = nullptr; > +#endif I'm not sure if the #ifndef NDEBUG was a great idea... when I see this, I think "why is this only needed with asserts enabled?" Probably better to keep the debug/release codepaths similar rather than different. Also: WebKit asserts can be enabled (ASSERT_ENABLED) even if libc asserts are not (NDEBUG). 1851807 7 cgarcia 2022-03-16 08:12:52 -0700 (In reply to Michael Catanzaro from comment #6) > Comment on attachment 454811 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454811&action=review > > > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL.cpp:96 > > +#ifndef NDEBUG > > + m_compositorThread = nullptr; > > +#endif > > I'm not sure if the #ifndef NDEBUG was a great idea... when I see this, I > think "why is this only needed with asserts enabled?" Probably better to > keep the debug/release codepaths similar rather than different. > > Also: WebKit asserts can be enabled (ASSERT_ENABLED) even if libc asserts > are not (NDEBUG). That's right, ASSERT_ENABLED should be used here and in other places where NDEBUG is used for m_compositorThread 1851839 8 alex 2022-03-16 08:59:25 -0700 (In reply to Michael Catanzaro from comment #6) > Comment on attachment 454811 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454811&action=review > > > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL.cpp:96 > > +#ifndef NDEBUG > > + m_compositorThread = nullptr; > > +#endif > > I'm not sure if the #ifndef NDEBUG was a great idea... when I see this, I > think "why is this only needed with asserts enabled?" Probably better to > keep the debug/release codepaths similar rather than different. > Basically all the file is using the variable with NDEBUG, if we don't use it the variable is not even defined, there is no change in the behaviour. It would be great if you can review this and the other proxies and propose a patch, I think the dmabuf is using a similar setup. > Also: WebKit asserts can be enabled (ASSERT_ENABLED) even if libc asserts > are not (NDEBUG). Urg, in that case even the compilation is broken because the variable is defined in the headers inside NDEBUG guards. A patch for that kind of code is really welcome. 1851848 9 mcatanzaro 2022-03-16 09:09:04 -0700 (In reply to Alejandro G. Castro from comment #8) > Basically all the file is using the variable with NDEBUG, if we don't use it > the variable is not even defined, there is no change in the behaviour. Oh OK, of course that's fine then. Should probably still use ASSERT_ENABLED rather than NDEBUG, but I see we have many other places in WebKit using NDEBUG. I suppose whoever next tries to enable assertions in release builds will wind up figuring this out.... 454811 2022-03-16 01:47:27 -0700 2022-03-16 06:04:58 -0700 Patch bug-237948-20220316094725.patch text/plain 2012 alex U3VidmVyc2lvbiBSZXZpc2lvbjogMjkxMzExCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDc5M2M5NDg4ZTRiOWMx MDQ3NDM2MTM5ZTA4YzAzODQzZDhmMWNmNy4uOGQ1NTBhMjc4YjBiZDc3YWI4OTY3ZTFkNTJhYzNj MDZkYjVhM2EzNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDIyLTAzLTE2ICBBbGVq YW5kcm8gRy4gQ2FzdHJvICA8YWxleEBpZ2FsaWEuY29tPgorCisgICAgICAgIFtHVEtdW1dQRV0g RGVidWcgYnVpbGQgYXNzZXJ0aW9uLCBUZXh0dXJlTWFwcGVyUGxhdGZvcm1MYXllclByb3h5R0wg YXNzZXJ0cyB3aGVuIGdvaW5nIGJhY2sgaW4gdGhlIG1pbmlicm93c2VyCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMzc5NDgKKworICAgICAgICBXZSBh cmUgbm90IGluaXRpYWxpemluZyBtX2NvbXBvc2l0b3JUaHJlYWQgcHJvcGVybHkgd2l0aCB0aGUK KyAgICAgICAgaW52YWxpZGF0aW9uIGFuZCB0aGUgVGhyZWFkZWRDb21wb3NpdG9yIGlzIGRlc3Ry b3llZCBhZnRlciBhCisgICAgICAgIG5hdmlnYXRpb24sIGFuZCB3aGVuIGdvaW5nIGJhY2sgdGhl IGJyb3dzZXIgYXNzZXJ0cyBiZWNhdXNlIHRoZQorICAgICAgICBUaHJlYWRlZENvbXBvc2l0b3Ig aXMgYSBkaWZmZXJlbnQgb25lLiBUaGlzIGlzIGp1c3QgZm9yIGRlYnVnCisgICAgICAgIGJ1aWxk cy4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBOb3Qg Y2xlYXIgaG93IHRvIHJlcHJvZHVjZSB0aGlzIHNpdHVhdGlvbiB3aXRoIHRlc3RzIGJlY2F1c2Ug d2UKKyAgICAgICAgbmVlZCB0byBuYXZpZ2F0ZSBiYWNrIHdpdGggZGlmZmVyZW50IG9yaWdpbnMu IEknbSBvcGVuaW5nIGEgYnVnCisgICAgICAgIGNvbm5lY3RlZCB0byB0aGlzIG9uZSB0byBjaGVj ayBob3cgdG8gZG8gaXQuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy90ZXhtYXAvVGV4 dHVyZU1hcHBlclBsYXRmb3JtTGF5ZXJQcm94eUdMLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlRl eHR1cmVNYXBwZXJQbGF0Zm9ybUxheWVyUHJveHlHTDo6aW52YWxpZGF0ZSk6CisKIDIwMjItMDMt MTUgIFBhdHJpY2sgR3JpZmZpcyAgPHBncmlmZmlzQGlnYWxpYS5jb20+CiAKICAgICAgICAgQWRk IGluaXRpYWwgaW1wbGVtZW50YXRpb24gb2YgRmV0Y2ggTWV0YWRhdGEKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3RleG1hcC9UZXh0dXJlTWFwcGVyUGxhdGZv cm1MYXllclByb3h5R0wuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvdGV4 bWFwL1RleHR1cmVNYXBwZXJQbGF0Zm9ybUxheWVyUHJveHlHTC5jcHAKaW5kZXggOWJlOTAyYjc2 Y2MzYjRkYTRjZTAwYTNlOTRiZWY3NGUwMjQxNjc2NC4uMzQwZGQwY2I3NDBmN2ViNjRhZDExYmUx NmExMmFmYzY5MWIwOWQ1NyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MvdGV4bWFwL1RleHR1cmVNYXBwZXJQbGF0Zm9ybUxheWVyUHJveHlHTC5jcHAKKysrIGIv U291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvdGV4bWFwL1RleHR1cmVNYXBwZXJQbGF0 Zm9ybUxheWVyUHJveHlHTC5jcHAKQEAgLTkxLDYgKzkxLDkgQEAgdm9pZCBUZXh0dXJlTWFwcGVy UGxhdGZvcm1MYXllclByb3h5R0w6OmludmFsaWRhdGUoKQogICAgIHsKICAgICAgICAgTG9ja2Vy IGxvY2tlciB7IG1fbG9jayB9OwogICAgICAgICBtX2NvbXBvc2l0b3IgPSBudWxscHRyOworI2lm bmRlZiBOREVCVUcKKyAgICAgICAgbV9jb21wb3NpdG9yVGhyZWFkID0gbnVsbHB0cjsKKyNlbmRp ZgogICAgICAgICBtX3RhcmdldExheWVyID0gbnVsbHB0cjsKIAogICAgICAgICBtX2N1cnJlbnRC dWZmZXIgPSBudWxscHRyOwo=