227607 2021-07-01 20:10:52 -0700 WebPageProxy::setAppHighlightsVisibility might send message from a background thread, ASSERTing 2021-07-02 10:57:21 -0700 1 1 1 Unclassified WebKit WebKit Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 beidson beidson achristensen cdumez darin ggaren thorton oldest_to_newest 1774347 0 beidson 2021-07-01 20:10:52 -0700 WebPageProxy::setAppHighlightsVisibility might send message from a background thread, ASSERTing <rdar://80056481> 1774349 1 432765 beidson 2021-07-01 20:13:58 -0700 Created attachment 432765 Patch 1774355 2 cdumez 2021-07-01 21:19:50 -0700 Can’t we just send the radar to the client? I worry about us opening the door to clients calling ours APIs on non main threads. 1774356 3 beidson 2021-07-01 21:25:28 -0700 (In reply to Chris Dumez from comment #2) > Can’t we just send the radar to the client? I worry about us opening the > door to clients calling ours APIs on non main threads. The client is *US* 1774357 4 beidson 2021-07-01 21:27:58 -0700 (In reply to Brady Eidson from comment #3) > (In reply to Chris Dumez from comment #2) > > Can’t we just send the radar to the client? I worry about us opening the > > door to clients calling ours APIs on non main threads. > > The client is *US* (It's not really us, but it's our own block we're passing to another system framework and it's expected that block gets called on a non-main thread) 1774358 5 beidson 2021-07-01 21:28:17 -0700 (In reply to Brady Eidson from comment #4) > (In reply to Brady Eidson from comment #3) > > (In reply to Chris Dumez from comment #2) > > > Can’t we just send the radar to the client? I worry about us opening the > > > door to clients calling ours APIs on non main threads. > > > > The client is *US* > > (It's not really us, but it's our own block we're passing to another system > framework and it's expected that block gets called on a non-main thread) So, if you prefer, I could fix this inside that block instead. 1774359 6 cdumez 2021-07-01 21:47:09 -0700 (In reply to Brady Eidson from comment #5) > (In reply to Brady Eidson from comment #4) > > (In reply to Brady Eidson from comment #3) > > > (In reply to Chris Dumez from comment #2) > > > > Can’t we just send the radar to the client? I worry about us opening the > > > > door to clients calling ours APIs on non main threads. > > > > > > The client is *US* > > > > (It's not really us, but it's our own block we're passing to another system > > framework and it's expected that block gets called on a non-main thread) > > So, if you prefer, I could fix this inside that block instead. Oh, I see. Then yes, I think it would look nicer to fix it at the call site rather than doing a trampoline in WebPageProxy. 1774360 7 432765 cdumez 2021-07-01 22:06:21 -0700 Comment on attachment 432765 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432765&action=review > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:591 > + callOnMainRunLoop([protectedThis = makeRef(*this), appHighlightsVisibility] { As I mentioned earlier, I think this should be fixed at the call site. Also, I looked at the call site in WebPageProxy::setUpHighlightsObserver() and I am not convinced this is safe. What ensures that setAppHighlightsVisibility() is not getting called on the background thread while the WebPageProxy is in the middle of destruction on the main thread? Your call to makeRef(*this) here would try to ref the page even though its destructor is running. Is there something I missed that makes sure this cannot happen? 1774363 8 thorton 2021-07-01 22:45:04 -0700 (In reply to Chris Dumez from comment #7) > Comment on attachment 432765 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=432765&action=review > > > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:591 > > + callOnMainRunLoop([protectedThis = makeRef(*this), appHighlightsVisibility] { > > As I mentioned earlier, I think this should be fixed at the call site. I said the same to Brady elsewhere and he promised to make that change (and then I r+’d anyway :| ) 1774488 9 beidson 2021-07-02 09:13:30 -0700 (In reply to Chris Dumez from comment #7) > Comment on attachment 432765 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=432765&action=review > > > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:591 > > + callOnMainRunLoop([protectedThis = makeRef(*this), appHighlightsVisibility] { > > As I mentioned earlier, I think this should be fixed at the call site. > > Also, I looked at the call site in WebPageProxy::setUpHighlightsObserver() > and I am not convinced this is safe. What ensures that > setAppHighlightsVisibility() is not getting called on the background thread > while the WebPageProxy is in the middle of destruction on the main thread? > Your call to makeRef(*this) here would try to ref the page even though its > destructor is running. Is there something I missed that makes sure this > cannot happen? Right. Turns out this code is already thread unsafe. Fixing now with some weakptr and Function<>-foo 1774490 10 432803 beidson 2021-07-02 09:26:14 -0700 Created attachment 432803 Patch reviewed on Slack 1774503 11 ews-feeder 2021-07-02 10:57:19 -0700 Committed r279509 (239361@main): <https://commits.webkit.org/239361@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 432803. 432765 2021-07-01 20:13:58 -0700 2021-07-01 21:55:39 -0700 Patch bug-227607-20210701201358.patch text/plain 1675 beidson U3VidmVyc2lvbiBSZXZpc2lvbjogMjc5NDY3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDM2YjhlODY0YjJjNzViNTU2 MGZiZDQ4ZjljOGFjOTZjNzgzNGRkMTUuLjkyMGYzZDgwNGExNWVmMDdiOThhNDViNmNiZDc2NWM3 ZDg2MWE4Y2MgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMjEtMDctMDEgIEJyYWR5IEVp ZHNvbiAgPGJlaWRzb25AYXBwbGUuY29tPgorCisgICAgICAgIFdlYlBhZ2VQcm94eTo6c2V0QXBw SGlnaGxpZ2h0c1Zpc2liaWxpdHkgbWlnaHQgc2VuZCBtZXNzYWdlIGZyb20gYSBiYWNrZ3JvdW5k IHRocmVhZCwgQVNTRVJUaW5nCisgICAgICAgIDxyZGFyOi8vODAwNTY0ODE+IGFuZCBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjI3NjA3CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQ29jb2EvV2ViUGFn ZVByb3h5Q29jb2EubW06CisgICAgICAgIChXZWJLaXQ6OldlYlBhZ2VQcm94eTo6c2V0QXBwSGln aGxpZ2h0c1Zpc2liaWxpdHkpOiBJZiBjYWxsZWQgb24gYSBiYWNrZ3JvdW5kIHRocmVhZCwgYm91 bmNlIHRvIG1haW4uCisKIDIwMjEtMDctMDEgIEplciBOb2JsZSAgPGplci5ub2JsZUBhcHBsZS5j b20+CiAKICAgICAgICAgW01hY10gQWRvcHQgYXN5bmMgR3JvdXBBY3Rpdml0eS5zZXNzaW9ucygp IGl0ZXJhYmxlIGluc3RlYWQgb2YgR3JvdXBTZXNzaW9uT2JzZXJ2ZXIKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJLaXQvVUlQcm9jZXNzL0NvY29hL1dlYlBhZ2VQcm94eUNvY29hLm1tIGIvU291cmNl L1dlYktpdC9VSVByb2Nlc3MvQ29jb2EvV2ViUGFnZVByb3h5Q29jb2EubW0KaW5kZXggZjliMjc2 NTM5OGY3ZmZmMzE0N2M1YmQwYTljMmRkMjRhZWVkYTEyYi4uOWU5NTMyMDBjOTQ4NmVlNDg3N2Yz NTg0ZWY3MTcyNTEyZjQwZmQ0YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9VSVByb2Nlc3Mv Q29jb2EvV2ViUGFnZVByb3h5Q29jb2EubW0KKysrIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3Mv Q29jb2EvV2ViUGFnZVByb3h5Q29jb2EubW0KQEAgLTU4Nyw2ICs1ODcsMTQgQEAgdm9pZCBXZWJQ YWdlUHJveHk6OnJlc3RvcmVBcHBIaWdobGlnaHRzQW5kU2Nyb2xsVG9JbmRleChjb25zdCBWZWN0 b3I8UmVmPFNoYXJlZE0KIAogdm9pZCBXZWJQYWdlUHJveHk6OnNldEFwcEhpZ2hsaWdodHNWaXNp YmlsaXR5KFdlYkNvcmU6OkhpZ2hsaWdodFZpc2liaWxpdHkgYXBwSGlnaGxpZ2h0c1Zpc2liaWxp dHkpCiB7CisgICAgaWYgKCFpc01haW5SdW5Mb29wKCkpIHsKKyAgICAgICAgY2FsbE9uTWFpblJ1 bkxvb3AoW3Byb3RlY3RlZFRoaXMgPSBtYWtlUmVmKCp0aGlzKSwgYXBwSGlnaGxpZ2h0c1Zpc2li aWxpdHldIHsKKyAgICAgICAgICAgIHByb3RlY3RlZFRoaXMtPnNldEFwcEhpZ2hsaWdodHNWaXNp YmlsaXR5KGFwcEhpZ2hsaWdodHNWaXNpYmlsaXR5KTsKKyAgICAgICAgfSk7CisgICAgICAgIAor ICAgICAgICByZXR1cm47CisgICAgfQorICAgIAogICAgIGlmICghaGFzUnVubmluZ1Byb2Nlc3Mo KSkKICAgICAgICAgcmV0dXJuOwogCg== 432803 2021-07-02 09:26:14 -0700 2021-07-02 10:57:20 -0700 Patch reviewed on Slack pat text/plain 2185 beidson ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9DaGFu Z2VMb2cKaW5kZXggMzZiOGU4NjRiMmM3Li44OTE0NWQ0MTBhOWEgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMg KzEsMTUgQEAKKzIwMjEtMDctMDIgIEJyYWR5IEVpZHNvbiAgPGJlaWRzb25AYXBwbGUuY29tPgor CisgICAgICAgIFdlYlBhZ2VQcm94eTo6c2V0QXBwSGlnaGxpZ2h0c1Zpc2liaWxpdHkgbWlnaHQg c2VuZCBtZXNzYWdlIGZyb20gYSBiYWNrZ3JvdW5kIHRocmVhZCwgQVNTRVJUaW5nCisgICAgICAg IDxyZGFyOi8vODAwNTY0ODE+IGFuZCBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MjI3NjA3CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgQ2hyaXMgRHVtZXouCisKKyAgICAg ICAgKiBVSVByb2Nlc3MvQ29jb2EvV2ViUGFnZVByb3h5Q29jb2EubW06CisgICAgICAgIChXZWJL aXQ6OldlYlBhZ2VQcm94eTo6c2V0QXBwSGlnaGxpZ2h0c1Zpc2liaWxpdHkpOgorICAgICAgICAo V2ViS2l0OjpXZWJQYWdlUHJveHk6OnNldFVwSGlnaGxpZ2h0c09ic2VydmVyKTogVGhlIGNhbGxi YWNrIHdlIGdldCBoZXJlIGlzIG9mdGVuIG9uIGEgYmFja2dyb3VuZCB0aHJlYWQuCisgICAgICAg ICAgU28gYm91bmNlIGl0IHRvIG1haW4uCisKIDIwMjEtMDctMDEgIEplciBOb2JsZSAgPGplci5u b2JsZUBhcHBsZS5jb20+CiAKICAgICAgICAgW01hY10gQWRvcHQgYXN5bmMgR3JvdXBBY3Rpdml0 eS5zZXNzaW9ucygpIGl0ZXJhYmxlIGluc3RlYWQgb2YgR3JvdXBTZXNzaW9uT2JzZXJ2ZXIKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0NvY29hL1dlYlBhZ2VQcm94eUNvY29h Lm1tIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQ29jb2EvV2ViUGFnZVByb3h5Q29jb2EubW0K aW5kZXggZjliMjc2NTM5OGY3Li5jZmE1Njg1M2ZlOTkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJL aXQvVUlQcm9jZXNzL0NvY29hL1dlYlBhZ2VQcm94eUNvY29hLm1tCisrKyBiL1NvdXJjZS9XZWJL aXQvVUlQcm9jZXNzL0NvY29hL1dlYlBhZ2VQcm94eUNvY29hLm1tCkBAIC01ODcsNiArNTg3LDgg QEAgdm9pZCBXZWJQYWdlUHJveHk6OnJlc3RvcmVBcHBIaWdobGlnaHRzQW5kU2Nyb2xsVG9JbmRl eChjb25zdCBWZWN0b3I8UmVmPFNoYXJlZE0KIAogdm9pZCBXZWJQYWdlUHJveHk6OnNldEFwcEhp Z2hsaWdodHNWaXNpYmlsaXR5KFdlYkNvcmU6OkhpZ2hsaWdodFZpc2liaWxpdHkgYXBwSGlnaGxp Z2h0c1Zpc2liaWxpdHkpCiB7CisgICAgUkVMRUFTRV9BU1NFUlQoaXNNYWluUnVuTG9vcCgpKTsK KyAgICAKICAgICBpZiAoIWhhc1J1bm5pbmdQcm9jZXNzKCkpCiAgICAgICAgIHJldHVybjsKIApA QCAtNjA0LDkgKzYwNiwxNiBAQCB2b2lkIFdlYlBhZ2VQcm94eTo6c2V0VXBIaWdobGlnaHRzT2Jz ZXJ2ZXIoKQogewogICAgIGlmIChtX2FwcEhpZ2hsaWdodHNPYnNlcnZlcikKICAgICAgICAgcmV0 dXJuOworCisgICAgYXV0byB3ZWFrVGhpcyA9IG1ha2VXZWFrUHRyKCp0aGlzKTsKICAgICBhdXRv IHVwZGF0ZUFwcEhpZ2hsaWdodHNWaXNpYmlsaXR5ID0gXihCT09MIGlzVmlzaWJsZSkgewotICAg ICAgICBzZXRBcHBIaWdobGlnaHRzVmlzaWJpbGl0eShpc1Zpc2libGUgPyBXZWJDb3JlOjpIaWdo bGlnaHRWaXNpYmlsaXR5OjpWaXNpYmxlIDogV2ViQ29yZTo6SGlnaGxpZ2h0VmlzaWJpbGl0eTo6 SGlkZGVuKTsKKyAgICAgICAgZW5zdXJlT25NYWluUnVuTG9vcChbd2Vha1RoaXMsIGlzVmlzaWJs ZV0geworICAgICAgICAgICAgaWYgKCF3ZWFrVGhpcykKKyAgICAgICAgICAgICAgICByZXR1cm47 CisgICAgICAgICAgICB3ZWFrVGhpcy0+c2V0QXBwSGlnaGxpZ2h0c1Zpc2liaWxpdHkoaXNWaXNp YmxlID8gV2ViQ29yZTo6SGlnaGxpZ2h0VmlzaWJpbGl0eTo6VmlzaWJsZSA6IFdlYkNvcmU6Okhp Z2hsaWdodFZpc2liaWxpdHk6OkhpZGRlbik7CisgICAgICAgIH0pOwogICAgIH07CisgICAgCiAg ICAgbV9hcHBIaWdobGlnaHRzT2JzZXJ2ZXIgPSBhZG9wdE5TKFthbGxvY1NZTm90ZXNBY3RpdmF0 aW9uT2JzZXJ2ZXJJbnN0YW5jZSgpIGluaXRXaXRoSGFuZGxlcjp1cGRhdGVBcHBIaWdobGlnaHRz VmlzaWJpbGl0eV0pOwogfQogCg==