Bug 216594 – WebKit::XPCServiceEventHandler block should call exit() on the main thread

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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>

[David Kilzer (:ddkilzer)]

Created attachment 408887 [details]
Patch v1

[Alexey Proskuryakov]

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?

[Chris Dumez]

(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.

[Sam Weinig]

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?

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Sam Weinig]

(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.

[Chris Dumez]

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?

[Darin Adler]

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).

[Chris Dumez]

(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.

[Sam Weinig]

(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.

[EWS]

Committed r267206: <https://trac.webkit.org/changeset/267206>

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