Bug 200346 – Crash under WebProcessProxy::didBecomeUnresponsive()

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

<rdar://problem/53795984>

[Chris Dumez]

Created attachment 375313 [details]
Patch

[Chris Dumez]

Created attachment 375316 [details]
Patch

[Chris Dumez]

Created attachment 375317 [details]
Patch

[Geoffrey Garen]

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?

[Alex Christensen]

Comment on attachment 375317 [details]
Patch

CSSFontFaceSet does something a little cleaner than this.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 375330 [details]
Patch

[Chris Dumez]

Created attachment 375331 [details]
Patch

[Geoffrey Garen]

> > -- 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()?

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 375332 [details]
Patch

[Chris Dumez]

Created attachment 375333 [details]
Patch

[Chris Dumez]

Created attachment 375334 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375334 [details]
Patch

Clearing flags on attachment: 375334

Committed r248121: <https://trac.webkit.org/changeset/248121>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.