133200 2014-05-22 20:16:36 -0700 Add UI process watchdog on iOS to ensure WebProcess connections close 2014-05-27 11:32:53 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 barraclough barraclough jberlin oldest_to_newest 1011130 0 barraclough 2014-05-22 20:16:36 -0700 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. 1011131 1 barraclough 2014-05-22 20:16:49 -0700 <rdar://problem/16997983> 1011133 2 231934 barraclough 2014-05-22 20:21:44 -0700 Created attachment 231934 Fix 1011508 3 231934 darin 2014-05-24 08:36:18 -0700 Comment on attachment 231934 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? 1011846 4 barraclough 2014-05-26 16:58:55 -0700 (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. 1011850 5 barraclough 2014-05-26 17:50:22 -0700 Fixed in r169362 1011987 6 jberlin 2014-05-27 09:32:35 -0700 r169362 broke the ML builders. I rolled it out in r169384. 1012020 7 barraclough 2014-05-27 11:32:53 -0700 Relanded 169393. 231934 2014-05-22 20:21:44 -0700 2014-05-24 08:36:18 -0700 Fix 133200.1.patch text/plain 4890 barraclough SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2OTIzOSkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDMxIEBACisyMDE0LTA1LTIyICBHYXZpbiBC YXJyYWNsb3VnaCAgPGJhcmFjbG91Z2hAYXBwbGUuY29tPgorCisgICAgICAgIEFkZCBVSSBwcm9j ZXNzIHdhdGNoZG9nIG9uIGlPUyB0byBlbnN1cmUgV2ViUHJvY2VzcyBjb25uZWN0aW9ucyBjbG9z ZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTMzMjAw CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8xNjk5Nzk4Mz4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXaGVuIHRoZSBXZWJQcm9jZXNzUHJveHkgd2Fu dHMgdG8gZGlzY29ubmVjdCBmcm9tIGEgV2ViQ29udGVudCBwcm9jZXNzIGl0IGp1c3QgZHJvcHMg dGhlIGNvbm5lY3Rpb24sCisgICAgICAgIGFuZCBob3BlcyB0aGUgY29ubmVjdGlvbiBjbG9zZXMu IFRoZXJlIGlzIGEgd2F0Y2hkb2cgdGhyZWFkIGluIHRoZSBDaGlsZFByb2Nlc3MgdG8gdHJ5IHRv IGVuc3VyZSB0aGlzCisgICAgICAgIGhhcHBlbnMuCisKKyAgICAgICAgT24gaU9TIHRoZSBwcm9j ZXNzIG1heSBub3QgYmUgcnVubmFibGUgYXQgdGhlIHRpbWUsIHByZXZlbnRpbmcgdGVybWluYXRp b24uIEluc3RlYWQgYWRkIGEgd2F0Y2hkb2cgaW4KKyAgICAgICAgdGhlIFVJIHByb2Nlc3MgdG8g bWFrZSB0aGUgcHJvY2VzcyBydW5uYWJsZSwgYW5kIHRvIHRlcm1pbmF0ZSBpZiBpdCBkb2Vzbid0 IHF1aXQgaW4gYSB0aW1lbHkgZmFzaGlvbi4KKworICAgICAgICAqIFBsYXRmb3JtL0lQQy9Db25u ZWN0aW9uLmg6CisgICAgICAgICAgICAtIGFkZGVkIHRlcm1pbmF0ZVNvb24uCisgICAgICAgICog UGxhdGZvcm0vSVBDL21hYy9Db25uZWN0aW9uTWFjLm1tOgorICAgICAgICAoSVBDOjpDb25uZWN0 aW9uVGVybWluYXRpb25XYXRjaGRvZzo6Q29ubmVjdGlvblRlcm1pbmF0aW9uV2F0Y2hkb2cpOgor ICAgICAgICAgICAgLSB0YWtlIGFuIGFzc2VydGlvbiB0byBtYWtlIHRoZSBwcm9jZXNzIHJ1bm5h YmxlLCBhbmQgc3RhcnQgYSB3YXRjaGRvZyB0aW1lci4KKyAgICAgICAgKElQQzo6Q29ubmVjdGlv blRlcm1pbmF0aW9uV2F0Y2hkb2c6OndhdGNoZG9nVGltZXJGaXJlZCk6CisgICAgICAgICAgICAt IGlmIHRoZSBwcm9jZXNzIGhhc24ndCBxdWl0IGJ5IHRoZSB0aW1lciB0aGUgd2F0Y2hkb2cgZmly ZXMsIGtpbGwgaXQuCisgICAgICAgIChJUEM6OkNvbm5lY3Rpb246OnRlcm1pbmF0ZVNvb24pOgor ICAgICAgICAgICAgLSBjcmVhdGUgYSBDb25uZWN0aW9uVGVybWluYXRpb25XYXRjaGRvZy4KKyAg ICAgICAgKiBVSVByb2Nlc3MvV2ViUHJvY2Vzc1Byb3h5LmNwcDoKKyAgICAgICAgKFdlYktpdDo6 V2ViUHJvY2Vzc1Byb3h5OjpyZW1vdmVXZWJQYWdlKToKKyAgICAgICAgICAgIC0gd2hlbiBkaXNj b25uZWN0aW5nIGZyb20gYSBwcm9jZXNzLCBmaXJzdCB0ZWxsIGl0IHRvIHRlcm1pbmF0ZVNvb24u CisKIDIwMTQtMDUtMjIgIEJlbmphbWluIFBvdWxhaW4gIDxicG91bGFpbkBhcHBsZS5jb20+CiAK ICAgICAgICAgRG8gbm90IGZvcmNlIGEgbGF5b3V0IHdoZW4gY2hhbmdpbmcgdGhlIEZpeGVkTGF5 b3V0U2l6ZSBpbiBXZWJQYWdlCkluZGV4OiBTb3VyY2UvV2ViS2l0Mi9QbGF0Zm9ybS9JUEMvQ29u bmVjdGlvbi5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJLaXQyL1BsYXRmb3JtL0lQQy9Db25u ZWN0aW9uLmgJKHJldmlzaW9uIDE2OTE3NikKKysrIFNvdXJjZS9XZWJLaXQyL1BsYXRmb3JtL0lQ Qy9Db25uZWN0aW9uLmgJKHdvcmtpbmcgY29weSkKQEAgLTE3OCw2ICsxNzgsNyBAQCBwdWJsaWM6 CiAKICNpZiBQTEFURk9STShDT0NPQSkKICAgICBib29sIGtpbGwoKTsKKyAgICB2b2lkIHRlcm1p bmF0ZVNvb24oZG91YmxlIHNlY29uZHMpOwogI2VuZGlmCiAKIHByaXZhdGU6CkluZGV4OiBTb3Vy Y2UvV2ViS2l0Mi9QbGF0Zm9ybS9JUEMvbWFjL0Nvbm5lY3Rpb25NYWMubW0KPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gU291cmNlL1dlYktpdDIvUGxhdGZvcm0vSVBDL21hYy9Db25uZWN0aW9uTWFjLm1tCShyZXZp c2lvbiAxNjkxNzYpCisrKyBTb3VyY2UvV2ViS2l0Mi9QbGF0Zm9ybS9JUEMvbWFjL0Nvbm5lY3Rp b25NYWMubW0JKHdvcmtpbmcgY29weSkKQEAgLTM2LDYgKzM2LDEwIEBACiAjaW5jbHVkZSA8d3Rm L1J1bkxvb3AuaD4KICNpbmNsdWRlIDx4cGMveHBjLmg+CiAKKyNpZiBQTEFURk9STShJT1MpCisj aW5jbHVkZSAiUHJvY2Vzc0Fzc2VydGlvbi5oIgorI2VuZGlmCisKICNpZiBfX2hhc19pbmNsdWRl KDx4cGMvcHJpdmF0ZS5oPikKICNpbmNsdWRlIDx4cGMvcHJpdmF0ZS5oPgogI2Vsc2UKQEAgLTY1 LDYgKzY5LDM2IEBAIGVudW0gewogICAgIE1lc3NhZ2VCb2R5SXNPdXRPZkxpbmUgPSAxIDw8IDAK IH07CiAgICAgCisvLyBDb25uZWN0aW9uVGVybWluYXRpb25XYXRjaGRvZyBkb2VzIHR3byB0aGlu Z3M6CisvLyAxKSBJdCBzZXRzIGEgd2F0Y2hkb2cgdGltZXIgdG8ga2lsbCB0aGUgcGVlcmVkIHBy b2Nlc3MuCisvLyAyKSBPbiBpT1MsIG1ha2UgdGhlIHByb2Nlc3MgcnVubmFibGUgZm9yIHRoZSBk dXJhdGlvbiBvZiB0aGUgd2F0Y2hkb2cKKy8vICAgIHRvIGVuc3VyZSBpdCBoYXMgYSBjaGFuY2Ug dG8gdGVybWluYXRlIGNsZWFubHkuCitjbGFzcyBDb25uZWN0aW9uVGVybWluYXRpb25XYXRjaGRv ZyB7CitwdWJsaWM6CisgICAgQ29ubmVjdGlvblRlcm1pbmF0aW9uV2F0Y2hkb2coWFBDUHRyPHhw Y19jb25uZWN0aW9uX3Q+JiB4cGNDb25uZWN0aW9uLCBkb3VibGUgc2Vjb25kcykKKyAgICAgICAg OiBtX3hwY0Nvbm5lY3Rpb24oeHBjQ29ubmVjdGlvbikKKyAgICAgICAgLCBtX3dhdGNoZG9nVGlt ZXIoUnVuTG9vcDo6bWFpbigpLCB0aGlzLCAmQ29ubmVjdGlvblRlcm1pbmF0aW9uV2F0Y2hkb2c6 OndhdGNoZG9nVGltZXJGaXJlZCkKKyNpZiBQTEFURk9STShJT1MpCisgICAgICAgICwgbV9hc3Nl cnRpb24oc3RkOjptYWtlX3VuaXF1ZTxXZWJLaXQ6OlByb2Nlc3NBc3NlcnRpb24+KHhwY19jb25u ZWN0aW9uX2dldF9waWQobV94cGNDb25uZWN0aW9uLmdldCgpKSwgV2ViS2l0OjpBc3NlcnRpb25T dGF0ZTo6QmFja2dyb3VuZCkpCisjZW5kaWYKKyAgICB7CisgICAgICAgIG1fd2F0Y2hkb2dUaW1l ci5zdGFydE9uZVNob3Qoc2Vjb25kcyk7CisgICAgfQorICAgIAorICAgIHZvaWQgd2F0Y2hkb2dU aW1lckZpcmVkKCkKKyAgICB7CisgICAgICAgIHhwY19jb25uZWN0aW9uX2tpbGwobV94cGNDb25u ZWN0aW9uLmdldCgpLCBTSUdLSUxMKTsKKyAgICAgICAgZGVsZXRlIHRoaXM7CisgICAgfQorCitw cml2YXRlOgorICAgIFhQQ1B0cjx4cGNfY29ubmVjdGlvbl90PiBtX3hwY0Nvbm5lY3Rpb247Cisg ICAgUnVuTG9vcDo6VGltZXI8Q29ubmVjdGlvblRlcm1pbmF0aW9uV2F0Y2hkb2c+IG1fd2F0Y2hk b2dUaW1lcjsKKyNpZiBQTEFURk9STShJT1MpCisgICAgc3RkOjp1bmlxdWVfcHRyPFdlYktpdDo6 UHJvY2Vzc0Fzc2VydGlvbj4gbV9hc3NlcnRpb247CisjZW5kaWYKK307CisgICAgCiB2b2lkIENv bm5lY3Rpb246OnBsYXRmb3JtSW52YWxpZGF0ZSgpCiB7CiAgICAgaWYgKCFtX2lzQ29ubmVjdGVk KQpAQCAtOTcsNyArMTMxLDEzIEBAIHZvaWQgQ29ubmVjdGlvbjo6cGxhdGZvcm1JbnZhbGlkYXRl KCkKIAogICAgIG1feHBjQ29ubmVjdGlvbiA9IG51bGxwdHI7CiB9Ci0KKyAgICAKK3ZvaWQgQ29u bmVjdGlvbjo6dGVybWluYXRlU29vbihkb3VibGUgc2Vjb25kcykKK3sKKyAgICBpZiAobV94cGND b25uZWN0aW9uKQorICAgICAgICBuZXcgQ29ubmVjdGlvblRlcm1pbmF0aW9uV2F0Y2hkb2cobV94 cGNDb25uZWN0aW9uLCBzZWNvbmRzKTsKK30KKyAgICAKIHZvaWQgQ29ubmVjdGlvbjo6cGxhdGZv cm1Jbml0aWFsaXplKElkZW50aWZpZXIgaWRlbnRpZmllcikKIHsKICNpZiAhUExBVEZPUk0oSU9T KQpJbmRleDogU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL1dlYlByb2Nlc3NQcm94eS5jcHAKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL1dlYlByb2Nlc3NQcm94eS5jcHAJ KHJldmlzaW9uIDE2OTE3NikKKysrIFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9XZWJQcm9jZXNz UHJveHkuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yMTAsNiArMjEwLDkgQEAgdm9pZCBXZWJQcm9j ZXNzUHJveHk6OnJlbW92ZVdlYlBhZ2UodWludAogICAgIC8vIFdlIG9ubHkgYWxsb3cgdGhpcyB3 aGVuIHVzaW5nIGEgbmV0d29yayBwcm9jZXNzLCBhcyBvdGhlcndpc2UgdGhlIFdlYlByb2Nlc3Mg bmVlZHMgdG8gcHJlc2VydmUgaXRzIHNlc3Npb24gc3RhdGUuCiAgICAgaWYgKG1fY29udGV4dC0+ dXNlc05ldHdvcmtQcm9jZXNzKCkgJiYgY2FuVGVybWluYXRlQ2hpbGRQcm9jZXNzKCkpIHsKICAg ICAgICAgYWJvcnRQcm9jZXNzTGF1bmNoSWZOZWVkZWQoKTsKKyNpZiBQTEFURk9STShJT1MpICYm IFVTRShYUENfU0VSVklDRVMpCisgICAgICAgIGNvbm5lY3Rpb24oKS0+dGVybWluYXRlU29vbigz MCk7CisjZW5kaWYKICAgICAgICAgZGlzY29ubmVjdCgpOwogICAgIH0KIH0K