Bug 124722 – [EFL] X11Helper::createPixmap doesn't initialise out value handleId

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Simon Pena]

Both 

void X11Helper::createPixmap(Pixmap* handleId, const XVisualInfo& visualInfo, const IntSize& size) and 
void X11Helper::createPixmap(Pixmap* handleId, const EGLint id, bool hasAlpha, const IntSize& size) 

don't initialise the out parameter handleId.

The problem with that is that these functions do early returns under certain error situations, so clients using them could be checking an invalid value for handleId. At the moment, this is only call from EGLXSurface.cpp and GLXSurface.cpp, and while they could initialise themselves the argument before the call, I think that is error prone. I propose doing an early initialisation in createPixmap just to be on the safe side.

Patch following now.

[Simon Pena]

Created attachment 217581 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 217581 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        be reliably checked for errors when the function returns.

Though I'm not an expert in this area, I don't understand this description well. one of createPixmap() functions already supports error messages as below.

void X11Helper::createPixmap(Pixmap* handleId, const XVisualInfo& visualInfo, const IntSize& size)
{
    Display* display = nativeDisplay();
    if (!display)
        return;

    if (!visualInfo.visual) {
        LOG_ERROR("Failed to find valid XVisual.");
        return;
    }

    Window xWindow = offscreenRootWindow();
    if (!xWindow) {
        LOG_ERROR("Failed to create offscreen root window.");
        return;
    }

    Pixmap tempHandleId = XCreatePixmap(display, xWindow, size.width(), size.height(), visualInfo.depth);

    if (!tempHandleId) {
        LOG_ERROR("Failed to create offscreen pixmap.");
        return;
    }

    *handleId = tempHandleId;
    XSync(X11Helper::nativeDisplay(), false);

...

If we need to handle error situations for all createPixmap(), aren't we just add error messages in the other function as well ?


Could you explain what is benefit when initializing *handleId ?

[Simon Pena]

(In reply to comment #2)
> (From update of attachment 217581 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217581&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        be reliably checked for errors when the function returns.
> 
> Though I'm not an expert in this area, I don't understand this description well. one of createPixmap() functions already supports error messages as below.

...

> ...
> 
> If we need to handle error situations for all createPixmap(), aren't we just add error messages in the other function as well ?
> 
> 
> Could you explain what is benefit when initializing *handleId ?

The problem with that approach is that we rely on error messages that will be seen in runtime. Since the function doesn't return false, or give any other hint that it failed, in runtime you cannot really check if everything is going fine.

When properly initialising handleId, the clients consuming the API which are currently doing 

if (!handleId)
   doSomething()

will be able to manage the error in a proper way. At the moment, that kind of checks is wrong: as handleId hasn't been initialised, it can have any value.

So my point is: ensuring handleId is initialised, its clients can safely use it.

[Simon Pena]

In my first paragraph, I meant that the error messages go to the console, but you cannot really use them in the logic of the clients using that method.

[Gyuyoung Kim]

Comment on attachment 217581 [details]
Patch

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

>>> Source/WebCore/ChangeLog:13
>>> +        be reliably checked for errors when the function returns.
>> 
>> Though I'm not an expert in this area, I don't understand this description well. one of createPixmap() functions already supports error messages as below.
>> 
>> void X11Helper::createPixmap(Pixmap* handleId, const XVisualInfo& visualInfo, const IntSize& size)
>> {
>>     Display* display = nativeDisplay();
>>     if (!display)
>>         return;
>> 
>>     if (!visualInfo.visual) {
>>         LOG_ERROR("Failed to find valid XVisual.");
>>         return;
>>     }
>> 
>>     Window xWindow = offscreenRootWindow();
>>     if (!xWindow) {
>>         LOG_ERROR("Failed to create offscreen root window.");
>>         return;
>>     }
>> 
>>     Pixmap tempHandleId = XCreatePixmap(display, xWindow, size.width(), size.height(), visualInfo.depth);
>> 
>>     if (!tempHandleId) {
>>         LOG_ERROR("Failed to create offscreen pixmap.");
>>         return;
>>     }
>> 
>>     *handleId = tempHandleId;
>>     XSync(X11Helper::nativeDisplay(), false);
>> 
>> ...
>> 
>> If we need to handle error situations for all createPixmap(), aren't we just add error messages in the other function as well ?
>> 
>> 
>> Could you explain what is benefit when initializing *handleId ?
> 
> ...

Ok, I understand what concern you point out. Caller can't check if pixmap is null under current implementation. r=me. However, it would be good if graphics folk take a look this finally before landing.

[WebKit Commit Bot]

Comment on attachment 217581 [details]
Patch

Clearing flags on attachment: 217581

Committed r159772: <http://trac.webkit.org/changeset/159772>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.