[webkit-reviews] review granted: [Bug 171865] Use Mach exceptions instead of signals where possible : [Attachment 309908] 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 171865: Use Mach exceptions instead of signals where possible
https://bugs.webkit.org/show_bug.cgi?id=171865

Attachment 309908: Patch

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




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

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

r=me with fixes.

> Source/WTF/ChangeLog:11
> +	   that port in a thread. When another thread raises a exception (say

/a exception/an exception/

> Source/WTF/ChangeLog:20
> +	   simply suspending the thread then running the message at that

/thread then/thread, and then/.

> Source/WTF/ChangeLog:23
> +	   You can read more abount mach exceptions here:

/abount/about/

> Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:93
> +bool dispatchTerminateCallbackCalled = false;
> +static bool dispatchTermitateCallback(JSContextRef, void*)
> +{
> +    dispatchTerminateCallbackCalled = true;
> +    return true;
> +}

You should enclose this in #if HAVE(MACH_EXCEPTIONS) too because the static
function dispatchTermitateCallback is not used otherwise.  If we ever force
HAVE(MACH_EXCEPTIONS) to be false for a test build, we'll get a build failure
due to this.

> Source/JavaScriptCore/runtime/Options.h:423
> +    v(bool, useMachForExceptions, true, Normal, "Use mach exceptions rather
than signals to handle faults and pass thread messages. (This does nothing on
platforms without mach)") \

Should default to false.  We override it to true only when
HAVE(MACH_EXCEPTIONS) in overrideDefaults().

> Source/WTF/wtf/ThreadMessage.cpp:94
> +	   installSignalHandler(signal, [] (Signal, SigInfo&,
PlatformRegisters& context) {

Rename context to registers.

> Source/WTF/wtf/ThreadMessage.cpp:104
> +		   data->message(context);

Ditto.

> Source/WTF/wtf/ThreadMessage.cpp:214
> +#if HAVE(MACH_EXCEPTIONS)
> +    if (useMach)
> +	   return sendMessageMach(thread, message);
> +#endif
> +    return sendMessageSignal(thread, message);

suggestion: sendMessageViaMach instead of sendMessageMach, and
sendMessageViaSignal instead of sendMessageSignal?

> Source/WTF/wtf/threads/Signals.cpp:63
> +// You can read more abount mach exceptions here:

typo: /abount/about/.

> Source/WTF/wtf/threads/Signals.cpp:196
> +	   switch (handlerResult) {
> +	   case SignalAction::Handled:
> +	       didHandle = true;
> +	       break;
> +	   default:
> +	       break;
> +	   }

You can just say:
    didHandle = didHandle || (handlerResult == SignalAction::Handled);

> Source/WTF/wtf/threads/Signals.cpp:232
> +void registerThread(Thread* thread)

Let's name this "registerThreadForMachExceptionHandling" so that it adds some
context in the client code on what the registration is for.

> Source/WTF/wtf/threads/Signals.cpp:241
> +void removeThread(Thread* thread)

small nit: I'd prefer "unregisterThread" to match "registerThread" simply for
symmetry.  So, let's name this "unregisterThreadForMachExceptionHandling".

> Source/WTF/wtf/threads/Signals.cpp:257
> +    if (signal == Signal::Bus && useMach)

We should get rid of Signal::Bus as an option that the client can select, and
force the signal version to make the Signal::Segv handler handle SIGBUS as
well.  For now, just add a FIXME with bug url to fix that later.  Maybe rename
Signal::Segv to Signal::BadAccess.

> Source/WTF/wtf/threads/Signals.h:107
> +using WTF::SigInfo;

Somewhere below this, you should add:
#if HAVE(MACH_EXCEPTIONS)
using WTF::registerThreadForMachExceptionHandling;
using WTF::unregisterThreadForMachExceptionHandling;
#endif

The webkit way is to not have to qualify WTF APIs with WTF:: in the client
code.  Also using more descriptive names make it clearer what the registration
is for in client code.