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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=216106

Attachment 410107: Patch

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




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

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.

>
Source/ThirdParty/ANGLE/extensions/EGL_ANGLE_platform_angle_device_context_vola
tile_cgl.txt:3
> +    ANGLE_platform_angle_device_context_volatile_CGL

Lowercase _CGL here and below.

>
Source/ThirdParty/ANGLE/extensions/EGL_ANGLE_platform_angle_device_context_vola
tile_cgl.txt:11
> +    Kenneth Russel, Google

Two "l's please :)

>
Source/ThirdParty/ANGLE/extensions/EGL_ANGLE_platform_angle_device_context_vola
tile_cgl.txt:15
> +    Kenneth Russel, Google (kbr 'at' chromium 'dot' org)

Here too.

>
Source/ThirdParty/ANGLE/extensions/EGL_ANGLE_platform_angle_device_context_vola
tile_eagl.txt:11
> +    Kenneth Russel, Google

Here too.

>
Source/ThirdParty/ANGLE/extensions/EGL_ANGLE_platform_angle_device_context_vola
tile_eagl.txt:15
> +    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.