[WK2][iOS] callOnMainThread() from main thread sometimes results in ASSERTs.

[Jer Noble]

[WK2] callOnMainThread() from main thread sometimes results in ASSERTs.

[Jer Noble]

Created attachment 221791 [details]
Patch

[Jer Noble]

Too many commas in the ChangeLog. Will fix.

[Jer Noble]

Comment on attachment 221791 [details]
Patch

ap, after talking to JoePeck about this patch on IRC, I think the new direction is to add a less ambiguous "callOnWebThread()" method.

[Jer Noble]

Created attachment 221808 [details]
Patch

[WebKit Commit Bot]

Attachment 221808 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/MainThread.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/MainThread.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 221808 [details]
Patch

The new patch crashes right and left on EWS, see <http://webkit-queues.appspot.com/results/5607835786280960>. Crashes are flaky, so EWS can't decide what to report.

[Jer Noble]

It looks like the behavior of WebThreadRun() is subtly different than callOnMainThread().  Specifically, if WebThreadRun() is called from the web thread, the task is run synchronously.  In callOnMainThread(), if the current thread is the main (or web) thread, the task is run in the next run-loop.

[Pratik Solanki]

I think we should make sure that callOnMainThread always calls on the WebThread for iOS. The fact that it dispatched on the main (UI) thread sometimes is a bug.

[Pratik Solanki]

Created attachment 222013 [details]
potential patch

Something like this may do the trick. Needs testing though.

[Joseph Pecoraro]

Comment on attachment 222013 [details]
potential patch

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

> Source/WTF/wtf/mac/MainThreadMac.mm:80
>      mainThreadEstablishedAsPthreadMain = false;
>      mainThreadPthread = pthread_self();
>      mainThreadNSThread = [[NSThread currentThread] retain];
> +    mainThreadRunLoop = CFRunLoopGetCurrent();

This is inside of !USE(WEB_THREAD). I think you will want to always initialize this variable, regardless of USE(WEB_THREAD) or not.

Which means, on iOS which has USE(WEB_THREAD), mainThreadRunLoop in the UIProcess, NetworkProcess, and WebProcess will be uninitialized until initializeWebThreadPlatform is called. On the WebProcess, that is probably pretty early on. In the UIProcess / NetworkProcess I don't know if that is ever called. I suspect it may not be called at all. In that case, having an uninitialized mainThreadRunLoop and calling callOnMainThread would trigger CFRunLoopAddTimer(NULL, ...).

There are UIProcess/NetworkProcess calls to "callOnMainThread":

    shell> cd Source/WebKit2; ack callOnMainThread
    NetworkProcess/NetworkResourceLoadScheduler.cpp
    170:        callOnMainThread(NetworkResourceLoadScheduler::removeScheduledLoaders, this);

    UIProcess/API/mac/WKPrintingView.mm
    400:        callOnMainThread(prepareDataForPrintingOnSecondaryThread, self);

Next, on iOS, we would want to verify that those NetworkProcess/UIProcess calls would be okay to happen on either the MainThread or WebThread. Since the "mainThreadRunLoop" could spontaneous change from MainThread to WebThread in the UIProcess if the UIProcess lazily initializes a WebThread (e.g. makes a WebKit1 WebView).

[Pratik Solanki]

Created attachment 222043 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 222043 [details]
Patch

This looks good to me. Does Benjamin Poulain or another person more familiar with the WebKit2 wanna check this as well?

[Pratik Solanki]

Comment on attachment 222043 [details]
Patch

Withdrawing this patch. There's more subtlety here.

[Pratik Solanki]

Joe and I discussed this and we think the first patch <https://bug-127370-attachments.webkit.org/attachment.cgi?id=221791> that Jer had is the right approach. callOnMainThread() always scheduled work on the web thread on iOS. That changed recently when we made changes to the definition of isMainThread() as part of upstreaming iOS code (r153950 made for bug 119644). We should use isWebThread() in this function. isWebThread() is defined to be the main thread on non-iOS code. And will do the right thing on iOS.

[Jer Noble]

Comment on attachment 221791 [details]
Patch

Un-obsoleting attachment #221791 [details].

[WebKit Commit Bot]

Comment on attachment 221791 [details]
Patch

Clearing flags on attachment: 221791

Committed r162862: <http://trac.webkit.org/changeset/162862>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.