Bug 65408

Summary: [EFL] Add "return" statement corresponding to abnormal condition on _ewk_frame_smart_add.
Product: WebKit Reporter: KwangHyuk <hyuki.kim>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: antognolli+webkit, aroben, gyuyoung.kim, leandro, lucas.de.marchi, rakuco, ryuan.choi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch. none

Description KwangHyuk 2011-07-29 22:50:05 PDT
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.
Comment 1 KwangHyuk 2011-07-29 22:52:55 PDT
Created attachment 102427 [details]
Patch
Comment 2 Gyuyoung Kim 2011-07-30 18:26:32 PDT
Comment on attachment 102427 [details]
Patch

LGTM
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-08-01 05:51:10 PDT
Please adjust the smart_add in ewk_view too.
Comment 4 KwangHyuk 2011-08-08 05:50:31 PDT
> 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.
Comment 5 KwangHyuk 2011-08-08 19:24:09 PDT
> 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
Comment 6 KwangHyuk 2011-08-09 05:45:56 PDT
Hi Kubo and Leandro, Shall I get your review opinion for this patch ? :-)
Comment 7 Leandro Pereira 2011-08-09 07:06:40 PDT
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 ***
Comment 8 KwangHyuk 2011-08-10 18:43:42 PDT
> 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):
Comment 9 Leandro Pereira 2011-08-16 07:05:18 PDT
Reopening bug, as I mistook ewk_frame with ewk_view.
Comment 10 Leandro Pereira 2011-08-16 09:35:03 PDT
Comment on attachment 102427 [details]
Patch

Informal r+.
Comment 11 Gyuyoung Kim 2011-08-16 18:07:20 PDT
Comment on attachment 102427 [details]
Patch

LGTM.
Comment 12 Adam Roben (:aroben) 2011-08-19 05:28:59 PDT
Comment on attachment 102427 [details]
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.
Comment 13 KwangHyuk 2011-08-19 06:10:30 PDT
> 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. :-)
Comment 14 KwangHyuk 2011-08-19 06:31:33 PDT
Created attachment 104504 [details]
Patch.
Comment 15 WebKit Review Bot 2011-08-19 08:25:16 PDT
Comment on attachment 104504 [details]
Patch.

Clearing flags on attachment: 104504

Committed r93410: <http://trac.webkit.org/changeset/93410>
Comment 16 WebKit Review Bot 2011-08-19 08:25:21 PDT
All reviewed patches have been landed.  Closing bug.