[webkit-reviews] review granted: [Bug 176310] Remove "malloc" and "free" use : [Attachment 319783] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 176310: Remove "malloc" and "free" use
https://bugs.webkit.org/show_bug.cgi?id=176310

Attachment 319783: Patch

https://bugs.webkit.org/attachment.cgi?id=319783&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 319783
  --> https://bugs.webkit.org/attachment.cgi?id=319783
Patch

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

Great general direction to get rid of malloc/free.

But I don’t think we need MallocPtr at any of these call sites. And I am
wondering why we have it at all. See comment below.

> Source/JavaScriptCore/API/JSWrapperMap.mm:79
> +    auto buffer = MallocPtr<char>::malloc(header + strlen(firstColon + 1) +
1);

I would not have used MallocPtr<char>::malloc(size) here.

- I would have suggested Vector<char, SOME_CONSTANT> here instead. Pluses: 1)
Avoids heap allocation entirely for common case. 2) Familiar idiom used many
places in WebKit. Minuses: 1) Uses more stack space than malloc. 2) Extra
overhead to keep track of size and capacity for resizing, which is wasteful in
cases where we do not resize. 3) Doesn’t build in type casting for in cases
where we need to allocate a certain number of bytes and cast to a pointer type
other than "char*". 4) May involve unwanted zeroing of bytes in some cases.

- My second choice would have been std::make_unique<char[]>(size). Pluses: 1)
Built into the standard library. Minuses: 1) Zeroes out the bytes.

I am not sure why WebKit needs the MallocPtr class unless there is are places
we definitely can’t use either of these. I did not check where we are already
using it to see why we didn’t use either of my alternatives here.

> Source/WTF/wtf/Assertions.cpp:107
> +	   auto buffer = MallocPtr<char>::malloc(length + 1);

Vector<char, SOME_CONSTANT>?

> Source/WTF/wtf/Assertions.cpp:114
>	   CFRelease(str);
>	   CFRelease(cfFormat);

If we are already touching this code to deal with object lifetimes, we should
consider changing these to RetainPtr.

> Source/WTF/wtf/Assertions.cpp:135
> +	       auto buffer = MallocPtr<char>::malloc(size);

Vector<char, SOME_CONSTANT>?

The way this code silently stops and logs nothing if the allocation fails is
something we do not need to preserve.

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:141
> +    auto linkedFonts = MallocPtr<WCHAR>::malloc(linkedFontsBufferSize);

Vector<WCHAR, SOME_CONSTANT>?

>> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:57
>> +	LOGFONT logFont { 0 };
> 
> There is code below that takes the address of this variable. Is that safe?

The old code seems to leak a LOGFONT on the heap every time the function is
called. It’s good to fix that if we are confident it was a bug and nothing was
relying on that. As far as I can tell from reading documentation it was indeed
a mistake and had no benefit.

I don’t think the { 0 } is needed to preserve the old behavior, by the way,
because malloc does not zero out the memory it allocates on the heap.

> Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:58
>      memcpy(logFont.lfFaceName,
m_name.charactersWithNullTermination().data(), sizeof(logFont.lfFaceName[0]) *
std::min<size_t>(static_cast<size_t>(LF_FACESIZE), 1 + m_name.length()));

If we were writing new code, I would suggest using wcscpy_s rather than memcpy
here, which would make the code less verbose and do the same thing. Not sure
it’s worth changing, though, because this does look correct.

> Source/WebCore/platform/graphics/win/FontPlatformDataWin.cpp:61
> +	   auto metrics = MallocPtr<OUTLINETEXTMETRICW>::malloc(bufferSize);

I would use Vector<char, SOME_CONSTANT> here and a typecast to
OUTLINETEXTMETRICW*.

> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:1050
> +    auto after = MallocPtr<char>::malloc(bufferLength); // large enough to
%-escape every character

Vector<char, SOME_CONSTANT>?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:283
> +	   auto headerBuffer = MallocPtr<char>::malloc(headerBufferLength);

Vector<char, SOME_CONSTANT>?