Bug 207354

Summary: REGRESSION (r254706): Crash under WebProcessPool::terminateServiceWorkerProcess()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Service WorkersAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, commit-queue, darin, ggaren, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 206325    
Attachments:
Description Flags
Patch none

Description Chris Dumez 2020-02-06 14:28:23 PST 
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
Comment 1 Chris Dumez 2020-02-06 14:28:40 PST 
<rdar://problem/59184818>
Comment 2 Chris Dumez 2020-02-06 14:31:54 PST 
Created attachment 389997 [details]
Patch
Comment 3 Geoffrey Garen 2020-02-06 14:38:50 PST 
Comment on attachment 389997 [details]
Patch

r=me
Comment 4 WebKit Commit Bot 2020-02-06 15:35:12 PST 
Comment on attachment 389997 [details]
Patch

Clearing flags on attachment: 389997

Committed r255989: <https://trac.webkit.org/changeset/255989>
Comment 5 WebKit Commit Bot 2020-02-06 15:35:14 PST 
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2020-02-06 18:56:21 PST 
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 7 Chris Dumez 2020-02-07 08:14:01 PST 
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.
Comment 8 Chris Dumez 2020-02-07 08:30:12 PST 
(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 9 Darin Adler 2020-02-07 09:18:25 PST 
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).