CompletionHandlers should be called on their creation threads
Created attachment 341719 [details] Patch
Comment on attachment 341719 [details] Patch Attachment 341719 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/7914250 New failing tests: fast/workers/worker-messageport-gc.html fast/workers/termination-with-port-messages.html http/tests/workers/worker-messageport.html workers/worker-to-worker.html fast/workers/worker-messageport.html fast/workers/worker-close-more.html workers/message-port-gc.html
Created attachment 341724 [details] Archive of layout-test-results from ews115 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 341719 [details] Patch Attachment 341719 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7914252 New failing tests: http/tests/storageAccess/deny-storage-access-under-opener.html http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-prevalent-subresource-requests.html http/tests/resourceLoadStatistics/remove-blocking-in-redirect.html http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-prevalent-subresource-redirects.html http/tests/resourceLoadStatistics/add-blocking-to-redirect.html http/tests/resourceLoadStatistics/partitioned-and-unpartitioned-cookie-with-partitioning-timeout.html
Created attachment 341726 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 341719 [details] Patch Attachment 341719 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7916748 New failing tests: http/tests/storageAccess/deny-storage-access-under-opener.html http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-prevalent-subresource-requests.html http/tests/resourceLoadStatistics/remove-blocking-in-redirect.html http/tests/resourceLoadStatistics/strip-referrer-to-origin-for-prevalent-subresource-redirects.html http/tests/resourceLoadStatistics/add-blocking-to-redirect.html http/tests/resourceLoadStatistics/partitioned-and-unpartitioned-cookie-with-partitioning-timeout.html
Created attachment 341736 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 341719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341719&action=review I see there are still some broken tests uncovered by the debug bot. > Source/WTF/wtf/CompletionHandler.h:33 > +#if !ASSERT_DISABLED > +#include <wtf/Threading.h> > +#endif Nit: I see very limited value in adding guards around headers except when they are strictly needed. I assume the time savings from not including the header in release builds is going to be negligible, and that eventually someone developing with debug builds is going to assume that the header is available and commit something that doesn't build in release mode. > Source/WTF/wtf/CompletionHandler.h:66 > + ASSERT_WITH_MESSAGE(m_creationThread == &Thread::current(), "Completion handler should be called on its creation thread. Creation %p Current %p", m_creationThread, &Thread::current()); Excellent, very good.... > Source/WTF/wtf/CompletionHandler.h:67 > + return std::exchange(m_function, nullptr)(std::forward<In>(in)...); This looks good too, but now that m_function can be expected to be nullptr if the completion handler is invoked twice, I think you should add a separate ASSERT_WITH_MESSAGE to detect such misuse. > Source/WTF/wtf/CompletionHandler.h:71 > - mutable WTF::Function<Out(In...)> m_function; > + Function<Out(In...)> m_function; Is it really worth changing? Adding mutable all over the place does not seem useful.
(In reply to Michael Catanzaro from comment #8) > Nit: I see very limited value in adding guards around headers except when > they are strictly needed. I assume the time savings from not including the > header in release builds is going to be negligible, and that eventually > someone developing with debug builds is going to assume that the header is > available and commit something that doesn't build in release mode. I agree. > > Source/WTF/wtf/CompletionHandler.h:67 > > + return std::exchange(m_function, nullptr)(std::forward<In>(in)...); > > This looks good too, but now that m_function can be expected to be nullptr > if the completion handler is invoked twice, I think you should add a > separate ASSERT_WITH_MESSAGE to detect such misuse. That behavior is not changed. > > Source/WTF/wtf/CompletionHandler.h:71 > > - mutable WTF::Function<Out(In...)> m_function; > > + Function<Out(In...)> m_function; > > Is it really worth changing? Adding mutable all over the place does not seem > useful. Yes, it makes it more const correct.
(In reply to Alex Christensen from comment #9) > > This looks good too, but now that m_function can be expected to be nullptr > > if the completion handler is invoked twice, I think you should add a > > separate ASSERT_WITH_MESSAGE to detect such misuse. > That behavior is not changed. I don't understand, you added a std::exchange, so m_function will now be set to nullptr after it is used, right? That seems good to me, because it can be used to detect if the CompletionHandler is improperly called multiple times. So seems like a good opportunity to take advantage of that.
(In reply to Michael Catanzaro from comment #10) > (In reply to Alex Christensen from comment #9) > > > This looks good too, but now that m_function can be expected to be nullptr > > > if the completion handler is invoked twice, I think you should add a > > > separate ASSERT_WITH_MESSAGE to detect such misuse. > > That behavior is not changed. > > I don't understand, you added a std::exchange, so m_function will now be set > to nullptr after it is used, right? That seems good to me, because it can be > used to detect if the CompletionHandler is improperly called multiple times. > So seems like a good opportunity to take advantage of that. We were already WTFMoving the function when we call it, which would make it nullptr. This just makes it more explicit, and it makes it so we can put it all on one line.
(In reply to Alex Christensen from comment #9) > > This looks good too, but now that m_function can be expected to be nullptr > > if the completion handler is invoked twice, I think you should add a > > separate ASSERT_WITH_MESSAGE to detect such misuse. > That behavior is not changed. Whoops, I'm blind: that assert already exists at the top of the function. (It is a good assert!)
Comment on attachment 341719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341719&action=review >> Source/WTF/wtf/CompletionHandler.h:66 >> + ASSERT_WITH_MESSAGE(m_creationThread == &Thread::current(), "Completion handler should be called on its creation thread. Creation %p Current %p", m_creationThread, &Thread::current()); > > Excellent, very good.... Strictly speaking, you need to lock a mutex or use an atomic to be called from other thread.
Comment on attachment 341719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341719&action=review > Source/WTF/wtf/CompletionHandler.h:73 > + Thread* m_creationThread = nullptr; Recommend to use Ref<Thread> / RefPtr<Thread> to keep the address of m_creationThread unique when comparing `m_creationThread == &Thread::current()`.
Comment on attachment 341719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341719&action=review >> Source/WTF/wtf/CompletionHandler.h:73 >> + Thread* m_creationThread = nullptr; > > Recommend to use Ref<Thread> / RefPtr<Thread> to keep the address of m_creationThread unique when comparing `m_creationThread == &Thread::current()`. IMHO, in realistic use cases, m_creationThread is alive when the comparing is executed. Even though m_creationThread would be destructed, in realistic use cases, it is safe to compare `m_creationThread == &Thread::current()`.
Comment on attachment 341719 [details] Patch Personally, I worry about this kind of change because of WorkQueue. On Cocoa, WorkQueues are based on dispatch_queues. When the dispatch queue is serial, tasks dispatched on the queue are guaranteed to run sequentially, one after another, but they are not guaranteed to run on the same thread. As a matter, they often run on different background thread, which is why we often have to disable threading checks in SQLDabatase. Some objects like ResourceLoadStatisticsPersistentStore / ResourceLoadStatisticsMemoryStore are always used from a background queue and rely extensively on CompletionHandlers. This usage is perfectly safe and yet would trip this new ASSERTION. As a result, I do not believe that adding such assertion is OK / feasible.