Bug 221832 – Unexpected ASSERT when touch events are dispatched on the main thread

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

ASSERT when touch events are dispatched on the main thread

[Radar WebKit Bug Importer]

<rdar://problem/74283856>

[Dean Jackson]

Created attachment 420154 [details]
Patch

[EWS]

Committed r272799: <https://commits.webkit.org/r272799>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420154 [details].

[Ryan Haddad]

Reverted r272799 for reason:

Caused debug WK2 tests to exit early with an assertion failure

Committed r272816 (234052@main): <https://commits.webkit.org/234052@main>

[Alexey Proskuryakov]

Failures:

https://build.webkit.org/results/Apple-BigSur-Debug-WK2-Tests/r272815%20(126)/results.html

e.g.

stderr:
ASSERTION FAILED: m_shouldBeCalledOnMainThread == isMainThread()
/Volumes/Data/slave/bigsur-debug/build/WebKitBuild/Debug/usr/local/include/wtf/CompletionHandler.h(65) : Out WTF::CompletionHandler<void (const WTF::Optional<WebKit::SecItemResponseData> &)>::operator()(In...)

[Sam Weinig]

Since this was trying to fix a regression from r272558, I think we should probably just back out r272558 now instead.

[Dean Jackson]

Created attachment 420253 [details]
Patch

[Darin Adler]

Comment on attachment 420253 [details]
Patch

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

> Source/WTF/wtf/CompletionHandler.h:48
> +        // FIXME: The logic here is a bit confusing, because you can't say that you want the handler to be called
> +        // on the same thread it was constructed on, unless that was the main thread.

Should we add *that* feature instead of adding AnyThread?

[Dean Jackson]

Comment on attachment 420253 [details]
Patch

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

>> Source/WTF/wtf/CompletionHandler.h:48
>> +        // on the same thread it was constructed on, unless that was the main thread.
> 
> Should we add *that* feature instead of adding AnyThread?

I think so. I'm talking to Alex now.

Either way, the confusion comes from this class not being explicit that it will decide a handler should be called on the main thread if it was created on the main thread.

[Dean Jackson]

Comment on attachment 420253 [details]
Patch

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

>>> Source/WTF/wtf/CompletionHandler.h:48
>>> +        // on the same thread it was constructed on, unless that was the main thread.
>> 
>> Should we add *that* feature instead of adding AnyThread?
> 
> I think so. I'm talking to Alex now.
> 
> Either way, the confusion comes from this class not being explicit that it will decide a handler should be called on the main thread if it was created on the main thread.

Alex said "land it", so I'll leave it to him to clean this up.

(Another confusion comes because CompletionHandlers are nearly always constructed by assigning a function, and not passing the 2nd parameter to a constructor)

[Alex Christensen]

Comment on attachment 420253 [details]
Patch

Actually, I think I have an idea.

[Sam Weinig]

Can we just roll out the original if this is not going to land. The assert is really getting in the way.

[Alex Christensen]

Created attachment 420345 [details]
Patch

[Alex Christensen]

(In reply to Sam Weinig from comment #12)
> Can we just roll out the original if this is not going to land. The assert
> is really getting in the way.

Unfortunately a lot of code has landed on top of the original, which would require reverting several patches.

[Alex Christensen]

r272873