Bug 179800 – Use RunLoop and Mode from NetworkingContext if they are given

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Always call performSelectorOnMainThread if we call callOnMainThread with a valid SchedulePairHashSet

[Alex Christensen]

Created attachment 327119 [details]
Patch

[Geoffrey Garen]

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.

[Geoffrey Garen]

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.

[Alex Christensen]

Created attachment 327131 [details]
Patch

[Geoffrey Garen]

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.

[Alex Christensen]

https://trac.webkit.org/changeset/224952/webkit

I'll do Geoff's suggested renaming and cleanups in a followup patch

[Radar WebKit Bug Importer]

<rdar://problem/35604879>

[Alex Christensen]

Cleaning up in https://bugs.webkit.org/show_bug.cgi?id=179809

[Ryan Haddad]

This test introduced 80+ LayoutTest failures/timeouts on WK1 bots:

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20%28Tests%29/builds/6192

[Ryan Haddad]

(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

[Matt Lewis]

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

[Ryan Haddad]

Reverted r224952 for reason:

This change introduced LayoutTest failures on WK1.

Committed r224969: <https://trac.webkit.org/changeset/224969>

[Ryan Haddad]

(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

[Alex Christensen]

Created attachment 327206 [details]
Patch

[Geoffrey Garen]

Does this patch need review?

[Alex Christensen]

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.

[Alex Christensen]

http://trac.webkit.org/r224982