Bug 106582 - [EFL][WebGL] Add error handling to carefully manage Window backing pixmaps
Summary: [EFL][WebGL] Add error handling to carefully manage Window backing pixmaps
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Kalyan
URL:
Keywords:
Depends on:
Blocks: 104506
  Show dependency treegraph
 
Reported: 2013-01-10 11:02 PST by Kalyan
Modified: 2013-01-15 00:58 PST (History)
10 users (show)

See Also:


Attachments
patch (14.42 KB, patch)
2013-01-10 16:36 PST, Kalyan
kenneth: review-
Details | Formatted Diff | Diff
patch v2 (14.49 KB, patch)
2013-01-11 04:51 PST, Kalyan
no flags Details | Formatted Diff | Diff
patchv3 (14.48 KB, patch)
2013-01-11 07:36 PST, Kalyan
kenneth: review-
Details | Formatted Diff | Diff
patchv4 (9.32 KB, patch)
2013-01-11 12:14 PST, Kalyan
no flags Details | Formatted Diff | Diff
patchv5 (9.23 KB, patch)
2013-01-13 13:36 PST, Kalyan
no flags Details | Formatted Diff | Diff
patchv6 (7.61 KB, patch)
2013-01-14 13:23 PST, Kalyan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kalyan 2013-01-10 11:02:48 PST
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.
Comment 1 Kalyan 2013-01-10 16:36:05 PST
Created attachment 182222 [details]
patch
Comment 2 Kenneth Rohde Christiansen 2013-01-10 17:25:40 PST
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?
Comment 3 Kalyan 2013-01-11 04:51:58 PST
Created attachment 182322 [details]
patch v2
Comment 4 Kalyan 2013-01-11 04:56:19 PST
(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.
Comment 5 Kenneth Rohde Christiansen 2013-01-11 06:35:42 PST
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)
Comment 6 Kalyan 2013-01-11 07:36:04 PST
Created attachment 182344 [details]
patchv3
Comment 7 Kalyan 2013-01-11 07:39:14 PST
(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
Comment 8 Kenneth Rohde Christiansen 2013-01-11 08:45:38 PST
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
Comment 9 Chris Dumez 2013-01-11 09:15:45 PST
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).
Comment 10 Kalyan 2013-01-11 12:14:38 PST
Created attachment 182391 [details]
patchv4
Comment 11 Kalyan 2013-01-11 12:17:33 PST
(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
Comment 12 Kalyan 2013-01-11 12:23:59 PST
(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
Comment 13 Viatcheslav Ostapenko 2013-01-11 12:40:27 PST
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.
Comment 14 Kalyan 2013-01-13 13:36:13 PST
Created attachment 182489 [details]
patchv5
Comment 15 Laszlo Gombos 2013-01-14 11:16:33 PST
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.
Comment 16 Kalyan 2013-01-14 13:23:09 PST
Created attachment 182615 [details]
patchv6
Comment 17 Kalyan 2013-01-14 22:51:42 PST
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.
Comment 18 Kalyan 2013-01-15 00:28:32 PST
Comment on attachment 182615 [details]
patchv6

created a new bug to handle the regression.
Comment 19 WebKit Review Bot 2013-01-15 00:58:08 PST
Comment on attachment 182615 [details]
patchv6

Clearing flags on attachment: 182615

Committed r139725: <http://trac.webkit.org/changeset/139725>
Comment 20 WebKit Review Bot 2013-01-15 00:58:13 PST
All reviewed patches have been landed.  Closing bug.