Crash under WebProcessProxy::didBecomeUnresponsive(): Crashed Thread: 0 Dispatch queue: com.apple.main-thread Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0xfffffffffffffff8 Exception Note: EXC_CORPSE_NOTIFY Termination Signal: Segmentation fault: 11 Termination Reason: Namespace SIGNAL, Code 0xb Terminating Process: exc handler [487] VM Regions Near 0xfffffffffffffff8: --> shared memory 00007ffffff5b000-00007ffffff5c000 [ 4K] r-x/r-x SM=SHM Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 ??? 0xfffffffffffffff8 0 + 18446744073709551608 1 com.apple.WebKit 0x00007fff40cd025e WebKit::WebProcessProxy::didBecomeUnresponsive() + 360 2 com.apple.JavaScriptCore 0x00007fff341bcdeb WTF::RunLoop::TimerBase::timerFired(__CFRunLoopTimer*, void*) + 27 3 com.apple.CoreFoundation 0x00007fff2fb338d4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20 4 com.apple.CoreFoundation 0x00007fff2fb3348e __CFRunLoopDoTimer + 872 5 com.apple.CoreFoundation 0x00007fff2fb32ea1 __CFRunLoopDoTimers + 317 6 com.apple.CoreFoundation 0x00007fff2fb144be __CFRunLoopRun + 2227 7 com.apple.CoreFoundation 0x00007fff2fb13996 CFRunLoopRunSpecific + 503 8 com.apple.HIToolbox 0x00007fff2e6c908d RunCurrentEventLoopInMode + 292 9 com.apple.HIToolbox 0x00007fff2e6c8dcd ReceiveNextEventCommon + 600 10 com.apple.HIToolbox 0x00007fff2e6c8b57 _BlockUntilNextEventMatchingListInModeWithFilter + 64 11 com.apple.AppKit 0x00007fff2cd836b0 _DPSNextEvent + 990 12 com.apple.AppKit 0x00007fff2cd82424 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1352 13 com.apple.Safari.framework 0x00007fff5ad72edf -[BrowserApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 251 14 com.apple.AppKit 0x00007fff2cd7cb82 -[NSApplication run] + 658 15 com.apple.AppKit 0x00007fff2cd6ea4e NSApplicationMain + 777 16 libdyld.dylib 0x00007fff670a2c35 start + 1
<rdar://problem/53795984>
Created attachment 375313 [details] Patch
Created attachment 375316 [details] Patch
Created attachment 375317 [details] Patch
Comment on attachment 375317 [details] Patch r=me I wonder why we don't just hold a RefPtr to our client -- or why our client doesn't clear itself when destroyed?
Comment on attachment 375317 [details] Patch CSSFontFaceSet does something a little cleaner than this.
(In reply to Geoffrey Garen from comment #5) > Comment on attachment 375317 [details] > Patch > > r=me > > I wonder why we don't just hold a RefPtr to our client You mean have the ResponsivenessTimer::Client subclass RefCounted<> instead of WebProcessProxy? > -- or why our client > doesn't clear itself when destroyed? The issue is that the client is the WebProcessProxy and that it has 2 data members (1 BackgroundResponsivenessTimer + 1 ResponsivenessTimer). There is no point in having the client unregister itself from the Timer at the moment because the Timer owns the Timer. Once the client is dead, so is the Timer.
(In reply to Chris Dumez from comment #7) > (In reply to Geoffrey Garen from comment #5) > > Comment on attachment 375317 [details] > > Patch > > > > r=me > > > > I wonder why we don't just hold a RefPtr to our client > > You mean have the ResponsivenessTimer::Client subclass RefCounted<> instead > of WebProcessProxy? > > > -- or why our client > > doesn't clear itself when destroyed? > > The issue is that the client is the WebProcessProxy and that it has 2 data > members (1 BackgroundResponsivenessTimer + 1 ResponsivenessTimer). There is > no point in having the client unregister itself from the Timer at the moment > because the Timer owns the Timer. Once the client is dead, so is the Timer. The *Client* owns the Timer.
Created attachment 375330 [details] Patch
Created attachment 375331 [details] Patch
> > -- or why our client > > doesn't clear itself when destroyed? > > The issue is that the client is the WebProcessProxy and that it has 2 data > members (1 BackgroundResponsivenessTimer + 1 ResponsivenessTimer). There is > no point in having the client unregister itself from the Timer at the moment > because the [ client ] owns the Timer. Once the client is dead, so is the Timer. I see: Our theory of this crash is that the client is live at the time of timer fire, but dies afterward? I guess that's possible. WebProcessProxy::didBecomeUnresponsive() already does "auto protectedThis = makeRef(*this);", but WebProcessProxy::didBecomeResponsive(), WebProcessProxy::willChangeIsResponsive(), and WebProcessProxy::didChangeIsResponsive() do not. Do we also need to cover ResponsivenessTimer::stop()?
(In reply to Geoffrey Garen from comment #11) > > > -- or why our client > > > doesn't clear itself when destroyed? > > > > The issue is that the client is the WebProcessProxy and that it has 2 data > > members (1 BackgroundResponsivenessTimer + 1 ResponsivenessTimer). There is > > no point in having the client unregister itself from the Timer at the moment > > because the [ client ] owns the Timer. Once the client is dead, so is the Timer. > > I see: Our theory of this crash is that the client is live at the time of > timer fire, but dies afterward? Yes, this is my latest theory since my protector inside WebProcessProxy::didBecomeUnresponsive() did not fix this crash. > > I guess that's possible. WebProcessProxy::didBecomeUnresponsive() already > does "auto protectedThis = makeRef(*this);", but > WebProcessProxy::didBecomeResponsive(), > WebProcessProxy::willChangeIsResponsive(), and > WebProcessProxy::didChangeIsResponsive() do not. Right. > Do we also need to cover ResponsivenessTimer::stop()? Same pattern so it seems better to cover it too. I'll update the patch.
Created attachment 375332 [details] Patch
Created attachment 375333 [details] Patch
Created attachment 375334 [details] Patch
Comment on attachment 375334 [details] Patch Clearing flags on attachment: 375334 Committed r248121: <https://trac.webkit.org/changeset/248121>
All reviewed patches have been landed. Closing bug.