Bug 65408 – [EFL] Add "return" statement corresponding to abnormal condition on _ewk_frame_smart_add.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[KwangHyuk]

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.

[KwangHyuk]

Created attachment 102427 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 102427 [details]
Patch

LGTM

[Raphael Kubo da Costa (:rakuco)]

Please adjust the smart_add in ewk_view too.

[KwangHyuk]

> 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.

[KwangHyuk]

> 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

[KwangHyuk]

Hi Kubo and Leandro, Shall I get your review opinion for this patch ? :-)

[Leandro Pereira]

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 ***

[KwangHyuk]

> 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):

[Leandro Pereira]

Reopening bug, as I mistook ewk_frame with ewk_view.

[Leandro Pereira]

Comment on attachment 102427 [details]
Patch

Informal r+.

[Gyuyoung Kim]

Comment on attachment 102427 [details]
Patch

LGTM.

[Adam Roben (:aroben)]

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.

[KwangHyuk]

> 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. :-)

[KwangHyuk]

Created attachment 104504 [details]
Patch.

[WebKit Review Bot]

Comment on attachment 104504 [details]
Patch.

Clearing flags on attachment: 104504

Committed r93410: <http://trac.webkit.org/changeset/93410>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.