Always call performSelectorOnMainThread if we call callOnMainThread with a valid SchedulePairHashSet
Created attachment 327119 [details] Patch
Comment on attachment 327119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327119&action=review > Source/WTF/wtf/MainThread.cpp:158 > + if (pairs || needToSchedule) > scheduleDispatchFunctionsOnMainThread(pairs); This fixes the bug in one direction but not the other: If items supplying pairs are scheduled, a callOnMainThread that does not supply pairs will not be scheduled. There's also another bug in this code: When we ultimately fire, we'll run tasks regardless of their pairs, including tasks that shouldn't run right now.
Maybe a good way to fix this is to say that, if you do not supply pairs, you get the function queue optimizations, but if you do supply pairs, you just invoke performSelectorOnMainThread directly without any amortization. We believe that use of pairs is rare and not performance-critical, so this might be the simplest solution. So, you would break out a helper function called scheduleDispatchFunctionOnMainThread(Function<void()>&&, SchedulePairHashSet*), and it it would do the Right Thing (TM). This would also make it trivial to honor not just the run loop mode but also the run loop.
Created attachment 327131 [details] Patch
r=me Can we remove SchedulePairHashSet support from MainThread.h now? > Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:54 > +static void callOnMainThreadOrSchedule(Function<void()>&& function, SchedulePairHashSet* pairs) I don't think "OrSchedule" adds value to this name. In all cases, we end up scheduling something on a run loop. > Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:66 > + } else > + callOnMainThread(WTFMove(function)); I think this function would read more clearly if the !pairs case came first as an early return. Then the rest of the logic doesn't need indentation.
https://trac.webkit.org/changeset/224952/webkit I'll do Geoff's suggested renaming and cleanups in a followup patch
<rdar://problem/35604879>
Cleaning up in https://bugs.webkit.org/show_bug.cgi?id=179809
This test introduced 80+ LayoutTest failures/timeouts on WK1 bots: https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20%28Tests%29/builds/6192
(In reply to Ryan Haddad from comment #9) > This test introduced 80+ LayoutTest failures/timeouts on WK1 bots: > > https://build.webkit.org/builders/ > Apple%20Sierra%20Release%20WK1%20%28Tests%29/builds/6192 s/test/change
The test WebKitLegacy.ScheduleInRunLoop does pass as stated in the changelog, but it now intermittently times out on macOS Debug platforms: https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20(Tests)/builds/4058/steps/run-api-tests/logs/stdio https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20(Tests)/builds/4058
Reverted r224952 for reason: This change introduced LayoutTest failures on WK1. Committed r224969: <https://trac.webkit.org/changeset/224969>
(In reply to Ryan Haddad from comment #12) > Reverted r224952 for reason: > > This change introduced LayoutTest failures on WK1. > > Committed r224969: <https://trac.webkit.org/changeset/224969> Tests are green again after the rollout, so this was indeed the cause. https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK1%20%28Tests%29/builds/6360
Created attachment 327206 [details] Patch
Does this patch need review?
This is just a slight modification of the previous patch to only CFRunLoopPerformBlock if we're scheduled in a custom mode. It works with UIWebView, it works with the app that's causing this bug, and it works with the WebKit tests, unlike the previous attempt.
http://trac.webkit.org/r224982