Bug 172775 – Use dispatch queues for mach exceptions

• Component: New Bugs
• Status: REOPENED

[Keith Miller]

Use dispatch queues for mach exceptions

[Keith Miller]

Created attachment 311631 [details]
Patch

[Keith Miller]

*** Bug 172003 has been marked as a duplicate of this bug. ***

[Mark Lam]

Comment on attachment 311631 [details]
Patch

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

> Source/WTF/wtf/threads/Signals.cpp:84
> +        // It's not clear that this needs to be the high priority queue but it should be rare and it might be
> +        // handling exceptions from high priority threads. Anyway, our handlers should be very fast anyway so it's
> +        // probably not the end of the world if we handle a low priority exception on a high priority queue.

I think it should be high priority because this is the mechanism for interrupting the main thread.  Otherwise, it may never interrupt the main thread if the main thread has a higher priority.

> Source/WTF/wtf/threads/Signals.cpp:111
> +            // We should never cancel our handler since it's a perminant thing.

typo: /perminant/permanent/

[Keith Miller]

Comment on attachment 311631 [details]
Patch

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

>> Source/WTF/wtf/threads/Signals.cpp:84
>> +        // probably not the end of the world if we handle a low priority exception on a high priority queue.
> 
> I think it should be high priority because this is the mechanism for interrupting the main thread.  Otherwise, it may never interrupt the main thread if the main thread has a higher priority.

I'm assuming you're talking about the thread messaging API. We don't interrupt the main thread with mach exceptions. We use mach_thread_suspend/resume.

>> Source/WTF/wtf/threads/Signals.cpp:111
>> +            // We should never cancel our handler since it's a perminant thing.
> 
> typo: /perminant/permanent/

fixed.

[Keith Miller]

Created attachment 311634 [details]
Patch

[Geoffrey Garen]

Comment on attachment 311634 [details]
Patch

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

r=me

> Source/WTF/wtf/threads/Signals.cpp:87
> +        RELEASE_ASSERT_WITH_MESSAGE(source, "We need to ensure our source was created");

dispatch_source_create doesn't use null as an error condition return value. So you should remove this assert.

> Source/WTF/wtf/threads/Signals.cpp:89
> +        dispatch_source_set_event_handler(source, ^{

You need to keep a pointer to your dispatch_source_t or it will appear to the leaks tool that it has leaked. I'd suggest capturing a pointer in your block:

static dispatch_source_t s_source = source;

It might also work just to do this:

UNUSED_PARAM(source);

> Source/WTF/wtf/threads/Signals.cpp:113
> +        dispatch_source_set_cancel_handler(source, ^{
> +            // We should never cancel our handler since it's a permanent thing.
> +            RELEASE_ASSERT_NOT_REACHED();
> +        });

No need for this.

[Keith Miller]

Created attachment 311640 [details]
Patch for landing

[Keith Miller]

Comment on attachment 311634 [details]
Patch

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

>> Source/WTF/wtf/threads/Signals.cpp:87
>> +        RELEASE_ASSERT_WITH_MESSAGE(source, "We need to ensure our source was created");
> 
> dispatch_source_create doesn't use null as an error condition return value. So you should remove this assert.

https://developer.apple.com/reference/dispatch/1385630-dispatch_source_create seems to disagree with you:

Return Value
A new dispatch source object or NULL if the dispatch source could not be created.

>> Source/WTF/wtf/threads/Signals.cpp:89
>> +        dispatch_source_set_event_handler(source, ^{
> 
> You need to keep a pointer to your dispatch_source_t or it will appear to the leaks tool that it has leaked. I'd suggest capturing a pointer in your block:
> 
> static dispatch_source_t s_source = source;
> 
> It might also work just to do this:
> 
> UNUSED_PARAM(source);

I'll try "UNUSED_PARAM(source);"

>> Source/WTF/wtf/threads/Signals.cpp:113
>> +        });
> 
> No need for this.

OK, I removed it.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 311640 [details]:

workers/wasm-hashset-many.html bug 172781 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 311640 [details]
Patch for landing

Clearing flags on attachment: 311640

Committed r217631: <http://trac.webkit.org/changeset/217631>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 172785

[Keith Miller]

Created attachment 311717 [details]
Patch for landing

[Keith Miller]

I think I figured out the issue. I think it was that we were calling set_state, while inside CFRunLoop which made things sad...

[Keith Miller]

(In reply to Keith Miller from comment #14)
> I think I figured out the issue. I think it was that we were calling
> set_state, while inside CFRunLoop which made things sad...

I just removed the set-state code since that was not used anyway.

[WebKit Commit Bot]

Comment on attachment 311717 [details]
Patch for landing

Rejecting attachment 311717 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 311717, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
 with fuzz 3.
patching file Source/WTF/wtf/ThreadMessage.cpp
Hunk #1 FAILED at 169.
1 out of 1 hunk FAILED -- saving rejects to file Source/WTF/wtf/ThreadMessage.cpp.rej
patching file Source/WTF/wtf/threads/Signals.cpp
Hunk #1 FAILED at 39.
Hunk #2 FAILED at 73.
Hunk #3 FAILED at 194.
3 out of 3 hunks FAILED -- saving rejects to file Source/WTF/wtf/threads/Signals.cpp.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/3854480