Bug 200346

Summary: Crash under WebProcessProxy::didBecomeUnresponsive()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, ggaren, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=197883
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2019-08-01 09:26:07 PDT
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
Comment 1 Chris Dumez 2019-08-01 09:26:22 PDT
<rdar://problem/53795984>
Comment 2 Chris Dumez 2019-08-01 09:28:45 PDT
Created attachment 375313 [details]
Patch
Comment 3 Chris Dumez 2019-08-01 09:33:48 PDT
Created attachment 375316 [details]
Patch
Comment 4 Chris Dumez 2019-08-01 09:39:05 PDT
Created attachment 375317 [details]
Patch
Comment 5 Geoffrey Garen 2019-08-01 11:11:53 PDT
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 6 Alex Christensen 2019-08-01 11:37:47 PDT
Comment on attachment 375317 [details]
Patch

CSSFontFaceSet does something a little cleaner than this.
Comment 7 Chris Dumez 2019-08-01 11:43:42 PDT
(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.
Comment 8 Chris Dumez 2019-08-01 11:49:02 PDT
(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.
Comment 9 Chris Dumez 2019-08-01 11:59:08 PDT
Created attachment 375330 [details]
Patch
Comment 10 Chris Dumez 2019-08-01 11:59:52 PDT
Created attachment 375331 [details]
Patch
Comment 11 Geoffrey Garen 2019-08-01 12:00:25 PDT
> > -- 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()?
Comment 12 Chris Dumez 2019-08-01 12:03:40 PDT
(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.
Comment 13 Chris Dumez 2019-08-01 12:05:45 PDT
Created attachment 375332 [details]
Patch
Comment 14 Chris Dumez 2019-08-01 12:08:27 PDT
Created attachment 375333 [details]
Patch
Comment 15 Chris Dumez 2019-08-01 12:10:58 PDT
Created attachment 375334 [details]
Patch
Comment 16 WebKit Commit Bot 2019-08-01 12:53:26 PDT
Comment on attachment 375334 [details]
Patch

Clearing flags on attachment: 375334

Committed r248121: <https://trac.webkit.org/changeset/248121>
Comment 17 WebKit Commit Bot 2019-08-01 12:53:27 PDT
All reviewed patches have been landed.  Closing bug.