Bug 72017 - [EFL] Replace alloca to C++ new placement
Summary: [EFL] Replace alloca to C++ new placement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-10 05:36 PST by Grzegorz Czajkowski
Modified: 2012-01-05 07:47 PST (History)
5 users (show)

See Also:


Attachments
proposed patch (5.00 KB, patch)
2011-11-10 05:38 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
updated patch (5.34 KB, patch)
2011-12-01 06:36 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
updated patch (2.83 KB, patch)
2011-12-01 06:45 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
updated patch (5.34 KB, patch)
2011-12-01 06:47 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
proposed patch (1.72 KB, patch)
2012-01-03 07:14 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
updated patch (1.45 KB, patch)
2012-01-05 00:05 PST, Grzegorz Czajkowski
kling: review+
kling: commit-queue-
Details | Formatted Diff | Diff
updated patch (1.60 KB, patch)
2012-01-05 01:03 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2011-11-10 05:36:04 PST
Adds NULL checks before access to pointer which is initialized by alloca.

Adds NULL check to ewk_window_features_new_from_core after creating a new instance of WebCore's WindowFeatures.
It prevents unexpected result while calling ewk_window_features_unref if WindowFeatures is NULL.

Adds NULL check to ewk_cookies_get_all which returns list of cookies.
Cookie won't be added If malloc returns NULL.
Comment 1 Grzegorz Czajkowski 2011-11-10 05:38:25 PST
Created attachment 114478 [details]
proposed patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-11-11 03:54:57 PST
Comment on attachment 114478 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114478&action=review

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:160
> +    Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>
> +        (alloca(sizeof(Edje_Message_Float_Set) + sizeof(float)));

I'm not a big fan of alloca myself, as it is not very portable. Now that you're on this part of the code, it'd be good to replace it with new+delete or a smart pointer.

> Source/WebKit/efl/ewk/ewk_cookies.cpp:104
> +        } else
> +            CRITICAL("Could not allocate Ewk_Cookie.");

Has this happened in your daily usage? I'm asking because every once in a while patches checking for memory allocations are sent and most of the time they are rejected for the same reason (there's not much point in checking for every allocation failure across WebKit/WebCore).

> Source/WebKit/efl/ewk/ewk_window_features.cpp:126
> +    if (!window_features->core) {
> +        CRITICAL("Could not allocate WebCore::WindowFeatures.");
> +        return 0;
> +    }

Ditto.
Comment 3 Grzegorz Czajkowski 2011-11-14 07:35:18 PST
(In reply to comment #2)
> (From update of attachment 114478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114478&action=review
> 
> > Source/WebCore/platform/efl/ScrollbarEfl.cpp:160
> > +    Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>
> > +        (alloca(sizeof(Edje_Message_Float_Set) + sizeof(float)));
> 
> I'm not a big fan of alloca myself, as it is not very portable. Now that you're on this part of the code, it'd be good to replace it with new+delete or a smart pointer.

Yes, you're right. As I know all new/delete and malloc/free calls will be replaced in one patch so it would be better to use new+delete here instead of alloca.

> 
> > Source/WebKit/efl/ewk/ewk_cookies.cpp:104
> > +        } else
> > +            CRITICAL("Could not allocate Ewk_Cookie.");
> 
> Has this happened in your daily usage? I'm asking because every once in a while patches checking for memory allocations are sent and most of the time they are rejected for the same reason (there's not much point in checking for every allocation failure across WebKit/WebCore).

No, it hasn't. But generally WebKit-EFL checks the most of allocation failure. And some patches have already been landed, for example
https://bugs.webkit.org/show_bug.cgi?id=64932
https://bugs.webkit.org/show_bug.cgi?id=65853
Comment 4 Grzegorz Czajkowski 2011-12-01 06:36:12 PST
Created attachment 117406 [details]
updated patch

Replaced alloca to new/delete in compliance with WebKit coding style.
Comment 5 Grzegorz Czajkowski 2011-12-01 06:45:51 PST
Created attachment 117408 [details]
updated patch

In the previous patch only pointer was freed but whole table should be deleted.
Comment 6 Grzegorz Czajkowski 2011-12-01 06:47:00 PST
Created attachment 117409 [details]
updated patch
Comment 7 Grzegorz Czajkowski 2011-12-02 06:20:53 PST
Hi Kubo,
I uploaded a new patch - alloca was changed to new/delete.
Could you review this patch again?
Comment 8 Raphael Kubo da Costa (:rakuco) 2011-12-02 08:14:35 PST
Comment on attachment 117409 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117409&action=review

I still think it is not needed to check for all these conditions. Speaking of other bug reports, https://bugs.webkit.org/show_bug.cgi?id=65408#c12 still makes a lot of sense (even though the patch ended up being committed).

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:162
> +    Edje_Message_Float_Set* message = 0;
> +    char* mem = 0;
> +    if (mem = new char[sizeof(Edje_Message_Float_Set) + sizeof(double)])
> +        message = new(mem) Edje_Message_Float_Set;

This ended up looking quite ugly due to Edje's brain-dead way of manipulating this data structure. My suggestion is to just use
  Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>(malloc(sizeof(Edje_Message_Float_set) + sizeof(double)));
  if (!message) {
    // yadda yadda
  }
and then free() message after sending it.

If a reviewer prefers the use of placement new here, I suggest the following:
  char* mem = new char[sizeof(Edje_Message_Float_Set) * sizeof(double)];
  Edje_Message_Float_Set* message = new(mem) Edje_Message_Float_Set;
in this case, I don't even think it makes sense to check if the allocation fails, as new never returns NULL.

