Bug 61423 – [EFL] Eina_List out of memory handling

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Grzegorz Czajkowski]

If a new node for Eina_List can not be added/allocated there is no checking error code. In this case previously allocated node is returned and user get not complete Eina_List and any error information isn't returned by API. In the most cases we're adding a new node in for/while so if the error occurred then the next iterations are useless.

Macro checks error code and if it occurred then Eina_List is flushed and given value is returned.
I would like to know your an opinion about it.

[Grzegorz Czajkowski]

Created attachment 94747 [details]
proposed patch

[Gyuyoung Kim]

Comment on attachment 94747 [details]
proposed patch

Attachment 94747 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8734361

[Grzegorz Czajkowski]

Created attachment 94750 [details]
updated patch

[Raphael Kubo da Costa (:rakuco)]

Informal r- from my side: in case the OOM handler is called, the memory still won't be properly freed. As the kind of destruction calls varies depending on what kind of data you are freeing, the macro may end up looking ugly enough not to have its existence justified. IMO, these checks could be done in-place in each file, or you can just ignore the eina_list_append errors, which is done 99% of the time in EFL code.

> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:411
> +    EINA_LIST_OUT_OF_MEMORY_HANDLING_AND_RETURN(o->items)

You are just calling free() on the pointer, but you still need to call ewk_context_menu_item_free in order not to leak the memory allocated in the items inside the struct.

> Source/WebKit/efl/ewk/ewk_cookies.cpp:120
> +        EINA_LIST_OUT_OF_MEMORY_HANDLING_AND_RETURN(el, 0)

You are just calling free() on the pointer, but you are not freeing the memory allocated for the data in the struct it points to.

> Source/WebKit/efl/ewk/ewk_history.cpp:108
> +            EINA_LIST_OUT_OF_MEMORY_HANDLING_AND_RETURN(ret, 0)

