216594 2020-09-15 18:40:23 -0700 WebKit::XPCServiceEventHandler block should call exit() on the main thread 2022-08-15 14:49:28 -0700 1 1 1 Unclassified WebKit WebKit2 Other Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 217773 243955 1 ddkilzer ddkilzer ap bfulgham cdumez darin ggaren sam webkit-bug-importer oldest_to_newest 1688959 0 ddkilzer 2020-09-15 18:40:23 -0700 WebKit::XPCServiceEventHandler block should call exit() on the main thread. It turns out that exit() is not thread-safe when there are C++ exit-time destructors from other frameworks or libraries involved. <rdar://problem/68053217> 1688967 1 408887 ddkilzer 2020-09-15 19:24:23 -0700 Created attachment 408887 Patch v1 1688976 2 408887 ap 2020-09-15 20:23:59 -0700 Comment on attachment 408887 Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=408887&action=review > Source/WebKit/ChangeLog:3 > + WebKit::XPCServiceEventHandler block should call exit() on the main thread Could we call _exit and not execute those destructors? 1688996 3 cdumez 2020-09-15 21:38:38 -0700 (In reply to Alexey Proskuryakov from comment #2) > Comment on attachment 408887 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=408887&action=review > > > Source/WebKit/ChangeLog:3 > > + WebKit::XPCServiceEventHandler block should call exit() on the main thread > > Could we call _exit and not execute those destructors? We have to be careful. My understanding is that CFNetwork uses at-exit code to flush cookies. If we call _exit() that code won't run and we might loose cookies. 1689150 4 408887 sam 2020-09-16 10:30:33 -0700 Comment on attachment 408887 Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=408887&action=review >>> Source/WebKit/ChangeLog:3 >>> + WebKit::XPCServiceEventHandler block should call exit() on the main thread >> >> Could we call _exit and not execute those destructors? > > We have to be careful. My understanding is that CFNetwork uses at-exit code to flush cookies. If we call _exit() that code won't run and we might loose cookies. I hope that's not true :(. How could that possibly work with jetsam? 1689152 5 cdumez 2020-09-16 10:33:22 -0700 (In reply to Sam Weinig from comment #4) > Comment on attachment 408887 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=408887&action=review > > >>> Source/WebKit/ChangeLog:3 > >>> + WebKit::XPCServiceEventHandler block should call exit() on the main thread > >> > >> Could we call _exit and not execute those destructors? > > > > We have to be careful. My understanding is that CFNetwork uses at-exit code to flush cookies. If we call _exit() that code won't run and we might loose cookies. > > I hope that's not true :(. How could that possibly work with jetsam? I am 99% sure this is true. Yes, it wouldn't work with jetsams but at least this is a best effort kind of thing. 1689153 6 cdumez 2020-09-16 10:35:16 -0700 (In reply to Chris Dumez from comment #5) > (In reply to Sam Weinig from comment #4) > > Comment on attachment 408887 [details] > > Patch v1 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=408887&action=review > > > > >>> Source/WebKit/ChangeLog:3 > > >>> + WebKit::XPCServiceEventHandler block should call exit() on the main thread > > >> > > >> Could we call _exit and not execute those destructors? > > > > > > We have to be careful. My understanding is that CFNetwork uses at-exit code to flush cookies. If we call _exit() that code won't run and we might loose cookies. > > > > I hope that's not true :(. How could that possibly work with jetsam? > > I am 99% sure this is true. Yes, it wouldn't work with jetsams but at least > this is a best effort kind of thing. The reason I mentioned this is that WebKit uses to rely on _exit a lot and CFNetwork people asked us to use exit instead so that their at-exit code would run. Going back to _exit would re-introduce the issue so I'd rather we keep calling exit. If we really want to call _exit for some reason, then I'd suggest talking to the CFNetwork team first. 1689165 7 sam 2020-09-16 11:15:45 -0700 (In reply to Chris Dumez from comment #6) > (In reply to Chris Dumez from comment #5) > > (In reply to Sam Weinig from comment #4) > > > Comment on attachment 408887 [details] > > > Patch v1 > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=408887&action=review > > > > > > >>> Source/WebKit/ChangeLog:3 > > > >>> + WebKit::XPCServiceEventHandler block should call exit() on the main thread > > > >> > > > >> Could we call _exit and not execute those destructors? > > > > > > > > We have to be careful. My understanding is that CFNetwork uses at-exit code to flush cookies. If we call _exit() that code won't run and we might loose cookies. > > > > > > I hope that's not true :(. How could that possibly work with jetsam? > > > > I am 99% sure this is true. Yes, it wouldn't work with jetsams but at least > > this is a best effort kind of thing. > > The reason I mentioned this is that WebKit uses to rely on _exit a lot and > CFNetwork people asked us to use exit instead so that their at-exit code > would run. Going back to _exit would re-introduce the issue so I'd rather we > keep calling exit. If we really want to call _exit for some reason, then I'd > suggest talking to the CFNetwork team first. I definitely think you should talk with the CFNetwork folks about this. No code should be relying on at-exit code anymore. It is incompatible with how applications and daemons are meant to work these days. 1689455 8 408887 cdumez 2020-09-17 08:34:54 -0700 Comment on attachment 408887 Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=408887&action=review >>>>>>> Source/WebKit/ChangeLog:3 >>>>>>> + WebKit::XPCServiceEventHandler block should call exit() on the main thread >>>>>> >>>>>> Could we call _exit and not execute those destructors? >>>>> >>>>> We have to be careful. My understanding is that CFNetwork uses at-exit code to flush cookies. If we call _exit() that code won't run and we might loose cookies. >>>> >>>> I hope that's not true :(. How could that possibly work with jetsam? >>> >>> I am 99% sure this is true. Yes, it wouldn't work with jetsams but at least this is a best effort kind of thing. >> >> The reason I mentioned this is that WebKit uses to rely on _exit a lot and CFNetwork people asked us to use exit instead so that their at-exit code would run. Going back to _exit would re-introduce the issue so I'd rather we keep calling exit. If we really want to call _exit for some reason, then I'd suggest talking to the CFNetwork team first. > > I definitely think you should talk with the CFNetwork folks about this. No code should be relying on at-exit code anymore. It is incompatible with how applications and daemons are meant to work these days. Given that CFNetwork currently has at-exit handlers, the safest approach to me seems to be to dispatch to the main thread to call exit() instead of calling _exit() from the background thread. I would r+ if not for Sam pushing back. Sam, could you explain why you don't like this approach? 1689552 9 408887 darin 2020-09-17 12:35:06 -0700 Comment on attachment 408887 Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=408887&action=review >>>>>>>> Source/WebKit/ChangeLog:3 >>>>>>>> + WebKit::XPCServiceEventHandler block should call exit() on the main thread >>>>>>> >>>>>>> Could we call _exit and not execute those destructors? >>>>>> >>>>>> We have to be careful. My understanding is that CFNetwork uses at-exit code to flush cookies. If we call _exit() that code won't run and we might loose cookies. >>>>> >>>>> I hope that's not true :(. How could that possibly work with jetsam? >>>> >>>> I am 99% sure this is true. Yes, it wouldn't work with jetsams but at least this is a best effort kind of thing. >>> >>> The reason I mentioned this is that WebKit uses to rely on _exit a lot and CFNetwork people asked us to use exit instead so that their at-exit code would run. Going back to _exit would re-introduce the issue so I'd rather we keep calling exit. If we really want to call _exit for some reason, then I'd suggest talking to the CFNetwork team first. >> >> I definitely think you should talk with the CFNetwork folks about this. No code should be relying on at-exit code anymore. It is incompatible with how applications and daemons are meant to work these days. > > Given that CFNetwork currently has at-exit handlers, the safest approach to me seems to be to dispatch to the main thread to call exit() instead of calling _exit() from the background thread. I would r+ if not for Sam pushing back. Sam, could you explain why you don't like this approach? I believe Sam is pushing back on the big picture, not laser-focused on the specifics of this patch’s work around. I think he’s saying that the CFNetwork behavior is not the right strategy, and someone should discuss that with them and once that is fixed, WebKit code might change again. Sam, maybe you should write up your CFNetwork concern in Radar to help foster the conversation you’d like Chris to have with them (and we can make that an internal Apple discussion rather than here in open source comments). 1689566 10 cdumez 2020-09-17 13:03:09 -0700 (In reply to Darin Adler from comment #9) > Comment on attachment 408887 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=408887&action=review > > >>>>>>>> Source/WebKit/ChangeLog:3 > >>>>>>>> + WebKit::XPCServiceEventHandler block should call exit() on the main thread > >>>>>>> > >>>>>>> Could we call _exit and not execute those destructors? > >>>>>> > >>>>>> We have to be careful. My understanding is that CFNetwork uses at-exit code to flush cookies. If we call _exit() that code won't run and we might loose cookies. > >>>>> > >>>>> I hope that's not true :(. How could that possibly work with jetsam? > >>>> > >>>> I am 99% sure this is true. Yes, it wouldn't work with jetsams but at least this is a best effort kind of thing. > >>> > >>> The reason I mentioned this is that WebKit uses to rely on _exit a lot and CFNetwork people asked us to use exit instead so that their at-exit code would run. Going back to _exit would re-introduce the issue so I'd rather we keep calling exit. If we really want to call _exit for some reason, then I'd suggest talking to the CFNetwork team first. > >> > >> I definitely think you should talk with the CFNetwork folks about this. No code should be relying on at-exit code anymore. It is incompatible with how applications and daemons are meant to work these days. > > > > Given that CFNetwork currently has at-exit handlers, the safest approach to me seems to be to dispatch to the main thread to call exit() instead of calling _exit() from the background thread. I would r+ if not for Sam pushing back. Sam, could you explain why you don't like this approach? > > I believe Sam is pushing back on the big picture, not laser-focused on the > specifics of this patch’s work around. > > I think he’s saying that the CFNetwork behavior is not the right strategy, > and someone should discuss that with them and once that is fixed, WebKit > code might change again. > > Sam, maybe you should write up your CFNetwork concern in Radar to help > foster the conversation you’d like Chris to have with them (and we can make > that an internal Apple discussion rather than here in open source comments). Yes, there is a mail thread going on with CFNetwork and Sam. 1689567 11 sam 2020-09-17 13:07:46 -0700 (In reply to Sam Weinig from comment #7) > (In reply to Chris Dumez from comment #6) > > (In reply to Chris Dumez from comment #5) > > > (In reply to Sam Weinig from comment #4) > > > > Comment on attachment 408887 [details] > > > > Patch v1 > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=408887&action=review > > > > > > > > >>> Source/WebKit/ChangeLog:3 > > > > >>> + WebKit::XPCServiceEventHandler block should call exit() on the main thread > > > > >> > > > > >> Could we call _exit and not execute those destructors? > > > > > > > > > > We have to be careful. My understanding is that CFNetwork uses at-exit code to flush cookies. If we call _exit() that code won't run and we might loose cookies. > > > > > > > > I hope that's not true :(. How could that possibly work with jetsam? > > > > > > I am 99% sure this is true. Yes, it wouldn't work with jetsams but at least > > > this is a best effort kind of thing. > > > > The reason I mentioned this is that WebKit uses to rely on _exit a lot and > > CFNetwork people asked us to use exit instead so that their at-exit code > > would run. Going back to _exit would re-introduce the issue so I'd rather we > > keep calling exit. If we really want to call _exit for some reason, then I'd > > suggest talking to the CFNetwork team first. > > I definitely think you should talk with the CFNetwork folks about this. No > code should be relying on at-exit code anymore. It is incompatible with how > applications and daemons are meant to work these days. I apologize I wasn't more clear with this comment :(. I really meant we should follow up with the CFNetwork team *in addition* to making this change. I realize now that wasn't at all clear. Sorry about that. This change should definitely go in. 1689573 12 ews-feeder 2020-09-17 13:21:39 -0700 Committed r267206: <https://trac.webkit.org/changeset/267206> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408887. 408887 2020-09-15 19:24:23 -0700 2020-09-17 13:21:39 -0700 Patch v1 bug-216594-20200915192445.patch text/plain 3875 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjY2OTc5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDE0YzhlMzNhMjFmZWRkOWYz OWIwOTJiNDg4ODI5OGY0ZjU0NDk2MDYuLjllYjgzOWY4ZjA1Mzc4YzVmYjQ2Yzc5ZmNmN2RkNmY0 MGY4ZTIyYTIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjUgQEAKKzIwMjAtMDktMTUgIERhdmlkIEtp bHplciAgPGRka2lsemVyQGFwcGxlLmNvbT4KKworICAgICAgICBXZWJLaXQ6OlhQQ1NlcnZpY2VF dmVudEhhbmRsZXIgYmxvY2sgc2hvdWxkIGNhbGwgZXhpdCgpIG9uIHRoZSBtYWluIHRocmVhZAor ICAgICAgICA8aHR0cHM6Ly93ZWJraXQub3JnL2IvMjE2NTk0PgorICAgICAgICA8cmRhcjovL3By b2JsZW0vNjgwNTMyMTc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgKiBTaGFyZWQvRW50cnlQb2ludFV0aWxpdGllcy9Db2NvYS9YUENTZXJ2aWNlL1hQ Q1NlcnZpY2VNYWluLm1tOgorICAgICAgICAoV2ViS2l0OjpYUENTZXJ2aWNlRXZlbnRIYW5kbGVy KToKKyAgICAgICAgLSBDYWxsIGV4aXQoKSBvbiB0aGUgbWFpbiB0aHJlYWQuIEkgY2hvc2UgZGlz cGF0Y2hfc3luYygpIGhlcmUKKyAgICAgICAgICBzbyB0aGF0IGlmIG90aGVyIGNyYXNoZXMgb2Nj dXIgb24gdGhlIG1haW4gdGhyZWFkLCB3ZSB3aWxsCisgICAgICAgICAga25vdyBpZiB0aGlzIGJh Y2tncm91bmQgdGhyZWFkIGlzIHdhaXRpbmcgdG8gY2FsbCBleGl0KCkgYXQKKyAgICAgICAgICB0 aGUgc2FtZSB0aW1lLiBBbHNvLCBpZiBhIGRpZmZlcmVudCBiYWNrZ3JvdW5kIHRocmVhZCBjYWxs cworICAgICAgICAgIGV4aXQoKSBpbiB0aGUgZnV0dXJlIHdpdGggdGhpcyBibG9jayBydW5uaW5n IG9uIHRoZSBtYWluCisgICAgICAgICAgdGhyZWFkLCB3ZSdsbCBrbm93IHdoZXJlIHRoZSBibG9j ayBjYW1lIGZyb20gc2luY2UgdGhlIFhQQworICAgICAgICAgIGhhbmRsZXIgdGhyZWFkIHdpbGwg YmUgd2FpdGluZyBmb3IgdGhlIGRpc3BhdGNoX3N5bmMoKSB0bworICAgICAgICAgIHJldHVybiAo aW5zdGVhZCBvZiBoYXZpbmcgYSBteXN0ZXJpb3VzIGJsb2NrIGNhbGxpbmcgZXhpdCgpIG9uCisg ICAgICAgICAgdGhlIG1haW4gdGhyZWFkIGFuZCBub3Qga25vd2luZyB3aGVyZSBpdCBjYW1lIGZy b20pLgorICAgICAgICAtIEFkZCBsb2dnaW5nIChpbmNsdWRpbmcgc2ltdWxhdGVkIGNyYXNoIGxv Z3MpIHdoZW4gZWl0aGVyIG9mCisgICAgICAgICAgdGhlc2UgZXhpdCgpIGNvZGUgcGF0aHMgaXMg dGFrZW4uCisKIDIwMjAtMDktMTEgIEFsZXggQ2hyaXN0ZW5zZW4gIDxhY2hyaXN0ZW5zZW5Ad2Vi a2l0Lm9yZz4KIAogICAgICAgICBNb3ZlIF9zZXRDb29raWVBY2NlcHRQb2xpY3kgdG8gV0tIVFRQ Q29va2llU3RvcmUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvU2hhcmVkL0VudHJ5UG9pbnRV dGlsaXRpZXMvQ29jb2EvWFBDU2VydmljZS9YUENTZXJ2aWNlTWFpbi5tbSBiL1NvdXJjZS9XZWJL aXQvU2hhcmVkL0VudHJ5UG9pbnRVdGlsaXRpZXMvQ29jb2EvWFBDU2VydmljZS9YUENTZXJ2aWNl TWFpbi5tbQppbmRleCA1ZmM1NTFkMTZmNjk5MDBiYTRlMjA3NGU0NzhiYTM5ZjNjOTE5N2M3Li45 M2I3ZjQ1MjE4NjBmODJiN2U3NDI2Zjc1NzE0MjFlZjIxYTQzMGE5IDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViS2l0L1NoYXJlZC9FbnRyeVBvaW50VXRpbGl0aWVzL0NvY29hL1hQQ1NlcnZpY2UvWFBD U2VydmljZU1haW4ubW0KKysrIGIvU291cmNlL1dlYktpdC9TaGFyZWQvRW50cnlQb2ludFV0aWxp dGllcy9Db2NvYS9YUENTZXJ2aWNlL1hQQ1NlcnZpY2VNYWluLm1tCkBAIC0yNiw2ICsyNiw3IEBA CiAjaW1wb3J0ICJjb25maWcuaCIKIAogI2ltcG9ydCAiSGFuZGxlWFBDRW5kcG9pbnRNZXNzYWdl cy5oIgorI2ltcG9ydCAiTG9nZ2luZy5oIgogI2ltcG9ydCAiV0tDcmFzaFJlcG9ydGVyLmgiCiAj aW1wb3J0ICJYUENTZXJ2aWNlRW50cnlQb2ludC5oIgogI2ltcG9ydCA8Q29yZUZvdW5kYXRpb24v Q29yZUZvdW5kYXRpb24uaD4KQEAgLTQ3LDggKzQ4LDExIEBAIHN0YXRpYyB2b2lkIFhQQ1NlcnZp Y2VFdmVudEhhbmRsZXIoeHBjX2Nvbm5lY3Rpb25fdCBwZWVyKQogICAgICAgICB4cGNfdHlwZV90 IHR5cGUgPSB4cGNfZ2V0X3R5cGUoZXZlbnQpOwogICAgICAgICBpZiAodHlwZSA9PSBYUENfVFlQ RV9FUlJPUikgewogICAgICAgICAgICAgaWYgKGV2ZW50ID09IFhQQ19FUlJPUl9DT05ORUNUSU9O X0lOVkFMSUQgfHwgZXZlbnQgPT0gWFBDX0VSUk9SX1RFUk1JTkFUSU9OX0lNTUlORU5UKSB7Cisg ICAgICAgICAgICAgICAgUkVMRUFTRV9MT0dfRkFVTFQoSVBDLCAiRXhpdGluZzogUmVjZWl2ZWQg WFBDIGV2ZW50IHR5cGU6ICVzIiwgZXZlbnQgPT0gWFBDX0VSUk9SX0NPTk5FQ1RJT05fSU5WQUxJ RCA/ICJYUENfRVJST1JfQ09OTkVDVElPTl9JTlZBTElEIiA6ICJYUENfRVJST1JfVEVSTUlOQVRJ T05fSU1NSU5FTlQiKTsKICAgICAgICAgICAgICAgICAvLyBGSVhNRTogSGFuZGxlIHRoaXMgY2Fz ZSBtb3JlIGdyYWNlZnVsbHkuCi0gICAgICAgICAgICAgICAgZXhpdChFWElUX0ZBSUxVUkUpOwor ICAgICAgICAgICAgICAgIGRpc3BhdGNoX3N5bmMoZGlzcGF0Y2hfZ2V0X21haW5fcXVldWUoKSwg XnsKKyAgICAgICAgICAgICAgICAgICAgZXhpdChFWElUX0ZBSUxVUkUpOworICAgICAgICAgICAg ICAgIH0pOwogICAgICAgICAgICAgfQogICAgICAgICB9IGVsc2UgewogICAgICAgICAgICAgYXNz ZXJ0KHR5cGUgPT0gWFBDX1RZUEVfRElDVElPTkFSWSk7CkBAIC03Miw4ICs3NiwxMCBAQCBzdGF0 aWMgdm9pZCBYUENTZXJ2aWNlRXZlbnRIYW5kbGVyKHhwY19jb25uZWN0aW9uX3QgcGVlcikKICAg ICAgICAgICAgICAgICB0eXBlZGVmIHZvaWQgKCpJbml0aWFsaXplckZ1bmN0aW9uKSh4cGNfY29u bmVjdGlvbl90LCB4cGNfb2JqZWN0X3QsIHhwY19vYmplY3RfdCk7CiAgICAgICAgICAgICAgICAg SW5pdGlhbGl6ZXJGdW5jdGlvbiBpbml0aWFsaXplckZ1bmN0aW9uUHRyID0gcmVpbnRlcnByZXRf Y2FzdDxJbml0aWFsaXplckZ1bmN0aW9uPihDRkJ1bmRsZUdldEZ1bmN0aW9uUG9pbnRlckZvck5h bWUod2ViS2l0QnVuZGxlLCBlbnRyeVBvaW50RnVuY3Rpb25OYW1lKSk7CiAgICAgICAgICAgICAg ICAgaWYgKCFpbml0aWFsaXplckZ1bmN0aW9uUHRyKSB7Ci0gICAgICAgICAgICAgICAgICAgIE5T TG9nKEAiVW5hYmxlIHRvIGZpbmQgZW50cnkgcG9pbnQgaW4gV2ViS2l0LmZyYW1ld29yayB3aXRo IG5hbWU6ICVAIiwgKF9fYnJpZGdlIE5TU3RyaW5nICopZW50cnlQb2ludEZ1bmN0aW9uTmFtZSk7 Ci0gICAgICAgICAgICAgICAgICAgIGV4aXQoRVhJVF9GQUlMVVJFKTsKKyAgICAgICAgICAgICAg ICAgICAgUkVMRUFTRV9MT0dfRkFVTFQoSVBDLCAiRXhpdGluZzogVW5hYmxlIHRvIGZpbmQgZW50 cnkgcG9pbnQgaW4gV2ViS2l0LmZyYW1ld29yayB3aXRoIG5hbWU6ICVzIiwgWyhfX2JyaWRnZSBO U1N0cmluZyAqKWVudHJ5UG9pbnRGdW5jdGlvbk5hbWUgVVRGOFN0cmluZ10pOworICAgICAgICAg ICAgICAgICAgICBkaXNwYXRjaF9zeW5jKGRpc3BhdGNoX2dldF9tYWluX3F1ZXVlKCksIF57Cisg ICAgICAgICAgICAgICAgICAgICAgICBleGl0KEVYSVRfRkFJTFVSRSk7CisgICAgICAgICAgICAg ICAgICAgIH0pOwogICAgICAgICAgICAgICAgIH0KIAogICAgICAgICAgICAgICAgIGF1dG8gcmVw bHkgPSBhZG9wdE9TT2JqZWN0KHhwY19kaWN0aW9uYXJ5X2NyZWF0ZV9yZXBseShldmVudCkpOwo=