Bug 173745 – Consistently use RunLoop::isMain() in WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Daniel Bates]

In bug #173738, we substituted RunLoop:Main() for WTF::isMainThread() only in file Source/WebKit2/UIProcess/GenericCallback.h with the explanation "We have verified locally that RunLoop::isMain() propertly (sic) return true in [in affected iOS apps] (while WTF::isMainThread() returns false)." It would be good to understand why WTF::isMainThread() can return the wrong result in iOS WebKit2 and fix it. If it is not feasible to fix it and we know that RunLoop:Main() always returns the correct answer (to the question whether the currently running thread is the main thread) then we should substitute RunLoop:Main() for all uses of WTF::isMainThread().

[Radar WebKit Bug Importer]

<rdar://problem/32936527>

[Chris Dumez]

The apps that were broken used both WK2 and WK1 I believe. I have not looked in details yet but my guess is that WTF::isMainThread() gets confused by the fact that there is a WebThread in the UIProcess?

[Chris Dumez]

Similar change made by Tim in:
https://bugs.webkit.org/show_bug.cgi?id=134900

[Chris Dumez]

And by Enrica in:
https://bugs.webkit.org/show_bug.cgi?id=129135

[Chris Dumez]

Created attachment 313683 [details]
Patch

[Simon Fraser (smfr)]

Can we rename isMainThread to avoid this confusion?

[Chris Dumez]

(In reply to Simon Fraser (smfr) from comment #6)
> Can we rename isMainThread to avoid this confusion?

Maybe. Although I'd rather do this in a separate patch. Any naming proposal? IsMainThreadOrWebThread?

However, For non-Cocoa ports, isMainThread() does the right thing and is accurately named. The issue is only with iOS WebKit1 and the WebThread.

[Chris Dumez]

Created attachment 313684 [details]
Patch

[Chris Dumez]

We recently had similar issues with using WebCore::Timer in the UIProcess. As a result, we'd want to add an assertion in the WebCore::Timer to make sure it is not called from the UIProcess. The issue is that we'd need a way to detect this accurately (and not break WebKit1 code running in the UIProcess).

Ideally, we'd add a similar assertion in WTF::isMainThread() (or make WTF::isMainThread() work reliably).

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> We recently had similar issues with using WebCore::Timer in the UIProcess.
> As a result, we'd want to add an assertion in the WebCore::Timer to make
> sure it is not called from the UIProcess. The issue is that we'd need a way
> to detect this accurately (and not break WebKit1 code running in the
> UIProcess).
> 
> Ideally, we'd add a similar assertion in WTF::isMainThread() (or make
> WTF::isMainThread() work reliably).

Note that this patch helps but does not protect again people using WTF::isMainThread() is the UIProcess again. It also doesn't take care of code that is in WebCore and potentially called from the UIProcess.

[Chris Dumez]

Created attachment 313686 [details]
Patch

[Brent Fulgham]

Comment on attachment 313686 [details]
Patch

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

Looks good. I was about to make this mistake in another patch! r=me.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:592
> +    bool calledOnMainThread = WTF::RunLoop::isMain();

Do we need WTF:: here? We don't use it elsewhere.

[Chris Dumez]

Created attachment 313744 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 313744 [details]
Patch

Clearing flags on attachment: 313744

Committed r218763: <http://trac.webkit.org/changeset/218763>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.