173738 2017-06-22 15:12:07 -0700 isMainThread() assertions in CallbackMap are incorrectly failing on some iOS apps 2017-06-22 18:57:48 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=173680 https://bugs.webkit.org/show_bug.cgi?id=173745 InRadar P2 Normal --- 1 cdumez cdumez beidson commit-queue dbates rniwa sam webkit-bug-importer wenson_hsieh oldest_to_newest 1322077 0 cdumez 2017-06-22 15:12:07 -0700 isMainThread() assertions in CallbackMap are incorrectly failing on some iOS apps. 1322078 1 cdumez 2017-06-22 15:12:24 -0700 <rdar://problem/32923933> 1322081 2 313664 cdumez 2017-06-22 15:14:24 -0700 Created attachment 313664 Patch 1322089 3 cdumez 2017-06-22 15:24:10 -0700 Committed r218724: <http://trac.webkit.org/changeset/218724> 1322090 4 dbates 2017-06-22 15:33:24 -0700 We use WTF::isMainThread() all over the place in the code base, including throughout WebKit2. Do you plan to these to RunLoop::isMain()? Can we get rid of WTF::isMainThread()? 1322091 5 dbates 2017-06-22 15:33:55 -0700 (In reply to Daniel Bates from comment #4) > Do you plan to these to RunLoop::isMain()? *Do you plan to update these to use RunLoop::isMain()? 1322095 6 cdumez 2017-06-22 15:41:03 -0700 (In reply to Daniel Bates from comment #5) > (In reply to Daniel Bates from comment #4) > > Do you plan to these to RunLoop::isMain()? > > *Do you plan to update these to use RunLoop::isMain()? Given the apps that were impacted and the implementation of isMainThread(), we suspect the issue has to do with mixing WebKit1 and WebKit2 in the UIProcess. So yes, I think we should update assertions running in the UIProcess to use RunLoop::isMain() rather than isMainThread(). 1322107 7 dbates 2017-06-22 16:09:14 -0700 (In reply to Chris Dumez from comment #6) > (In reply to Daniel Bates from comment #5) > > (In reply to Daniel Bates from comment #4) > > > Do you plan to these to RunLoop::isMain()? > > > > *Do you plan to update these to use RunLoop::isMain()? > > Given the apps that were impacted and the implementation of isMainThread(), > we suspect the issue has to do with mixing WebKit1 and WebKit2 in the > UIProcess. > > So yes, I think we should update assertions running in the UIProcess to use > RunLoop::isMain() rather than isMainThread(). "think"? Is RunLoop:isMain() the solution or not? If it is then please substitute RunLoop:isMain() in all UIProcess code that uses WTF::isMainThread(). If we cannot do such substitution mechanically then please explain how to interpret the second sentence of your ChangeLog message. 1322108 8 cdumez 2017-06-22 16:12:01 -0700 (In reply to Daniel Bates from comment #7) > (In reply to Chris Dumez from comment #6) > > (In reply to Daniel Bates from comment #5) > > > (In reply to Daniel Bates from comment #4) > > > > Do you plan to these to RunLoop::isMain()? > > > > > > *Do you plan to update these to use RunLoop::isMain()? > > > > Given the apps that were impacted and the implementation of isMainThread(), > > we suspect the issue has to do with mixing WebKit1 and WebKit2 in the > > UIProcess. > > > > So yes, I think we should update assertions running in the UIProcess to use > > RunLoop::isMain() rather than isMainThread(). > > "think"? Is RunLoop:isMain() the solution or not? If it is then please > substitute RunLoop:isMain() in all UIProcess code that uses > WTF::isMainThread(). If we cannot do such substitution mechanically then > please explain how to interpret the second sentence of your ChangeLog > message. What I said, upon local testing RunLoop::isMain() was returning the correct result but WTF::isMainThread() wasn't. My only speculation is about why WTF::isMainThread() does not return the correct result. That part I do not know for sure. My priority was to unbreak those apps. A mechanical change in WebKit2 sounds fine. 1322109 9 cdumez 2017-06-22 16:14:53 -0700 (In reply to Daniel Bates from comment #7) > (In reply to Chris Dumez from comment #6) > > (In reply to Daniel Bates from comment #5) > > > (In reply to Daniel Bates from comment #4) > > > > Do you plan to these to RunLoop::isMain()? > > > > > > *Do you plan to update these to use RunLoop::isMain()? > > > > Given the apps that were impacted and the implementation of isMainThread(), > > we suspect the issue has to do with mixing WebKit1 and WebKit2 in the > > UIProcess. > > > > So yes, I think we should update assertions running in the UIProcess to use > > RunLoop::isMain() rather than isMainThread(). > > "think"? Is RunLoop:isMain() the solution or not? If it is then please > substitute RunLoop:isMain() in all UIProcess code that uses > WTF::isMainThread(). If we cannot do such substitution mechanically then > please explain how to interpret the second sentence of your ChangeLog > message. FYI, you make it really hard for foreigners like me to talk to you if you're going to pick on a given wording like that. 1322110 10 dbates 2017-06-22 16:26:33 -0700 (In reply to Chris Dumez from comment #9) > (In reply to Daniel Bates from comment #7) > > (In reply to Chris Dumez from comment #6) > > > (In reply to Daniel Bates from comment #5) > > > > (In reply to Daniel Bates from comment #4) > > > > > Do you plan to these to RunLoop::isMain()? > > > > > > > > *Do you plan to update these to use RunLoop::isMain()? > > > > > > Given the apps that were impacted and the implementation of isMainThread(), > > > we suspect the issue has to do with mixing WebKit1 and WebKit2 in the > > > UIProcess. > > > > > > So yes, I think we should update assertions running in the UIProcess to use > > > RunLoop::isMain() rather than isMainThread(). > > > > "think"? Is RunLoop:isMain() the solution or not? If it is then please > > substitute RunLoop:isMain() in all UIProcess code that uses > > WTF::isMainThread(). If we cannot do such substitution mechanically then > > please explain how to interpret the second sentence of your ChangeLog > > message. > > FYI, you make it really hard for foreigners like me to talk to you if you're > going to pick on a given wording like that. How do you suggest a person interpret your wording? I mean, the only available information that the general public has is what is written in the comments in this bug and in the ChangeLog/commit message. Maybe it would be better to discuss this over a different medium? 1322125 11 dbates 2017-06-22 16:51:42 -0700 (In reply to Chris Dumez from comment #8) > (In reply to Daniel Bates from comment #7) > > (In reply to Chris Dumez from comment #6) > > > (In reply to Daniel Bates from comment #5) > > > > (In reply to Daniel Bates from comment #4) > > > > > Do you plan to these to RunLoop::isMain()? > > > > > > > > *Do you plan to update these to use RunLoop::isMain()? > > > > > > Given the apps that were impacted and the implementation of isMainThread(), > > > we suspect the issue has to do with mixing WebKit1 and WebKit2 in the > > > UIProcess. > > > > > > So yes, I think we should update assertions running in the UIProcess to use > > > RunLoop::isMain() rather than isMainThread(). > > > > "think"? Is RunLoop:isMain() the solution or not? If it is then please > > substitute RunLoop:isMain() in all UIProcess code that uses > > WTF::isMainThread(). If we cannot do such substitution mechanically then > > please explain how to interpret the second sentence of your ChangeLog > > message. > > What I said, upon local testing RunLoop::isMain() was returning the correct > result but WTF::isMainThread() wasn't. > > My only speculation is about why WTF::isMainThread() does not return the > correct result. That part I do not know for sure. My priority was to unbreak > those apps. > > A mechanical change in WebKit2 sounds fine. Filed bug #173745 to investigate WTF::isMainThread() and/or substitute RunLoop:isMain() for uses of WTF::isMainThread(). 1322162 12 dbates 2017-06-22 18:57:48 -0700 (In reply to Chris Dumez from comment #9) > FYI, you make it really hard for foreigners like me to talk to you if you're > going to pick on a given wording like that. This has nothing to do with you being a foreigner. I just wanted to clarify whether using RunLoop:isMain() is the correct solution to fix this bug (as opposed to fixing WTF::isMainThread() or some other solution) as well as ascertain when can we expect to replace uses of WTF::isMainThread() with RunLoop:isMain() throughout our codebase assuming using RunLoop:isMain() is the correct solution. We now have bug #173745 tracking this clarification/discussion. 313664 2017-06-22 15:14:24 -0700 2017-06-22 15:27:55 -0700 Patch bug-173738-20170622151424.patch text/plain 2269 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjE4NzEzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMGM4ZDdkMThhNzJkZThm NTg1YzY4ZGQyNmRkMGFhM2NkMmFlODYxMy4uZjZmNDMzZjk2ZmVlOWRjMDc3OWYyMjVmYTk2YzRh ZDgzYjJiMjhlZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDE3LTA2LTIyICBDaHJp cyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CisKKyAgICAgICAgaXNNYWluVGhyZWFkKCkgYXNz ZXJ0aW9ucyBpbiBDYWxsYmFja01hcCBhcmUgaW5jb3JyZWN0bHkgZmFpbGluZyBvbiBzb21lIGlP UyBhcHBzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x NzM3MzgKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzMyOTIzOTMzPgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBhc3NlcnRpb25zIGhpdCBvbiBj ZXJ0YWluIGlPUyBhcHBzIGV2ZW4gdGhvdWdoIHRoZSBjcmFzaCB0cmFjZXMgY2xlYXJseQorICAg ICAgICBzaG93IHRoZW0gb24gdGhlIG1haW4gdGhyZWFkLiBXZSBoYXZlIHZlcmlmaWVkIGxvY2Fs bHkgdGhhdCBSdW5Mb29wOjppc01haW4oKQorICAgICAgICBwcm9wZXJ0bHkgcmV0dXJuIHRydWUg aW4gdGhpcyBjYXNlICh3aGlsZSBXVEY6OmlzTWFpblRocmVhZCgpIHJldHVybnMgZmFsc2UpLgor ICAgICAgICBVcGRhdGUgdGhlIGFzc2VydGlvbnMgdG8gdXNlIFJ1bkxvb3A6OmlzTWFpbigpIGlu c3RlYWQuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvR2VuZXJpY0NhbGxiYWNrLmg6CisgICAgICAg IChXZWJLaXQ6OkNhbGxiYWNrTWFwOjpwdXQpOgorICAgICAgICAoV2ViS2l0OjpDYWxsYmFja01h cDo6dGFrZSk6CisgICAgICAgIChXZWJLaXQ6OkNhbGxiYWNrTWFwOjppbnZhbGlkYXRlKToKKwog MjAxNy0wNi0yMiAgRGFuaWVsIEJhdGVzICA8ZGFiYXRlc0BhcHBsZS5jb20+CiAKICAgICAgICAg TWFrZSBGcmFtZUxvYWRSZXF1ZXN0IGEgbW92ZS1vbmx5IHR5cGUKZGlmZiAtLWdpdCBhL1NvdXJj ZS9XZWJLaXQyL1VJUHJvY2Vzcy9HZW5lcmljQ2FsbGJhY2suaCBiL1NvdXJjZS9XZWJLaXQyL1VJ UHJvY2Vzcy9HZW5lcmljQ2FsbGJhY2suaAppbmRleCBiZmVhYzk5YzA2ZTZjNmQxZTYyYjFkMTc4 ZGQ1ZThjMGMzNTVlMjQyLi5mNjEwODE3NzMyNDUwY2MyYTM4ZTQ2MTljOWE0OWRkZDNhNmYyZTVh IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvR2VuZXJpY0NhbGxiYWNrLmgK KysrIGIvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0dlbmVyaWNDYWxsYmFjay5oCkBAIC0xODIs NyArMTgyLDcgQEAgY2xhc3MgQ2FsbGJhY2tNYXAgewogcHVibGljOgogICAgIHVpbnQ2NF90IHB1 dChSZWY8Q2FsbGJhY2tCYXNlPiYmIGNhbGxiYWNrKQogICAgIHsKLSAgICAgICAgUkVMRUFTRV9B U1NFUlQoaXNNYWluVGhyZWFkKCkpOworICAgICAgICBSRUxFQVNFX0FTU0VSVChSdW5Mb29wOjpp c01haW4oKSk7CiAgICAgICAgIHVpbnQ2NF90IGNhbGxiYWNrSUQgPSBjYWxsYmFjay0+Y2FsbGJh Y2tJRCgpOwogICAgICAgICBSRUxFQVNFX0FTU0VSVChjYWxsYmFja0lEKTsKICAgICAgICAgUkVM RUFTRV9BU1NFUlQoIW1fbWFwLmNvbnRhaW5zKGNhbGxiYWNrSUQpKTsKQEAgLTIxMSw3ICsyMTEs NyBAQCBwdWJsaWM6CiAgICAgUmVmUHRyPFQ+IHRha2UodWludDY0X3QgY2FsbGJhY2tJRCkKICAg ICB7CiAgICAgICAgIFJFTEVBU0VfQVNTRVJUKGNhbGxiYWNrSUQpOwotICAgICAgICBSRUxFQVNF X0FTU0VSVChpc01haW5UaHJlYWQoKSk7CisgICAgICAgIFJFTEVBU0VfQVNTRVJUKFJ1bkxvb3A6 OmlzTWFpbigpKTsKICAgICAgICAgYXV0byBiYXNlID0gbV9tYXAudGFrZShjYWxsYmFja0lEKTsK ICAgICAgICAgaWYgKCFiYXNlKQogICAgICAgICAgICAgcmV0dXJuIG51bGxwdHI7CkBAIC0yMjEs NyArMjIxLDcgQEAgcHVibGljOgogCiAgICAgdm9pZCBpbnZhbGlkYXRlKENhbGxiYWNrQmFzZTo6 RXJyb3IgZXJyb3IpCiAgICAgewotICAgICAgICBSRUxFQVNFX0FTU0VSVChpc01haW5UaHJlYWQo KSk7CisgICAgICAgIFJFTEVBU0VfQVNTRVJUKFJ1bkxvb3A6OmlzTWFpbigpKTsKICAgICAgICAg aW52YWxpZGF0ZUNhbGxiYWNrTWFwKG1fbWFwLCBlcnJvcik7CiAgICAgfQogCg==