You are just calling free() on the pointer, but you are not freeing the memory allocated for the data (or de-ref'ing the data when necessary), which is done in _ewk_history_item_free().

> Source/WebKit/efl/ewk/ewk_private.h:49
> +#define EINA_LIST_OUT_OF_MEMORY_HANDLING_AND_RETURN(list, ...)          \

The macro name looks a bit misleading, as you are not necessarily out of memory when there is an error.

> Source/WebKit/efl/ewk/ewk_view.cpp:4119
> +        EINA_LIST_OUT_OF_MEMORY_HANDLING_AND_RETURN(priv->popup.menu.items)

The same comment I made to your changes to ewk_contextmenu.cpp applies here.

[Leandro Pereira]

Comment on attachment 94750 [details]
updated patch

Setting r- since Kubo doesn't have editbugs permission.

[Grzegorz Czajkowski]

Thanks for review. Sounds reasonably to separate OOM handling for each function. What will you say of following scenario:

func() {

   eina_list_append();
   if (eina_error_get())
      goto out_of_memory_handling_func;

// Out of memory handling for func()
out_of_memory_handling_func:
    CRITICAL("%s", eina_error_msg_get(eina_error_get()));
    void* _tmp_data;
    EINA_LIST_FREE(list, _tmp_data)
       method_for_free_item(_tmp_data) // like ewk_cookies_free(_tmp_data), ewk_context_menu_item_free(_tmp_data)
    return ;
}

A similar solution was proposed in html saving feature for OOM (bug id: 55455)

(In reply to comment #4)
> Informal r- from my side: in case the OOM handler is called, the memory still won't be properly freed. As the kind of destruction calls varies depending on what kind of data you are freeing, the macro may end up looking ugly enough not to have its existence justified. IMO, these checks could be done in-place in each file, or you can just ignore the eina_list_append errors, which is done 99% of the time in EFL code.
> 
> > Source/WebKit/efl/ewk/ewk_contextmenu.cpp:411
> > +    EINA_LIST_OUT_OF_MEMORY_HANDLING_AND_RETURN(o->items)
> 
> You are just calling free() on the pointer, but you still need to call ewk_context_menu_item_free in order not to leak the memory allocated in the items inside the struct.
> 
> > Source/WebKit/efl/ewk/ewk_cookies.cpp:120
> > +        EINA_LIST_OUT_OF_MEMORY_HANDLING_AND_RETURN(el, 0)
> 
> You are just calling free() on the pointer, but you are not freeing the memory allocated for the data in the struct it points to.
> 
> > Source/WebKit/efl/ewk/ewk_history.cpp:108
> > +            EINA_LIST_OUT_OF_MEMORY_HANDLING_AND_RETURN(ret, 0)
> 
> You are just calling free() on the pointer, but you are not freeing the memory allocated for the data (or de-ref'ing the data when necessary), which is done in _ewk_history_item_free().
> 
> > Source/WebKit/efl/ewk/ewk_private.h:49
> > +#define EINA_LIST_OUT_OF_MEMORY_HANDLING_AND_RETURN(list, ...)          \
> 
> The macro name looks a bit misleading, as you are not necessarily out of memory when there is an error.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:4119
> > +        EINA_LIST_OUT_OF_MEMORY_HANDLING_AND_RETURN(priv->popup.menu.items)
> 
> The same comment I made to your changes to ewk_contextmenu.cpp applies here.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #6)
> Thanks for review. Sounds reasonably to separate OOM handling for each function. What will you say of following scenario:
> 
> func() {
> 
>    eina_list_append();
>    if (eina_error_get())
>       goto out_of_memory_handling_func;
> 
> // Out of memory handling for func()
> out_of_memory_handling_func:
>     CRITICAL("%s", eina_error_msg_get(eina_error_get()));
>     void* _tmp_data;
>     EINA_LIST_FREE(list, _tmp_data)
>        method_for_free_item(_tmp_data) // like ewk_cookies_free(_tmp_data), ewk_context_menu_item_free(_tmp_data)
>     return ;
> }
> 
> A similar solution was proposed in html saving feature for OOM (bug id: 55455)

That sounds better, indeed. Personally, I'd simply ignore errors which come from eina_list_append (or treat them all, which is infeasible), eina_list_append should not error out very often. Do you have some sort of setup which makes it fail more frequently than usual?

[Grzegorz Czajkowski]

(In reply to comment #7)
> (In reply to comment #6)
> > Thanks for review. Sounds reasonably to separate OOM handling for each function. What will you say of following scenario:
> > 
> > func() {
> > 
> >    eina_list_append();
> >    if (eina_error_get())
> >       goto out_of_memory_handling_func;
> > 
> > // Out of memory handling for func()
> > out_of_memory_handling_func:
> >     CRITICAL("%s", eina_error_msg_get(eina_error_get()));
> >     void* _tmp_data;
> >     EINA_LIST_FREE(list, _tmp_data)
> >        method_for_free_item(_tmp_data) // like ewk_cookies_free(_tmp_data), ewk_context_menu_item_free(_tmp_data)
> >     return ;
> > }
> > 
> > A similar solution was proposed in html saving feature for OOM (bug id: 55455)
> 
> That sounds better, indeed. Personally, I'd simply ignore errors which come from eina_list_append (or treat them all, which is infeasible), eina_list_append should not error out very often. Do you have some sort of setup which makes it fail more frequently than usual?

Generally WebKit-EFL checks return value from malloc (and friends functions). But it is done in simply cases only, for example ewk_view_paint_context_new, ewk_context_menu_item_new. If error occurred functions just return NULL.

It looks worse in cases where a new memory is created in iterations of loops. In the most cases the memory is added to Eina_List, for example ewk_cookies_get, ChromeClientEfl::runOpenPanel etc. I think we should accordingly check error status here too (from malloc, strdup, eina_list_append). As you mentioned in these cases return is not enough because it may affect memory leaks.

[Grzegorz Czajkowski]

Created attachment 96247 [details]
a new patch

This patch allows by given function to free all Eina_List's items and given object which has not been added to list yet.

[Lucas De Marchi]

Comment on attachment 96247 [details]
a new patch

OMG! This is ugly as hell! Please, don't do that!


Are you going to test each OOM condition? An interesting reading: http://rusty.ozlabs.org/?p=186. In this case, the following applies: "Never think to write malloc-fail-proof code without testing it thoroughly, otherwise you haven’t written malloc-fail-proof code".

If you really want to debug these situations, the best place to check for them is inside Eina (since you are talking about eina_list_append), not inside webkit.


For the reason pointed out above, informal r- on my part.

[Grzegorz Czajkowski]

(In reply to comment #10)
> (From update of attachment 96247 [details])
> 
> 
> Are you going to test each OOM condition?

It's already done but only in simply scenario. Do you really think that we should not check in this case?
When I try to skip NULL checking in loops my review was denied. I had to use goto instruction. Instead of that I would use ewk_util_oom_handling.

"Never think to write malloc-fail-proof code without testing it thoroughly, otherwise you haven’t written malloc-fail-proof code".

Yes, it was tested. I assigned 0 to some part of code in ewk_cookies_all_get().

> 
> If you really want to debug these situations, the best place to check for them is inside Eina (since you are talking about eina_list_append), not inside webkit.
> 

From Eina's documentation:
 list = eina_list_prepend(list, my_data);
 if (eina_error_get())
   {
     fprintf(stderr, "ERROR: Memory is low. List allocation failed.\n");
     exit(-1);
   
I think we do not want this inside Eina ;)

[Gustavo Sverzut Barbieri]

I doubt this will make any effect on WebKit due many of EFL code not checking for it.

Really, check Evas. It does use eina_list and no checks are done there, so if it does not break here, would break there.

All in all I'd agree new code to come with it, but patching such huge amount of stuff for that is something I don't like.

Also dislike ewk_util_oom_handling() thing.