Bug 172775

Created attachment 311631 [details] Patch

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

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/

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.

Created attachment 311634 [details] Patch

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.

Created attachment 311640 [details] Patch for landing

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.

The commit-queue encountered the following flaky tests while processing attachment 311640 [details]: workers/wasm-hashset-many.html bug 172781 (author: sbarati@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 311640 [details] Patch for landing Clearing flags on attachment: 311640 Committed r217631: <http://trac.webkit.org/changeset/217631>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 172785

Created attachment 311717 [details] Patch for landing

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

(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.

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