[webkit-reviews] review granted: [Bug 133200] Add UI process watchdog on iOS to ensure WebProcess connections close : [Attachment 231934] Fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 133200: Add UI process watchdog on iOS to ensure WebProcess connections
close
https://bugs.webkit.org/show_bug.cgi?id=133200

Attachment 231934: Fix
https://bugs.webkit.org/attachment.cgi?id=231934&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?