Bug 237398 – [macOS] WebContent processes crash with XPC_EXIT_REASON_SIGTERM_TIMEOUT when logging out

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

WebContent processes crash with XPC_EXIT_REASON_SIGTERM_TIMEOUT when logging out of macOS.

[Chris Dumez]

<rdar://88940229>

[Chris Dumez]

Created attachment 453674 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 453674 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453674&action=review

> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm:178
> +            exit(0);

Is it safe to call exit from a signal handler? atexit functions could do arbitrary work, so I’d be looking at _exit().

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 453674 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453674&action=review
> 
> > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm:178
> > +            exit(0);
> 
> Is it safe to call exit from a signal handler? atexit functions could do
> arbitrary work, so I’d be looking at _exit().

Well, we do want those atexit handlers to run though I think. It minimizes the odds of losing cookies and storage data.

What is the concern exactly? That the atexit handlers take too long and we reach the timeout and still get killed? Even if that’s the case, I would think it is preferable to at least try and run those handlers. Would be sad to lose some cookies or local storage data when logging out.

[Alexey Proskuryakov]

My concern is that atexit handlers will be using functions that are unsafe in signal handler context (which is extremely restrictive), so we'll be getting memory corruption, hangs and such.

If we need to run these even after receiving SIGTERM, we need to ignore the signal, and to initiate a clean exit from run loop somehow (I don't know how exactly to do that from signal handler context).

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #5)
> My concern is that atexit handlers will be using functions that are unsafe
> in signal handler context (which is extremely restrictive), so we'll be
> getting memory corruption, hangs and such.
> 
> If we need to run these even after receiving SIGTERM, we need to ignore the
> signal, and to initiate a clean exit from run loop somehow (I don't know how
> exactly to do that from signal handler context).

I may be able to do the following in my signal handler:
1. Clear the OS transaction
2. Reset to the default signal handler
3. raise() the signal again 

I need to validate this to make sure it actually works in practice though.

[Chris Dumez]

Created attachment 453770 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Alexey Proskuryakov from comment #5)
> > My concern is that atexit handlers will be using functions that are unsafe
> > in signal handler context (which is extremely restrictive), so we'll be
> > getting memory corruption, hangs and such.
> > 
> > If we need to run these even after receiving SIGTERM, we need to ignore the
> > signal, and to initiate a clean exit from run loop somehow (I don't know how
> > exactly to do that from signal handler context).
> 
> I may be able to do the following in my signal handler:
> 1. Clear the OS transaction
> 2. Reset to the default signal handler
> 3. raise() the signal again 
> 
> I need to validate this to make sure it actually works in practice though.

Ok, I implemented this alternative proposal that should be less controversial I hope.
I tested it manually and it seems to work just as well.

[Darin Adler]

Comment on attachment 453770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453770&action=review

> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm:180
> +        signal(SIGTERM, [](int) {
> +            globalTransaction.get() = nullptr;
> +            signal(SIGTERM, SIG_DFL);
> +            raise(SIGTERM);
> +        });

This looks great. Really glad Alexey pointed out it was not OK to call exit. I tried to do some more research on "cleanup and re-raise signal" and see if it’s more elegantly done with sigaction instead of signal, but the examples I found are all like what you wrote here.

[Alexey Proskuryakov]

Looks good to me too. I'm far from being an expert on writing code that works in signal handlers, but I couldn't find anything against this approach with a few web searches.

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #10)
> Looks good to me too. I'm far from being an expert on writing code that
> works in signal handlers, but I couldn't find anything against this approach
> with a few web searches.

Ok, thanks for pointing out the issue in the original proposal. TIL :)

[EWS]

Committed r290795 (248035@main): <https://commits.webkit.org/248035@main>

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

[Chris Dumez]

Reverted r290795 for reason:

Caused crashes <rdar://problem/89970722>

Committed r291110 (248272@trunk): <https://commits.webkit.org/248272@trunk>

[Chris Dumez]

Created attachment 454364 [details]
Patch

[Chris Dumez]

Comment on attachment 454364 [details]
Patch

Turns out I cannot even release the OS transaction in the signal handler. As a result, I am going back to Alexey's original proposal to call _exit(0).

Eventually we should just stop leaking this transaction (and adopt RunningBoard) but this is quite a bit of work and not risk free.

[Alexey Proskuryakov]

Comment on attachment 454364 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454364&action=review

> Source/WebKit/ChangeLog:14
> +        To address the issue, we now set our own SIGTERM handler that releases the OS transaction and calls _exit(0)

Please update the ChangeLog for updated patch.

[Chris Dumez]

Created attachment 454403 [details]
Patch for landing

[EWS]

Committed r291137 (248297@main): <https://commits.webkit.org/248297@main>

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