[webkit-reviews] review granted: [Bug 184202] Failures from mach port reference handling should be fatal : [Attachment 337177] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 184202: Failures from mach port reference handling should be fatal
https://bugs.webkit.org/show_bug.cgi?id=184202

Attachment 337177: Patch

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




--- Comment #18 from Anders Carlsson <andersca at apple.com> ---
Comment on attachment 337177
  --> https://bugs.webkit.org/attachment.cgi?id=337177
Patch

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

Not entirely sure why you moved MachSendRight to WTF - it doesn't seem to be
used by anything below WebCore. Looks good otherwise.

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:142
> +static Vector<MachSendRight> getMachThreads()

I think you should call this threadSendrights() - a get prefix usually implies
a boolean return value + out parameter.

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:148
> +	   return Vector<MachSendRight>();

Can just return { }; here I think.

>
Source/WebCore/platform/graphics/avfoundation/objc/VideoFullscreenLayerManagerO
bjC.mm:130
>	       mach_port_t fencePort = [oldContext createFencePort];
>	       [newContext setFencePort:fencePort];
> -	       mach_port_deallocate(mach_task_self(), fencePort);
> +	       deallocateSendRightSafely(fencePort);

Can just do

auto fencePort = MachSendRight::adopt([oldContext createFencePort] here.

> Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp:71
> +    if (m_port) {
> +	   auto kr = mach_port_deallocate(mach_task_self(), m_port);
> +	   if (kr != KERN_SUCCESS) {
> +#if RELEASE_LOG_DISABLED
> +	       ASSERT_UNUSED(kr, kr == KERN_SUCCESS);
> +#else
> +	       RELEASE_LOG_ERROR(VirtualMemory, "%p -
SharedMemory::Handle::clear: Failed to deallocate port. %{public}s (%x)", this,
mach_error_string(kr), kr);
> +	       ASSERT_NOT_REACHED();
> +#endif
> +	   }
> +    }

Can't this use deallocateSendRightSafely?