Bug 186171 – CompletionHandlers should be called on their creation threads

• Component: New Bugs
• Status: NEW

[Alex Christensen]

CompletionHandlers should be called on their creation threads

[Alex Christensen]

Created attachment 341719 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Michael Catanzaro]

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.

[Alex Christensen]

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

[Michael Catanzaro]

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

[Alex Christensen]

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

[Michael Catanzaro]

(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!)

[Fujii Hironori]

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.

[Yusuke Suzuki]

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

[Fujii Hironori]

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

[Chris Dumez]

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.