[webkit-reviews] review granted: [Bug 216106] [iOS WK1] Crashes when using ANGLE WebGL from another thread : [Attachment 410107] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 30 22:31:05 PDT 2020

Kenneth Russell <kbr at google.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 216106: [iOS WK1] Crashes when using ANGLE WebGL from another thread

Attachment 410107: Patch


--- Comment #29 from Kenneth Russell <kbr at google.com> ---
Comment on attachment 410107
  --> https://bugs.webkit.org/attachment.cgi?id=410107

View in context: https://bugs.webkit.org/attachment.cgi?id=410107&action=review

Great work Kimmo solving this problem! Great work on the test, too. This change
looks nearly perfect to me - only one comment / question, but setting r+ now in
order to save re-review time regardless of which way it's answered, once you
have the build failures fixed.

> +    ANGLE_platform_angle_device_context_volatile_CGL

Lowercase _CGL here and below.

> +    Kenneth Russel, Google

Two "l's please :)

> +    Kenneth Russel, Google (kbr 'at' chromium 'dot' org)

Here too.

> +    Kenneth Russel, Google

Here too.

> +    Kenneth Russel, Google (kbr 'at' chromium 'dot' org)

Here too.

> Source/ThirdParty/ANGLE/src/libANGLE/Display.h:105
> +    // Called before all display state dependent EGL function. Backends can
set up, for example,

function -> functions

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:765
> +    // potentially switching threads later, we should flush.

See below - if the code is removed then this comment should be updated.

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:771
> +	       gl::Flush();

I don't understand the need for this flush on MakeCurrent. If we're coming in
with a different WebGL context / GraphicsContextGL current than this one, it's
still not necessary to do a glFlush at this point, because the same hardware
context is used for both GraphicsContextGL instances. Commands are properly
ordered between them. ANGLE handles the virtual context switch and sending down
the user's GL context state to the hardware context in its EGL_MakeCurrent.

If we're coming in and the application's EGAL context (for example) is current,
then this code won't have the desired effect - ANGLE's EGL_GetCurrentContext()
will not know about that real native context and will return nullptr.
gl::Flush() will not necessarily perform a real glFlush anyway.

Basically, it seems to me that the 3 lines of code above - the calls to
EGL_GetCurrentContext through gl::Flush - should be removed.

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:801
> +	   // Called when we do not know if this thread again.

...know if we will ever see another call from this thread again.

More information about the webkit-reviews mailing list