Bug 216594

Summary: WebKit::XPCServiceEventHandler block should call exit() on the main thread
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebKit2Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bfulgham, cdumez, darin, ggaren, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 217773, 243955    
Attachments:
Description Flags
Patch v1 none

Description David Kilzer (:ddkilzer) 2020-09-15 18:40:23 PDT
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>
Comment 1 David Kilzer (:ddkilzer) 2020-09-15 19:24:23 PDT
Created attachment 408887 [details]
Patch v1
Comment 2 Alexey Proskuryakov 2020-09-15 20:23:59 PDT
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?
Comment 3 Chris Dumez 2020-09-15 21:38:38 PDT
(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.
Comment 4 Sam Weinig 2020-09-16 10:30:33 PDT
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?
Comment 5 Chris Dumez 2020-09-16 10:33:22 PDT
(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.
Comment 6 Chris Dumez 2020-09-16 10:35:16 PDT
(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.
Comment 7 Sam Weinig 2020-09-16 11:15:45 PDT
(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.
Comment 8 Chris Dumez 2020-09-17 08:34:54 PDT
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?
Comment 9 Darin Adler 2020-09-17 12:35:06 PDT
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).
Comment 10 Chris Dumez 2020-09-17 13:03:09 PDT
(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.
Comment 11 Sam Weinig 2020-09-17 13:07:46 PDT
(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.
Comment 12 EWS 2020-09-17 13:21:39 PDT
Committed r267206: <https://trac.webkit.org/changeset/267206>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408887 [details].