[webkit-reviews] review granted: [Bug 215737] WebKit::encodeSharedBuffer() needlessly combines SharedBuffer data segments when creating a SharedMemory : [Attachment 407020] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 215737: WebKit::encodeSharedBuffer() needlessly combines SharedBuffer data
segments when creating a SharedMemory
https://bugs.webkit.org/show_bug.cgi?id=215737

Attachment 407020: Patch

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




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

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

> Source/WebKit/ChangeLog:11
> +	   * Platform/SharedMemory.cpp:
> +	   (WebKit::SharedMemory::copyBuffer): Changed iterate data segments
using a for loop rather
> +	   than repeated calls to SharedBuffer::getSomeData().

So this is just a coding style improvement, not the fix?

> Source/WebKit/ChangeLog:14
> +	   * Shared/WebCoreArgumentCoders.cpp:
> +	   (IPC::encodeSharedBuffer): Changed to call
SharedMemory::copyBuffer() rather than
> +	   SharedMemory::allocate() + memcpy.

And this is the fix?

> Source/WebKit/Platform/SharedMemory.cpp:44
>      char* const sharedMemoryPtr =
reinterpret_cast<char*>(sharedMemory->data());

This should be static_cast, not reinterpret_cast.

I also suggest using auto rather than char* const.

> Source/WebKit/Platform/SharedMemory.cpp:46
> +	   ASSERT(segmentEntry.beginPosition + segmentEntry.segment->size() <=
sharedMemory->size());

While it’s OK to assert this, seems unnecessary.

> Source/WebKit/Platform/SharedMemory.cpp:48
> +	   auto result = memcpy(sharedMemoryPtr + segmentEntry.beginPosition,
segmentEntry.segment->data(), segmentEntry.segment->size());
> +	   ASSERT_UNUSED(result, result == sharedMemoryPtr +
segmentEntry.beginPosition);

This seems strange in both the old and new code. Why look at the return value
from memcpy? Just not what we normally need to do.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:144
> +    auto sharedMemoryBuffer = SharedMemory::copyBuffer(*buffer);
>      sharedMemoryBuffer->createHandle(handle,
SharedMemory::Protection::ReadOnly);

Not sure about the failure handling here. For some reason copyBuffer returns
null when it fails, rather than doing an explicit crash. Seems like we should
be doing an explicit crash rather than just letting it crash "naturally" by
dereferencing null.

But this is no different from when we were calling allocate directly. As a
cleanup I suggest we tighten this up later and crash rather than returning
null. If we really need the "return null" behavior I suggest we add a
tryAllocate and a tryCopyBuffer.