Summary: | Consistently use RunLoop::isMain() in WebKit2 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | andersca, ap, beidson, bfulgham, commit-queue, darin, mitz, sam, simon.fraser, thorton, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Local Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=173738 https://bugs.webkit.org/show_bug.cgi?id=173722 https://bugs.webkit.org/show_bug.cgi?id=134900 https://bugs.webkit.org/show_bug.cgi?id=129135 |
||||||||||||
Attachments: |
|
Description
Daniel Bates
2017-06-22 16:47:36 PDT
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? Similar change made by Tim in: https://bugs.webkit.org/show_bug.cgi?id=134900 And by Enrica in: https://bugs.webkit.org/show_bug.cgi?id=129135 Created attachment 313683 [details]
Patch
Can we rename isMainThread to avoid this confusion? (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. Created attachment 313684 [details]
Patch
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). (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. Created attachment 313686 [details]
Patch
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. Created attachment 313744 [details]
Patch
Comment on attachment 313744 [details] Patch Clearing flags on attachment: 313744 Committed r218763: <http://trac.webkit.org/changeset/218763> All reviewed patches have been landed. Closing bug. |