Add UI process watchdog on iOS to ensure WebProcess connections close

• Component: WebKit2
• Status: RESOLVED FIXED

[Gavin Barraclough]

When the WebProcessProxy wants to disconnect from a WebContent process it just drops the connection, and hopes the connection closes. There is a watchdog thread in the ChildProcess to try to ensure this happens.

On iOS the process may not be runnable at the time, preventing termination. Instead add a watchdog in the UI process to make the process runnable, and to terminate if it doesn't quit in a timely fashion.

[Gavin Barraclough]

<rdar://problem/16997983>

[Gavin Barraclough]

Created attachment 231934 [details]
Fix

[Darin Adler]

Comment on attachment 231934 [details]
Fix

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

I’m saying review+, but I see a few problems.

> Source/WebKit2/Platform/IPC/Connection.h:181
> +    void terminateSoon(double seconds);

I think we could do better on this argument name. It identifies the units of the argument, but not the meaning. I’m not even sure from this name if it’s an interval or an absolute time. Maybe just intervalInSeconds, but we could probably do even better than that.

Seems strange to have one function call this "kill" and another "terminate". Are these two different operations, or are they both the same thing?

> Source/WebKit2/Platform/IPC/mac/ConnectionMac.mm:86
> +    ConnectionTerminationWatchdog(XPCPtr<xpc_connection_t>& xpcConnection, double seconds)
> +        : m_xpcConnection(xpcConnection)
> +        , m_watchdogTimer(RunLoop::main(), this, &ConnectionTerminationWatchdog::watchdogTimerFired)
> +#if PLATFORM(IOS)
> +        , m_assertion(std::make_unique<WebKit::ProcessAssertion>(xpc_connection_get_pid(m_xpcConnection.get()), WebKit::AssertionState::Background))
> +#endif
> +    {
> +        m_watchdogTimer.startOneShot(seconds);
> +    }

Since this object deletes itself, it’s not good to have a public constructor. It’s illegal to allocate this any way besides new. So I suggest having the constructor be private and the only public function be a static member that does the “new” part. Not a big deal since this class is local, but also not a big deal to fix it and do it right.

> Source/WebKit2/Platform/IPC/mac/ConnectionMac.mm:92
> +    void watchdogTimerFired()
> +    {
> +        xpc_connection_kill(m_xpcConnection.get(), SIGKILL);
> +        delete this;
> +    }

No reason for this to be public. Please make it private.

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:213
> +#if PLATFORM(IOS) && USE(XPC_SERVICES)

What’s the rationale for this #if? Why only IOS? Why only when USE(XPC_SERVICES)? Needs a comment.

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:214
> +        connection()->terminateSoon(30);

What’s the rationale for 30 seconds?

[Gavin Barraclough]

(In reply to comment #3)
> (From update of attachment 231934 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231934&action=review
> 
> I’m saying review+, but I see a few problems.
> 
> > Source/WebKit2/Platform/IPC/Connection.h:181
> > +    void terminateSoon(double seconds);
> 
> I think we could do better on this argument name. It identifies the units of the argument, but not the meaning. I’m not even sure from this name if it’s an interval or an absolute time. Maybe just intervalInSeconds, but we could probably do even better than that.

Done.

> Seems strange to have one function call this "kill" and another "terminate". Are these two different operations, or are they both the same thing?

The two operations are subtly different, I'm going to stick with terminate.

The code currently uses the name 'terminate' in ChildProcess, ChildProcessProxy, and ProcessLauncher to cover the concept of child process exit in general. This termination may take place through  number of means, including a posix kill, xpc kill, or runloop completion. The 'terminateSoon' method fits within this naming scheme since it also may result in both kill and runloop exit.

The existing 'kill' method on connection is just about sending an xpc kill.

> > Source/WebKit2/Platform/IPC/mac/ConnectionMac.mm:86
> > +    ConnectionTerminationWatchdog(XPCPtr<xpc_connection_t>& xpcConnection, double seconds)
> > +        : m_xpcConnection(xpcConnection)
> > +        , m_watchdogTimer(RunLoop::main(), this, &ConnectionTerminationWatchdog::watchdogTimerFired)
> > +#if PLATFORM(IOS)
> > +        , m_assertion(std::make_unique<WebKit::ProcessAssertion>(xpc_connection_get_pid(m_xpcConnection.get()), WebKit::AssertionState::Background))
> > +#endif
> > +    {
> > +        m_watchdogTimer.startOneShot(seconds);
> > +    }
> 
> Since this object deletes itself, it’s not good to have a public constructor. It’s illegal to allocate this any way besides new. So I suggest having the constructor be private and the only public function be a static member that does the “new” part. Not a big deal since this class is local, but also not a big deal to fix it and do it right.

Done.

> > Source/WebKit2/Platform/IPC/mac/ConnectionMac.mm:92
> > +    void watchdogTimerFired()
> > +    {
> > +        xpc_connection_kill(m_xpcConnection.get(), SIGKILL);
> > +        delete this;
> > +    }
> 
> No reason for this to be public. Please make it private.

Done.

> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:213
> > +#if PLATFORM(IOS) && USE(XPC_SERVICES)
> 
> What’s the rationale for this #if? Why only IOS?

Comment added - on other platforms there is a watchdog in the WebContent; this is iOS only since the WebContent process may be suspended and unable to police itself.

> Why only when USE(XPC_SERVICES)? Needs a comment.

Removed; we don't need this.

> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:214
> > +        connection()->terminateSoon(30);
> 
> What’s the rationale for 30 seconds?

Comment added; this is a judgement call – what is a reasonable minimum amount of time to give a process for the clean completion of any outstanding tasks, vs what is the maximum amount of time we're willing to allow the process to run in the background.

[Gavin Barraclough]

Fixed in r169362

[Jessie Berlin]

r169362 broke the ML builders. I rolled it out in r169384.

[Gavin Barraclough]

Relanded 169393.