207354 2020-02-06 14:28:23 -0800 REGRESSION (r254706): Crash under WebProcessPool::terminateServiceWorkerProcess() 2020-02-07 09:18:25 -0800 1 1 1 Unclassified WebKit Service Workers WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 206325 1 cdumez cdumez achristensen beidson commit-queue darin ggaren webkit-bug-importer youennf oldest_to_newest 1615812 0 cdumez 2020-02-06 14:28:23 -0800 REGRESSION (r254706): Crash under WebProcessPool::terminateServiceWorkerProcess(): Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000000000020) [ 0] 0x00000001acec5d38 WebKit`WebKit::AuxiliaryProcessProxy::state() const [inlined] WTF::RefPtr<WebKit::ProcessLauncher, WTF::DumbPtrTraits<WebKit::ProcessLauncher> >::operator bool() const at RefPtr.h:87:47 -> 0x00000001acec5d38: ldr x8, [x0, #0x20] 0x00000001acec5d3c: cbz x8, 0x20bd50 ; <+24> [inlined] WTF::RefPtr<IPC::Connection, WTF::DumbPtrTraits<IPC::Connection> >::operator!() const at AuxiliaryProcessProxy.cpp:117 0x00000001acec5d40: ldrb w8, [x8, #0x58] 0x00000001acec5d44: cbz w8, 0x20bd50 ; <+24> [inlined] WTF::RefPtr<IPC::Connection, WTF::DumbPtrTraits<IPC::Connection> >::operator!() const at AuxiliaryProcessProxy.cpp:117 0x00000001acec5d48: mov w0, #0x0 [ 0] 0x00000001acec5d38 WebKit`WebKit::AuxiliaryProcessProxy::state() const at AuxiliaryProcessProxy.cpp:114 110 } 111 112 AuxiliaryProcessProxy::State AuxiliaryProcessProxy::state() const 113 { -> 114 if (m_processLauncher && m_processLauncher->isLaunching()) 115 return AuxiliaryProcessProxy::State::Launching; 116 117 if (!m_connection) 118 return AuxiliaryProcessProxy::State::Terminated; [ 1] 0x00000001acf759e3 WebKit`WebKit::WebProcessProxy::requestTermination(WebKit::ProcessTerminationReason) + 55 at WebProcessProxy.cpp:1084:9 1080 } 1081 1082 void WebProcessProxy::requestTermination(ProcessTerminationReason reason) 1083 { -> 1084 if (state() == State::Terminated) 1085 return; 1086 1087 auto protectedThis = makeRef(*this); 1088 RELEASE_LOG_IF(isReleaseLoggingAllowed(), Process, "%p - WebProcessProxy::requestTermination - reason %d", this, reason); [ 2] 0x00000001acf7595f WebKit`WebKit::WebProcessPool::terminateServiceWorkerProcess(WebCore::RegistrableDomain const&, PAL::SessionID const&) + 183 at WebProcessPool.cpp:1788:18 1784 #if ENABLE(SERVICE_WORKER) 1785 auto protectedThis = makeRef(*this); 1786 if (auto process = m_serviceWorkerProcesses.get({ domain, sessionID })) { 1787 process->disableServiceWorkers(); -> 1788 process->requestTermination(ProcessTerminationReason::ExceededCPULimit); 1789 } 1790 #endif 1791 } 1792 [ 3] 0x00000001ad01e5cf WebKit`WebKit::NetworkProcessProxy::terminateUnresponsiveServiceWorkerProcesses(WebCore::RegistrableDomain&&, PAL::SessionID) + 63 at NetworkProcessProxy.cpp:430:22 [ 4] 0x00000001acd2c227 WebKit`WebKit::NetworkProcessProxy::didReceiveNetworkProcessProxyMessage(IPC::Connection&, IPC::Decoder&) [inlined] void IPC::callMemberFunctionImpl<WebKit::NetworkProcessProxy, void (WebKit::NetworkProcessProxy::*)(WebCore::RegistrableDomain&&, PAL::SessionID), std::__1::tuple<WebCore::RegistrableDomain, PAL::SessionID>, 0ul, 1ul>(WebKit::NetworkProcessProxy*, void (WebKit::NetworkProcessProxy::*)(WebCore::RegistrableDomain&&, PAL::SessionID), std::__1::tuple<WebCore::RegistrableDomain, PAL::SessionID>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>) + 15 at HandleMessage.h:41:5 [ 4] 0x00000001acd2c218 WebKit`WebKit::NetworkProcessProxy::didReceiveNetworkProcessProxyMessage(IPC::Connection&, IPC::Decoder&) [inlined] void IPC::callMemberFunction<WebKit::NetworkProcessProxy, void (WebKit::NetworkProcessProxy::*)(WebCore::RegistrableDomain&&, PAL::SessionID), std::__1::tuple<WebCore::RegistrableDomain, PAL::SessionID>, std::__1::integer_sequence<unsigned long, 0ul, 1ul> >(std::__1::tuple<WebCore::RegistrableDomain, PAL::SessionID>&&, WebKit::NetworkProcessProxy*, void (WebKit::NetworkProcessProxy::*)(WebCore::RegistrableDomain&&, PAL::SessionID)) at HandleMessage.h:47 [ 4] 0x00000001acd2c218 WebKit`WebKit::NetworkProcessProxy::didReceiveNetworkProcessProxyMessage(IPC::Connection&, IPC::Decoder&) [inlined] void IPC::handleMessage<Messages::NetworkProcessProxy::TerminateUnresponsiveServiceWorkerProcesses, WebKit::NetworkProcessProxy, void (WebKit::NetworkProcessProxy::*)(WebCore::RegistrableDomain&&, PAL::SessionID)>(IPC::Decoder&, WebKit::NetworkProcessProxy*, void (WebKit::NetworkProcessProxy::*)(WebCore::RegistrableDomain&&, PAL::SessionID)) + 32 at HandleMessage.h:120 [ 4] 0x00000001acd2c1f8 WebKit`WebKit::NetworkProcessProxy::didReceiveNetworkProcessProxyMessage(IPC::Connection&, IPC::Decoder&) + 1360 at NetworkProcessProxyMessageReceiver.cpp:215 1615813 1 cdumez 2020-02-06 14:28:40 -0800 <rdar://problem/59184818> 1615819 2 389997 cdumez 2020-02-06 14:31:54 -0800 Created attachment 389997 Patch 1615833 3 389997 ggaren 2020-02-06 14:38:50 -0800 Comment on attachment 389997 Patch r=me 1615886 4 389997 commit-queue 2020-02-06 15:35:12 -0800 Comment on attachment 389997 Patch Clearing flags on attachment: 389997 Committed r255989: <https://trac.webkit.org/changeset/255989> 1615887 5 commit-queue 2020-02-06 15:35:14 -0800 All reviewed patches have been landed. Closing bug. 1615998 6 389997 darin 2020-02-06 18:56:21 -0800 Comment on attachment 389997 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389997&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:1779 > auto protectedThis = makeRef(*this); Is this still helpful? > Source/WebKit/UIProcess/WebProcessPool.cpp:1780 > + if (RefPtr<WebProcessProxy> process = m_serviceWorkerProcesses.get({ domain, sessionID }).get()) { Can this use makeRefPtr? 1616145 7 389997 cdumez 2020-02-07 08:14:01 -0800 Comment on attachment 389997 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389997&action=review >> Source/WebKit/UIProcess/WebProcessPool.cpp:1779 >> auto protectedThis = makeRef(*this); > > Is this still helpful? I don't think so since the WebProcessProxy refs its WebProcessPool. That said, I don't think it hurts much to keep it and it did not feel right to remove a protector in a crash fix. >> Source/WebKit/UIProcess/WebProcessPool.cpp:1780 >> + if (RefPtr<WebProcessProxy> process = m_serviceWorkerProcesses.get({ domain, sessionID }).get()) { > > Can this use makeRefPtr? Yes, I did not realize this was the preferred pattern. I can switch. 1616154 8 cdumez 2020-02-07 08:30:12 -0800 (In reply to Chris Dumez from comment #7) > Comment on attachment 389997 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=389997&action=review > > >> Source/WebKit/UIProcess/WebProcessPool.cpp:1779 > >> auto protectedThis = makeRef(*this); > > > > Is this still helpful? > > I don't think so since the WebProcessProxy refs its WebProcessPool. That > said, I don't think it hurts much to keep it and it did not feel right to > remove a protector in a crash fix. > > >> Source/WebKit/UIProcess/WebProcessPool.cpp:1780 > >> + if (RefPtr<WebProcessProxy> process = m_serviceWorkerProcesses.get({ domain, sessionID }).get()) { > > > > Can this use makeRefPtr? > > Yes, I did not realize this was the preferred pattern. I can switch. <https://trac.webkit.org/changeset/256022> 1616177 9 389997 darin 2020-02-07 09:18:25 -0800 Comment on attachment 389997 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389997&action=review >>> Source/WebKit/UIProcess/WebProcessPool.cpp:1780 >>> + if (RefPtr<WebProcessProxy> process = m_serviceWorkerProcesses.get({ domain, sessionID }).get()) { >> >> Can this use makeRefPtr? > > Yes, I did not realize this was the preferred pattern. I can switch. Not sure we have 100% consensus. What I like about makeRefPtr is that it emphasizes the lifetime part without possible doing an upcast as a side effect. For example, makeRefPtr on a HTMLPluginElement rather than using RefPtr<Element>. What some others don’t like about makeRefPtr is that the concrete type is implicit and you can’t see it when reading the code (same as with other cases where we use auto or an expression without putting it into local variable). 389997 2020-02-06 14:31:54 -0800 2020-02-06 15:35:12 -0800 Patch bug-207354-20200206143154.patch text/plain 1892 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjU1OTc0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGRhOTY1ZWYwZmU0MGQxYzY5 ZjA3Y2ZiZTkzNDlkOGUxYmI5NGM1ZTcuLmNjYmJmYTVhMTc0MGNmZWU3MjA1MDBmNDNiOGIyYmVh OTc2ZDcwM2UgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjAgQEAKKzIwMjAtMDItMDYgIENocmlzIER1 bWV6ICA8Y2R1bWV6QGFwcGxlLmNvbT4KKworICAgICAgICBSRUdSRVNTSU9OIChyMjU0NzA2KTog Q3Jhc2ggdW5kZXIgV2ViUHJvY2Vzc1Bvb2w6OnRlcm1pbmF0ZVNlcnZpY2VXb3JrZXJQcm9jZXNz KCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIwNzM1 NAorICAgICAgICA8cmRhcjovL3Byb2JsZW0vNTkxODQ4MTg+CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLCBub3QgZWFzaWx5IHRl c3RhYmxlIEFGQUlLIHNpbmNlIHRoaXMgaGFwcGVucyBvbiBmYWlsdXJlIHRvIHNlbmQgc3luYyBJ UEMgdG8KKyAgICAgICAgdGhlIHNlcnZpY2Ugd29ya2VyIHdoZW4gdGVybWluYXRpbmcgaXQuCisK KyAgICAgICAgKiBVSVByb2Nlc3MvV2ViUHJvY2Vzc1Bvb2wuY3BwOgorICAgICAgICAoV2ViS2l0 OjpXZWJQcm9jZXNzUG9vbDo6dGVybWluYXRlU2VydmljZVdvcmtlclByb2Nlc3MpOgorICAgICAg ICAnYXV0bycgcmVzb2x2ZWQgdG8gJ1dlYWtQdHI8V2ViUHJvY2Vzc1Byb3h5PicgaW4gdGhpcyBt ZXRob2QgYW5kIHRoZSBjYWxsIHRvCisgICAgICAgIGRpc2FibGVTZXJ2aWNlV29ya2VycygpIGNv dWxkIGNhdXNlIHRoZSBwcm9jZXNzIHRvIGdldCBkZXN0cm95ZWQuIFdlIHdvdWxkIHRoZW4KKyAg ICAgICAgZG8gYSBudWxsIGRlcmVmZXJlbmNlIG9uIHRoZSBuZXh0IGxpbmUuCisKIDIwMjAtMDIt MDYgIENocmlzIER1bWV6ICA8Y2R1bWV6QGFwcGxlLmNvbT4KIAogICAgICAgICBVbnJldmlld2Vk LCByb2xsaW5nIG91dCByMjU1OTU1LgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9VSVByb2Nl c3MvV2ViUHJvY2Vzc1Bvb2wuY3BwIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvV2ViUHJvY2Vz c1Bvb2wuY3BwCmluZGV4IGFhYzMzMmU5YjQ2NzY3YTI3MDM4NDlmMmU4NDMxOTU0Y2QyMDZhYjku LmFkYzY0YjUxYTkxNDhmNDY1ZDdlYTFhOWVjZWJkOTNkM2VhMzRiZTEgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJLaXQvVUlQcm9jZXNzL1dlYlByb2Nlc3NQb29sLmNwcAorKysgYi9Tb3VyY2UvV2Vi S2l0L1VJUHJvY2Vzcy9XZWJQcm9jZXNzUG9vbC5jcHAKQEAgLTE3NzcsNyArMTc3Nyw3IEBAIHZv aWQgV2ViUHJvY2Vzc1Bvb2w6OnRlcm1pbmF0ZVNlcnZpY2VXb3JrZXJQcm9jZXNzKGNvbnN0IFdl YkNvcmU6OlJlZ2lzdHJhYmxlRG9tCiB7CiAjaWYgRU5BQkxFKFNFUlZJQ0VfV09SS0VSKQogICAg IGF1dG8gcHJvdGVjdGVkVGhpcyA9IG1ha2VSZWYoKnRoaXMpOwotICAgIGlmIChhdXRvIHByb2Nl c3MgPSBtX3NlcnZpY2VXb3JrZXJQcm9jZXNzZXMuZ2V0KHsgZG9tYWluLCBzZXNzaW9uSUQgfSkp IHsKKyAgICBpZiAoUmVmUHRyPFdlYlByb2Nlc3NQcm94eT4gcHJvY2VzcyA9IG1fc2VydmljZVdv cmtlclByb2Nlc3Nlcy5nZXQoeyBkb21haW4sIHNlc3Npb25JRCB9KS5nZXQoKSkgewogICAgICAg ICBwcm9jZXNzLT5kaXNhYmxlU2VydmljZVdvcmtlcnMoKTsKICAgICAgICAgcHJvY2Vzcy0+cmVx dWVzdFRlcm1pbmF0aW9uKFByb2Nlc3NUZXJtaW5hdGlvblJlYXNvbjo6RXhjZWVkZWRDUFVMaW1p dCk7CiAgICAgfQo=