Bug 106582 – [EFL][WebGL] Add error handling to carefully manage Window backing pixmaps

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kalyan]

we use XCompositeNameWindowPixmap to create a pixmap that serves as a reference to the off-screen storage for the Window Handle. We expect the window to be valid and the created glx pixmap to be a valid drawable. This may not be valid always, we need to ensure this by adding appropriate X error checks.

[Kalyan]

Created attachment 182222 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 182222 [details]
patch

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

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:57
> +static int handleXError(Display *, XErrorEvent *event)

style... we position * differently

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:79
> +class scopedErrorHandler {

classes start with uppercase!

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:87
> +        m_orginalErrHandler = XSetErrorHandler(handleXError);

why not spell out error in originalErrorHandler

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:98
> +        ValidOperation = true;

g_validOperation to show that it is a static?

[Kalyan]

Created attachment 182322 [details]
patch v2

[Kalyan]

(In reply to comment #2)
> (From update of attachment 182222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182222&action=review

> > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:79
> > +class scopedErrorHandler {
> 
> classes start with uppercase!

done
 
> > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:87
> > +        m_orginalErrHandler = XSetErrorHandler(handleXError);
> 
> why not spell out error in originalErrorHandler
originalErrorHandler may be set by application or other object which GraphicsSurfaceGLX cannot have control on. 

> > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:98
> > +        ValidOperation = true;
> 
> g_validOperation to show that it is a static?
changed it to gValidOperation, style checker complaints about underscore.

[Kenneth Rohde Christiansen]

Comment on attachment 182322 [details]
patch v2

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

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:60
> +    if (event->error_code == BadMatch || event->error_code == BadWindow || event->error_code == BadAlloc)
> +        gValidOperation = false;

why not 

else gValidOperation = true?

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:85
> +        // XSync() needs to be called to ensure that current errors are handled by the original handler.

must be called

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:87
> +        m_orginalErrHandler = XSetErrorHandler(handleXError);

spelling error... "original" -> m_previousErrorHandler = ...

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:284
> +        ScopedErrorHandler errHandler(m_display);
> +        m_xPixmap = XCompositeNameWindowPixmap(m_display, winId);
> +
> +        if (!errHandler.isValidOperation()) {
> +            reset();

ScopedXErrorHandler handler(m_display)

[Kalyan]

Created attachment 182344 [details]
patchv3

[Kalyan]

(In reply to comment #5)
> (From update of attachment 182322 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182322&action=review

> why not 
> 
> else gValidOperation = true?
> 
This flag is reset to true (i.e in isValidOperation ) before checking for any errors.

> 
> must be called
>
> spelling error... "original" -> m_previousErrorHandler = ...
> 

> ScopedXErrorHandler handler(m_display)

fixed

[Kenneth Rohde Christiansen]

Comment on attachment 182344 [details]
patchv3

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

would like someone else to have a look

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:79
> +class ScopedErrorHandler {

I would add an X here... like ScopedXErrorHandler... also as it only treads some errors, it should maybe be renamed.

like ScopedXWindowCreationErrorHandler or so

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:93
> +        XSetErrorHandler(m_originalErrHandler);

I dont like Err... I would just called it m_previousHandler here

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:278
> +        // Ensure that the window is mapped.
> +        if (attr.map_state == IsUnmapped || attr.map_state == IsUnviewable)
> +            return;

these changes are not explained in the changelog

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:420
> +    void reset()

this seems more like a clear() than a reset.. as it doesnt set anything again

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:539
> +
> +    if (m_private->isReceiver() && platformGetTextureID()) {
>          glBindTexture(GL_TEXTURE_2D, platformGetTextureID());

these changes should be explained

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:553
> +    return m_private ? m_private->size() : IntSize();

ditto

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.cpp:93
>  
>      XSetWindowBackgroundPixmap(display, tempHandleId, 0);
> +#if USE(GRAPHICS_SURFACE)
>      XCompositeRedirectWindow(display, tempHandleId, CompositeRedirectManual);
> +#endif
>      *handleId = tempHandleId;

no error handling needed herE?

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.cpp:157
> +        for (int i = 0; i< matchingCount; i++) {

space after i please. change needs to be explained

[Chris Dumez]

Comment on attachment 182344 [details]
patchv3

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

It seems like this patch is doing many changes at once, maybe it could be split?

> Source/WebCore/ChangeLog:3
> +        [EFL] [WebGL]Add error handling to carefully manage Window backing pixmaps.

No space between the tags and there should be one space between the last tag and the title.

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:56
> +static bool gValidOperation = true;

why are you using 'g' prefix? I think usual style would be "validOperation".

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:86
> +        XSync(m_display, FALSE);

false

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:92
> +        // Restore the orginal handler.

original

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:96
> +    bool isValidOperation()

Feels like this should be const.

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:100
> +        XSync(m_display, FALSE);

false

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:425
> +        m_glxPixmap = 0;

zero assignments could be inside the if() scope.

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.cpp:142
> +        if (!m_configVisualInfo && chosenVisualInfo->depth == 32)

&& visualDepth?

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.cpp:152
> +    XVisualInfo* matchingVisuals;

This can be defined when assigned.

>> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.cpp:157
>> +        for (int i = 0; i< matchingCount; i++) {
> 
> space after i please. change needs to be explained

We usually use prefixed increment when possible (style).

[Kalyan]

Created attachment 182391 [details]
patchv4

[Kalyan]

(In reply to comment #8)
> (From update of attachment 182344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182344&action=review
> > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:79
> > +class ScopedErrorHandler {

changed name to ScopedXPixmapCreationErrorHandler

>        XSetErrorHandler(m_originalErrHandler); 
> I dont like Err... I would just called it m_previousHandler here

done

> 
> > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:420
> > +    void reset()
>
> this seems more like a clear() than a reset.. as it doesnt set anything again
done
 
> > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:539
> > +
> > +    if (m_private->isReceiver() && platformGetTextureID()) {
> >          glBindTexture(GL_TEXTURE_2D, platformGetTextureID());
> 
> these changes should be explained
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:553
> > +    return m_private ? m_private->size() : IntSize();
> 
> ditto
 done

[Kalyan]

(In reply to comment #9)
> (From update of attachment 182344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182344&action=review
> 
> It seems like this patch is doing many changes at once, maybe it could be split?

k, done

> > Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:56
> > +static bool gValidOperation = true;
> 
> why are you using 'g' prefix? I think usual style would be "validOperation".
comment2 had a proposal to use g_validOperation. I haven't found any guidelines on this. Class static data members are supposed to use s_ prefix.
Some classes seem to use caps for the first letter for global static variables. 
 
> false
> 
> original
> Feels like this should be const. 
> false
> zero assignments could be inside the if() scope.

done

[Viatcheslav Ostapenko]

Comment on attachment 182391 [details]
patchv4

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

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:86
> +        XSync(m_display, false);

Of course it will slow down things, but it is better than crash and only during canvas setup.

[Kalyan]

Created attachment 182489 [details]
patchv5

[Laszlo Gombos]

Comment on attachment 182489 [details]
patchv5

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

> Source/WebCore/ChangeLog:23
> +
> +        Helper Class to catch XErrors.

Seems to be an extra or misplaced line.

> Source/WebCore/ChangeLog:35
> +
> +         Added null ptr checks.

Same here - extra line.

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:56
> +static bool gValidOperation = true;

I saw the previous comment from Kenneth but it seems to me that this should be validOperation to match the rest of the code in WebKit.

[Kalyan]

Created attachment 182615 [details]
patchv6

[Kalyan]

Comment on attachment 182615 [details]
patchv6

Found regression with latest code, some WebGL demo's crash(Without this patch though).

Removing the request for cq till we are able to find the root cause for this.

[Kalyan]

Comment on attachment 182615 [details]
patchv6

created a new bug to handle the regression.

[WebKit Review Bot]

Comment on attachment 182615 [details]
patchv6

Clearing flags on attachment: 182615

Committed r139725: <http://trac.webkit.org/changeset/139725>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.