Bug 237948

Added a new bug for a follow up patch to create a test for the ThreadedCompositor destroyed situation.

I forgot to say it is bug 237949.

Created attachment 454811 [details]
Patch

Committed r291342 (248481@main): <https://commits.webkit.org/248481@main>

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

<rdar://problem/90365573>

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).

(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

(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.

(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....