124722 2013-11-21 10:12:06 -0800 [EFL] X11Helper::createPixmap doesn't initialise out value handleId 2013-11-25 15:53:16 -0800 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 spena spena cmarcelo commit-queue gyuyoung.kim kenneth laszlo.gombos lucas.de.marchi luiz noam zeno oldest_to_newest 953153 0 spena 2013-11-21 10:12:06 -0800 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. 953160 1 217581 spena 2013-11-21 10:23:28 -0800 Created attachment 217581 Patch 953921 2 217581 gyuyoung.kim 2013-11-25 01:37:51 -0800 Comment on attachment 217581 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 ? 953928 3 spena 2013-11-25 01:57:11 -0800 (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. 953929 4 spena 2013-11-25 01:57:50 -0800 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. 953931 5 217581 gyuyoung.kim 2013-11-25 02:21:33 -0800 Comment on attachment 217581 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. 954144 6 217581 commit-queue 2013-11-25 15:53:13 -0800 Comment on attachment 217581 Patch Clearing flags on attachment: 217581 Committed r159772: <http://trac.webkit.org/changeset/159772> 954145 7 commit-queue 2013-11-25 15:53:16 -0800 All reviewed patches have been landed. Closing bug. 217581 2013-11-21 10:23:28 -0800 2013-11-25 15:53:13 -0800 Patch bug-124722-20131121182308.patch text/plain 2154 spena U3VidmVyc2lvbiBSZXZpc2lvbjogMTU5NjI5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOGJlMDFmYjdkMDI1ZDBm MDhlNzMyMzg2NWFiYzk3YTYxZTU4YmZmMy4uYWY1ODdlZjhlN2MwYzI0OWRiZDQwMDdjMGVhNTkx ZWZhMWFjOTkzMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDEzLTExLTIxICBTaW1v biBQZW5hICA8c2ltb24ucGVuYUBzYW1zdW5nLmNvbT4KKworICAgICAgICBbRUZMXSBYMTFIZWxw ZXI6OmNyZWF0ZVBpeG1hcCBkb2Vzbid0IGluaXRpYWxpc2Ugb3V0IHZhbHVlIGhhbmRsZUlkCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjQ3MjIKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGUgb3Zlcmxv YWRlZCBmdW5jdGlvbnMgWDExSGVscGVyOjpjcmVhdGVQaXhtYXAgZG9uJ3QgaW5pdGlhbGlzZSBv dXQKKyAgICAgICAgdmFsdWUgaGFuZGxlSWQsIGFuZCB0aGV5IGRvIGVhcmx5IHJldHVybnMgb24g ZXJyb3Igc2l0dWF0aW9ucy4gU2luY2UKKyAgICAgICAgdGhlIGZ1bmN0aW9ucyBhcmUgdm9pZCBh bmQgdGhleSBkb24ndCBjb21tdW5pY2F0ZSB0aGVpciBmYWlsdXJlIGluIGFueQorICAgICAgICB3 YXksIHJldHVybmluZyBhbiBvdXQgdmFsdWUgd2l0aG91dCBpbml0aWFsaXNpbmcgaXQgY291bGQg bWFrZSB0aGUKKyAgICAgICAgZXJyb3IgZ28gZmFydGhlciB1bm5vdGljZWQuIFdpdGggdGhlIHZh cmlhYmxlIGJlaW5nIGluaXRpYWxpc2VkLCBpdCBjYW4KKyAgICAgICAgYmUgcmVsaWFibHkgY2hl Y2tlZCBmb3IgZXJyb3JzIHdoZW4gdGhlIGZ1bmN0aW9uIHJldHVybnMuCisKKyAgICAgICAgKiBw bGF0Zm9ybS9ncmFwaGljcy9zdXJmYWNlcy9nbHgvWDExSGVscGVyLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6OlgxMUhlbHBlcjo6Y3JlYXRlUGl4bWFwKTogSW5pdGlhbGlzZSBoYW5kbGVJZCB0byAw LgorCiAyMDEzLTExLTIxICBNaWhhaSBNYWVyZWFuICA8bW1hZXJlYW5AYWRvYmUuY29tPgogCiAg ICAgICAgIEZpeCBob3ZlciBhcmVhIGZvciBkaXZzIHdpdGggY3NzIHRyYW5zZm9ybXMKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3N1cmZhY2VzL2dseC9YMTFI ZWxwZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvc3VyZmFjZXMvZ2x4 L1gxMUhlbHBlci5jcHAKaW5kZXggNjk4ZTgwMjE0YWRkYzg5YmE1NWZhZjE0YTQ4MjkwOGMzNWNm NTliMy4uNDA5YTQ3ZjQwMWYyMzY1NmEzNzQ0YWIzYWM0ZGYxOGY1MmFiZTc0OSAxMDA2NDQKLS0t IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvc3VyZmFjZXMvZ2x4L1gxMUhlbHBl ci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvc3VyZmFjZXMvZ2x4 L1gxMUhlbHBlci5jcHAKQEAgLTE0Miw2ICsxNDIsNyBAQCB2b2lkIFgxMUhlbHBlcjo6cmVzaXpl V2luZG93KGNvbnN0IEludFJlY3QmIG5ld1JlY3QsIGNvbnN0IHVpbnQzMl90IHdpbmRvd0lkKQog CiB2b2lkIFgxMUhlbHBlcjo6Y3JlYXRlUGl4bWFwKFBpeG1hcCogaGFuZGxlSWQsIGNvbnN0IFhW aXN1YWxJbmZvJiB2aXN1YWxJbmZvLCBjb25zdCBJbnRTaXplJiBzaXplKQogeworICAgICpoYW5k bGVJZCA9IDA7CiAgICAgRGlzcGxheSogZGlzcGxheSA9IG5hdGl2ZURpc3BsYXkoKTsKICAgICBp ZiAoIWRpc3BsYXkpCiAgICAgICAgIHJldHVybjsKQEAgLTI2NSw2ICsyNjYsNyBAQCB2b2lkIFgx MUhlbHBlcjo6Y3JlYXRlT2ZmU2NyZWVuV2luZG93KHVpbnQzMl90KiBoYW5kbGVJZCwgY29uc3Qg RUdMaW50IGlkLCBib29sCiAKIHZvaWQgWDExSGVscGVyOjpjcmVhdGVQaXhtYXAoUGl4bWFwKiBo YW5kbGVJZCwgY29uc3QgRUdMaW50IGlkLCBib29sIGhhc0FscGhhLCBjb25zdCBJbnRTaXplJiBz aXplKQogeworICAgICpoYW5kbGVJZCA9IDA7CiAgICAgVmlzdWFsSUQgdmlzdWFsSWQgPSBzdGF0 aWNfY2FzdDxWaXN1YWxJRD4oaWQpOwogCiAgICAgaWYgKCF2aXN1YWxJZCkK