Bug 215737

WebKit::encodeSharedBuffer() needlessly combines SharedBuffer data segments when creating a SharedMemory

<rdar://problem/67568099>

Created attachment 407020 [details]
Patch

Comment on attachment 407020 [details]
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.

Created attachment 407025 [details]
Patch for landing

(In reply to Darin Adler from comment #5)
> Comment on attachment 407020 [details]
> 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?

Yeah. I've updated the ChangeLog with better words.

Committed r266014: <https://trac.webkit.org/changeset/266014>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407025 [details].