65408 2011-07-29 22:50:05 -0700 [EFL] Add "return" statement corresponding to abnormal condition on _ewk_frame_smart_add. 2011-08-19 08:25:21 -0700 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) Other Linux RESOLVED FIXED P2 Normal --- 0 hyuki.kim webkit-unassigned antognolli+webkit aroben gyuyoung.kim leandro lucas.de.marchi rakuco ryuan.choi webkit.review.bot oldest_to_newest 444258 0 hyuki.kim 2011-07-29 22:50:05 -0700 In order to avoid abnormal reference of variable "sd", it must be returned if it can't be allocated because variable "sd" can be referenced regardless of success of allocation. 444261 1 102427 hyuki.kim 2011-07-29 22:52:55 -0700 Created attachment 102427 Patch 444350 2 102427 gyuyoung.kim 2011-07-30 18:26:32 -0700 Comment on attachment 102427 Patch LGTM 444669 3 rakuco 2011-08-01 05:51:10 -0700 Please adjust the smart_add in ewk_view too. 447968 4 hyuki.kim 2011-08-08 05:50:31 -0700 > Please adjust the smart_add in ewk_view too. I could find the patch for ewk_view in our local repository that other engineer created. As a result, It will be created as another patch, and of course, I will ask him to create new bug for ewk_view. 448412 5 hyuki.kim 2011-08-08 19:24:09 -0700 > Please adjust the smart_add in ewk_view too. ewk_view's patch was created as new bug. :-) Refer the https://bugs.webkit.org/show_bug.cgi?id=65853 448533 6 hyuki.kim 2011-08-09 05:45:56 -0700 Hi Kubo and Leandro, Shall I get your review opinion for this patch ? :-) 448567 7 leandro 2011-08-09 07:06:40 -0700 I'm closing this bug as duplicate, since the patch in 65853 already fixes this. *** This bug has been marked as a duplicate of bug 65853 *** 449554 8 hyuki.kim 2011-08-10 18:43:42 -0700 > I'm closing this bug as duplicate, since the patch in 65853 already fixes this. Hi, Leandro, But, there was no ewk_frame related change on 65853. So, This is not duplicated. I have pulled change set from 65853. * ewk/ewk_view.cpp: (_ewk_view_smart_add): (_ewk_view_smart_resize): (_ewk_view_smart_move): (_ewk_view_smart_show): (_ewk_view_smart_hide): * ewk/ewk_view_single.c: (_ewk_view_single_smart_add): (_ewk_view_single_smart_resize): * ewk/ewk_view_tiled.c: (_ewk_view_tiled_smart_add): 451589 9 leandro 2011-08-16 07:05:18 -0700 Reopening bug, as I mistook ewk_frame with ewk_view. 451671 10 102427 leandro 2011-08-16 09:35:03 -0700 Comment on attachment 102427 Patch Informal r+. 452043 11 102427 gyuyoung.kim 2011-08-16 18:07:20 -0700 Comment on attachment 102427 Patch LGTM. 453761 12 102427 aroben 2011-08-19 05:28:59 -0700 Comment on attachment 102427 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102427&action=review > Source/WebKit/efl/ChangeLog:8 > + In order to avoid abnormal reference of variable "sd", > + it must be returned if it can't be allocated because variable "sd" can be referenced > + regardless of success of allocation. I think this could be stated a little more clearly. Something like: Bail out when we fail to allocate an Ewk_Frame_Smart_Data object rather than continuing on as if the allocation had succeeded. > Source/WebKit/efl/ewk/ewk_frame.cpp:184 > sd = (Ewk_Frame_Smart_Data*)calloc(1, sizeof(Ewk_Frame_Smart_Data)); > - if (!sd) > + if (!sd) { > CRITICAL("could not allocate Ewk_Frame_Smart_Data"); > - else > - evas_object_smart_data_set(o, sd); > + return; > + } Is this really a condition we expect to occur? Will we really continue to function correctly even if we return early from this function? We don't check for allocation failures in most of WebKit/WebCore, so I assume that if we're really in an out-of-memory situation we'll just crash a little later. 453777 13 hyuki.kim 2011-08-19 06:10:30 -0700 > I think this could be stated a little more clearly. Something like: Bail out when we fail to allocate an Ewk_Frame_Smart_Data object rather than continuing on as if the allocation had succeeded. Great sentence and I will update it according to your idea. :-) > Is this really a condition we expect to occur? Will we really continue to function correctly even if we return early from this function? We don't check for allocation failures in most of WebKit/WebCore, so I assume that if we're really in an out-of-memory situation we'll just crash a little later. I agree with you that crash may be the better way to detect the error condition faster. But, I believe that we will be able to notice the critical error message as soon as it will occur and this patch will give more time which browser can be alive little longer. :-) 453791 14 104504 hyuki.kim 2011-08-19 06:31:33 -0700 Created attachment 104504 Patch. 453840 15 104504 webkit.review.bot 2011-08-19 08:25:16 -0700 Comment on attachment 104504 Patch. Clearing flags on attachment: 104504 Committed r93410: <http://trac.webkit.org/changeset/93410> 453841 16 webkit.review.bot 2011-08-19 08:25:21 -0700 All reviewed patches have been landed. Closing bug. 102427 2011-07-29 22:52:55 -0700 2011-08-19 06:31:33 -0700 Patch 0730_return_statement.patch text/plain 1474 hyuki.kim ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvZWZsL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQv ZWZsL0NoYW5nZUxvZwppbmRleCA1MTliNzllLi42Nzc4Y2U2IDEwMDc1NQotLS0gYS9Tb3VyY2Uv V2ViS2l0L2VmbC9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdC9lZmwvQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTcgQEAKKzIwMTEtMDctMjkgIEt3YW5nSHl1ayBLaW0gIDxoeXVraS5raW1Ac2Ft c3VuZy5jb20+CisKKyAgICAgICAgW0VGTF0gQWRkICJyZXR1cm4iIHN0YXRlbWVudCBjb3JyZXNw b25kaW5nIHRvIGFibm9ybWFsIGNvbmRpdGlvbiBvbiBfZXdrX2ZyYW1lX3NtYXJ0X2FkZC4KKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTY1NDA4CisKKyAg ICAgICAgSW4gb3JkZXIgdG8gYXZvaWQgYWJub3JtYWwgcmVmZXJlbmNlIG9mIHZhcmlhYmxlICJz ZCIsCisgICAgICAgIGl0IG11c3QgYmUgcmV0dXJuZWQgaWYgaXQgY2FuJ3QgYmUgYWxsb2NhdGVk IGJlY2F1c2UgdmFyaWFibGUgInNkIiBjYW4gYmUgcmVmZXJlbmNlZAorICAgICAgICByZWdhcmRs ZXNzIG9mIHN1Y2Nlc3Mgb2YgYWxsb2NhdGlvbi4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICAqIGV3ay9ld2tfZnJhbWUuY3BwOgorICAgICAgICAoX2V3 a19mcmFtZV9zbWFydF9hZGQpOgorCiAyMDExLTA3LTI5ICBNaWNoYWwgUGFrdWxhIHZlbCBSdXRr YSAgPG0ucGFrdWxhQHNhbXN1bmcuY29tPgogCiAgICAgICAgIFtFRkxdIEltcGxlbWVudCBFZGl0 b3JDbGllbnRFZmw6OnJlc3BvbmRUb0NoYW5nZWRDb250ZW50cwpkaWZmIC0tZ2l0IGEvU291cmNl L1dlYktpdC9lZmwvZXdrL2V3a19mcmFtZS5jcHAgYi9Tb3VyY2UvV2ViS2l0L2VmbC9ld2svZXdr X2ZyYW1lLmNwcAppbmRleCAxMjJkNjYzLi44NjJiY2M5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi S2l0L2VmbC9ld2svZXdrX2ZyYW1lLmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0L2VmbC9ld2svZXdr X2ZyYW1lLmNwcApAQCAtMTc4LDEwICsxNzgsMTEgQEAgc3RhdGljIHZvaWQgX2V3a19mcmFtZV9z bWFydF9hZGQoRXZhc19PYmplY3QgKm8pCiAKICAgICBpZiAoIXNkKSB7CiAgICAgICAgIHNkID0g KEV3a19GcmFtZV9TbWFydF9EYXRhKiljYWxsb2MoMSwgc2l6ZW9mKEV3a19GcmFtZV9TbWFydF9E YXRhKSk7Ci0gICAgICAgIGlmICghc2QpCisgICAgICAgIGlmICghc2QpIHsKICAgICAgICAgICAg IENSSVRJQ0FMKCJjb3VsZCBub3QgYWxsb2NhdGUgRXdrX0ZyYW1lX1NtYXJ0X0RhdGEiKTsKLSAg ICAgICAgZWxzZQotICAgICAgICAgICAgZXZhc19vYmplY3Rfc21hcnRfZGF0YV9zZXQobywgc2Qp OworICAgICAgICAgICAgcmV0dXJuOworICAgICAgICB9CisgICAgICAgIGV2YXNfb2JqZWN0X3Nt YXJ0X2RhdGFfc2V0KG8sIHNkKTsKICAgICB9CiAKICAgICBzZC0+c2VsZiA9IG87Cg== 104504 2011-08-19 06:31:33 -0700 2011-08-19 08:25:16 -0700 Patch. 0730_return_statement_2.patch text/plain 1401 hyuki.kim ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvZWZsL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQv ZWZsL0NoYW5nZUxvZwppbmRleCAwMjZjYTI1Li5iMWZjZDE0IDEwMDc1NQotLS0gYS9Tb3VyY2Uv V2ViS2l0L2VmbC9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdC9lZmwvQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTUgQEAKKzIwMTEtMDgtMTkgIEt3YW5nSHl1ayBLaW0gIDxoeXVraS5raW1Ac2Ft c3VuZy5jb20+CisKKyAgICAgICAgW0VGTF0gQWRkICJyZXR1cm4iIHN0YXRlbWVudCBjb3JyZXNw b25kaW5nIHRvIGFibm9ybWFsIGNvbmRpdGlvbiBvbiBfZXdrX2ZyYW1lX3NtYXJ0X2FkZC4KKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTY1NDA4CisKKyAg ICAgICAgQmFpbCBvdXQgd2hlbiB3ZSBmYWlsIHRvIGFsbG9jYXRlIGFuIEV3a19GcmFtZV9TbWFy dF9EYXRhIG9iamVjdCByYXRoZXIgdGhhbiBjb250aW51aW5nIG9uIGFzIGlmIHRoZSBhbGxvY2F0 aW9uIGhhZCBzdWNjZWVkZWQuCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgKiBld2svZXdrX2ZyYW1lLmNwcDoKKyAgICAgICAgKF9ld2tfZnJhbWVfc21h cnRfYWRkKToKKwogMjAxMS0wOC0xOCAgUnl1YW4gQ2hvaSAgPHJ5dWFuLmNob2lAc2Ftc3VuZy5j b20+CiAKICAgICAgICAgW0VGTF0gQnJva2VuIHJlbmRlcmluZyBvY2N1cmVkIHdoZW4gcmVzaXpl ZCBpbiBld2tfdmlld19zaW5nbGUuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2VmbC9ld2sv ZXdrX2ZyYW1lLmNwcCBiL1NvdXJjZS9XZWJLaXQvZWZsL2V3ay9ld2tfZnJhbWUuY3BwCmluZGV4 IDM0YjY4ZjcuLmNlZDVlODEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvZWZsL2V3ay9ld2tf ZnJhbWUuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvZWZsL2V3ay9ld2tfZnJhbWUuY3BwCkBAIC0x ODIsMTAgKzE4MiwxMSBAQCBzdGF0aWMgdm9pZCBfZXdrX2ZyYW1lX3NtYXJ0X2FkZChFdmFzX09i amVjdCAqbykKIAogICAgIGlmICghc2QpIHsKICAgICAgICAgc2QgPSAoRXdrX0ZyYW1lX1NtYXJ0 X0RhdGEqKWNhbGxvYygxLCBzaXplb2YoRXdrX0ZyYW1lX1NtYXJ0X0RhdGEpKTsKLSAgICAgICAg aWYgKCFzZCkKKyAgICAgICAgaWYgKCFzZCkgewogICAgICAgICAgICAgQ1JJVElDQUwoImNvdWxk IG5vdCBhbGxvY2F0ZSBFd2tfRnJhbWVfU21hcnRfRGF0YSIpOwotICAgICAgICBlbHNlCi0gICAg ICAgICAgICBldmFzX29iamVjdF9zbWFydF9kYXRhX3NldChvLCBzZCk7CisgICAgICAgICAgICBy ZXR1cm47CisgICAgICAgIH0KKyAgICAgICAgZXZhc19vYmplY3Rfc21hcnRfZGF0YV9zZXQobywg c2QpOwogICAgIH0KIAogICAgIHNkLT5zZWxmID0gbzsK