[webkit-reviews] review granted: [Bug 213160] Signal handlers should have a two phase installation. : [Attachment 401844] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 213160: Signal handlers should have a two phase installation.
https://bugs.webkit.org/show_bug.cgi?id=213160

Attachment 401844: Patch

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




--- Comment #11 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 401844
  --> https://bugs.webkit.org/attachment.cgi?id=401844
Patch

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

r=me with fixes for limiting what signals can be activated.

> Source/WTF/ChangeLog:13
> +	   LLDB does not like it when we single step while there are mach
exception
> +	   handlers installed because LLDB suspends all the non-active threads.
> +	   With Mach exception handlers installed, the OS will send a mach
message
> +	   to our exception handler port, which is different than the active
thread.
> +	   When this happens, the combination of LLDB and the process JSC is in
effectively
> +	   deadlock.

It's not clear to me how this patch fixes the LLDB deadlock?  At some point, we
will call setExceptionPort() for the signals we wish to handle.  How does
having delayed the call to setExceptionPort() help after that it does get
called?  Wouldn't LLDB also still deadlock then?  Or are you saying that you're
only reducing the probability of deadlock here and not actually fixing it
completely?  Can you comment and clarify this point in the ChangeLog.

Anyway, I think this patch is an improvement.

> Source/WTF/wtf/threads/Signals.cpp:243
> +    kern_return_t result = thread_set_exception_ports(thread.machThread(),
activeExceptions, handlers.exceptionPort, EXCEPTION_STATE |
MACH_EXCEPTION_CODES, MACHINE_THREAD_STATE);

Replace "activeExceptions" with "activeExceptions & handlers.addedExceptions"
here.

> Source/WTF/wtf/threads/Signals.cpp:-295
> -	   Config::AssertNotFrozenScope assertScope;

Please restore this Config::AssertNotFrozenScope.

> Source/WTF/wtf/threads/Signals.cpp:323
> -	   handlers.activeExceptions |= toMachMask(signal);
> +	   activeExceptions |= toMachMask(signal);

To ensure that we don't allow activating any handlers for signals that we
didn't explicitly call addSignalHandler() for, let's do the following:
1. In SignalHandlers::add(), please add the following:
	addedExceptions |= toMachMask(signal);
2. Here in activateSignalHandlersFor(), change this line to:
	activeExceptions |= (toMachMask(signal) & handlers.addedExceptions);

> Source/WTF/wtf/threads/Signals.h:112
>      exception_mask_t activeExceptions;

Rename this "activeExceptions" field to to "addedExceptions".