> Source/WebKit/efl/ewk/ewk_cookies.cpp:104
> +        } else
> +            CRITICAL("Could not allocate Ewk_Cookie.");

I still wonder if this is really needed. Assuming the hypothetical case in which malloc fails in the middle of the loop, you nonetheless continue trying to allocate other Ewk_Cookies to add them to the list and then return. I don't think this helps the caller all that much.

> Source/WebKit/efl/ewk/ewk_window_features.cpp:126
> +    if (!window_features->core) {
> +        CRITICAL("Could not allocate WebCore::WindowFeatures.");
> +        return 0;
> +    }

new never returns NULL.
Comment 9 Grzegorz Czajkowski 2011-12-06 07:50:00 PST
(In reply to comment #8)
> (From update of attachment 117409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117409&action=review
> 
> I still think it is not needed to check for all these conditions. Speaking of other bug reports, https://bugs.webkit.org/show_bug.cgi?id=65408#c12 still makes a lot of sense (even though the patch ended up being committed).
> 
> > Source/WebCore/platform/efl/ScrollbarEfl.cpp:162
> > +    Edje_Message_Float_Set* message = 0;
> > +    char* mem = 0;
> > +    if (mem = new char[sizeof(Edje_Message_Float_Set) + sizeof(double)])
> > +        message = new(mem) Edje_Message_Float_Set;
> 
> This ended up looking quite ugly due to Edje's brain-dead way of manipulating this data structure. My suggestion is to just use
>   Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>(malloc(sizeof(Edje_Message_Float_set) + sizeof(double)));
>   if (!message) {
>     // yadda yadda
>   }
> and then free() message after sending it.
> 
> If a reviewer prefers the use of placement new here, I suggest the following:
>   char* mem = new char[sizeof(Edje_Message_Float_Set) * sizeof(double)];
>   Edje_Message_Float_Set* message = new(mem) Edje_Message_Float_Set;
> in this case, I don't even think it makes sense to check if the allocation fails, as new never returns NULL

Ok, I will skip NULL checks for pointers initialized by new. 

> 
> > Source/WebKit/efl/ewk/ewk_cookies.cpp:104
> > +        } else
> > +            CRITICAL("Could not allocate Ewk_Cookie.");
> 
> I still wonder if this is really needed. Assuming the hypothetical case in which malloc fails in the middle of the loop, you nonetheless continue trying to allocate other Ewk_Cookies to add them to the list and then return. I don't think this helps the caller all that much.


Do you prefer to stop loop in case of malloc failure and destroy (by free()) allocated memory? A similar solution in used in ewk_frame_source_get().

> 
> > Source/WebKit/efl/ewk/ewk_window_features.cpp:126
> > +    if (!window_features->core) {
> > +        CRITICAL("Could not allocate WebCore::WindowFeatures.");
> > +        return 0;
> > +    }
> 
> new never returns NULL.

Ok, thanks.
Comment 10 Raphael Kubo da Costa (:rakuco) 2011-12-06 08:02:50 PST
(In reply to comment #9)
> (In reply to comment #8)
> > > Source/WebKit/efl/ewk/ewk_cookies.cpp:104
> > > +        } else
> > > +            CRITICAL("Could not allocate Ewk_Cookie.");
> > 
> > I still wonder if this is really needed. Assuming the hypothetical case in which malloc fails in the middle of the loop, you nonetheless continue trying to allocate other Ewk_Cookies to add them to the list and then return. I don't think this helps the caller all that much.
> 
> Do you prefer to stop loop in case of malloc failure and destroy (by free()) allocated memory?

Well, I'm generally against the idea of checking all those allocations :) You will end up doing a lot more work for a hypothetical situation in which you are already pretty much screwed up.

> A similar solution in used in ewk_frame_source_get().

Sorry, I couldn't find similar code in that function.
Comment 11 Grzegorz Czajkowski 2012-01-03 07:14:35 PST
Created attachment 120943 [details]
proposed patch

As a conclusion of above discussion NULL checking isn't needed in these cases.
I only replaced calling alloca to C++ new placement for three reasons:
1. alloca it is not very portable.
2. NULL checking isn't required as new never returns NULL,
3. in compliance with WebKit coding style.
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-01-04 09:14:38 PST
Should be OK, albeit ugly :)
Comment 13 Grzegorz Czajkowski 2012-01-05 00:05:52 PST
Created attachment 121226 [details]
updated patch

Buffer is created on stack so we don't need to delete it.
Comment 14 Andreas Kling 2012-01-05 00:10:44 PST
Comment on attachment 121226 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121226&action=review

r=me

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:164
> -    Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>
> -        (alloca(sizeof(Edje_Message_Float_Set) + sizeof(float)));
> +    char buffer[sizeof(Edje_Message_Float_Set) + sizeof(double)];
> +    Edje_Message_Float_Set* message = reinterpret_cast<Edje_Message_Float_Set*>(buffer);

Please note in the ChangeLog why you are changing from float to double.
Comment 15 Grzegorz Czajkowski 2012-01-05 01:03:05 PST
Created attachment 121235 [details]
updated patch

Added reason of changing float to double to ChangeLog.
Patch for landing.
Comment 16 WebKit Review Bot 2012-01-05 07:47:17 PST
Comment on attachment 121235 [details]
updated patch

Clearing flags on attachment: 121235

Committed r104163: <http://trac.webkit.org/changeset/104163>
Comment 17 WebKit Review Bot 2012-01-05 07:47:23 PST
All reviewed patches have been landed.  Closing bug.