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

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied 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 337131: Patch

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




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

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

> Source/WTF/wtf/cocoa/CPUTimeCocoa.mm:88
>      RELEASE_ASSERT(ret == KERN_SUCCESS);
> -    mach_port_deallocate(mach_task_self(), threadPort);
> +    auto kr = mach_port_deallocate(mach_task_self(), threadPort);
> +    if (kr != KERN_SUCCESS) {
> +	   LOG_ERROR("mach_port_deallocate error for port %d: %s (%x)",
threadPort, mach_error_string(kr), kr);
> +	   if (kr == KERN_INVALID_RIGHT)
> +	       CRASH();
> +    }
>  
>      return Seconds(info.user_time.seconds + info.system_time.seconds) +
Seconds::fromMicroseconds(info.user_time.microseconds +
info.system_time.microseconds);

I think you can just use Mach::SendRight here directly, something like

auto threadPort = Mach::SendRight::adopt(mach_thread_self());
auto ret = thread_info(threadPort.sendRight(), ...);

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:171
>	   if (!(threadBasicInfo->flags & TH_FLAGS_IDLE))
>	       usage += threadBasicInfo->cpu_usage /
static_cast<float>(TH_USAGE_SCALE) * 100.0;
>  
> -	   mach_port_deallocate(mach_task_self(), threadList[i]);
> +	   auto kr = mach_port_deallocate(mach_task_self(), threadList[i]);
> +	   if (kr != KERN_SUCCESS) {
> +	       LOG_ERROR("mach_port_deallocate error for port %d: %s (%#x)",
threadList[i], mach_error_string(kr), kr);
> +	       if (kr == KERN_INVALID_RIGHT)
> +		   CRASH();
> +	   }

Looking at cpuUsage(), the memory (as well as the mach ports!) are leaked if
the call to thread_info fails. (Since it returns without deallocating ports or
memory).

I think you should add a function that returns the threads as a
Vector<MachSendRight>, something like

Vector<MachSendRight> getMachThreads();

then you can have cpuUsage call the function and you'll get the lifetime
management for free.

> Source/WebCore/platform/cocoa/MachSendRight.cpp:62
> +void deallocateSendOrReceiveRightSafely(mach_port_t port)

I don't think this function is ever used on receive rights (because they are
released using mach_port_mod_refs).