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
<rdar://problem/59184818>
Created attachment 389997 [details] Patch
Comment on attachment 389997 [details] Patch r=me
Comment on attachment 389997 [details] Patch Clearing flags on attachment: 389997 Committed r255989: <https://trac.webkit.org/changeset/255989>
All reviewed patches have been landed. Closing bug.
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? > Source/WebKit/UIProcess/WebProcessPool.cpp:1780 > + if (RefPtr<WebProcessProxy> process = m_serviceWorkerProcesses.get({ domain, sessionID }).get()) { Can this use makeRefPtr?
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.
(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>
Comment on attachment 389997 [details] 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).