Bug 187702

Summary: Make sure LibWebRTCMediaEndpoint is always destroyed on the main thread
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: MediaAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, eric.carlson, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Chris Dumez 2018-07-16 10:26:49 PDT
Make sure LibWebRTCMediaEndpoint is always destroyed on the main thread since it has a Timer data member.
Comment 1 Chris Dumez 2018-07-16 10:32:37 PDT
Created attachment 345098 [details]
Patch
Comment 2 youenn fablet 2018-07-16 10:36:35 PDT
Comment on attachment 345098 [details]
Patch

It seems safer with the patch.
I wonder whether this is an actual issue though.
When the pc goes away, LibWebRTCMediaEndpoint::close() is called which stops the timer.
Or is it needed to destroy the timer as well?
Comment 3 Chris Dumez 2018-07-16 10:41:05 PDT
(In reply to youenn fablet from comment #2)
> Comment on attachment 345098 [details]
> Patch
> 
> It seems safer with the patch.
> I wonder whether this is an actual issue though.
> When the pc goes away, LibWebRTCMediaEndpoint::close() is called which stops
> the timer.
> Or is it needed to destroy the timer as well?

You'd still hit release assertions I believe, the Timer destructor calls stop() and stop() calls TimerBase::setNextFireTime() which has:
RELEASE_ASSERT(canAccessThreadLocalDataForThread(m_thread.get()));
Comment 4 Chris Dumez 2018-07-16 10:43:09 PDT
(In reply to Chris Dumez from comment #3)
> (In reply to youenn fablet from comment #2)
> > Comment on attachment 345098 [details]
> > Patch
> > 
> > It seems safer with the patch.
> > I wonder whether this is an actual issue though.
> > When the pc goes away, LibWebRTCMediaEndpoint::close() is called which stops
> > the timer.
> > Or is it needed to destroy the timer as well?
> 
> You'd still hit release assertions I believe, the Timer destructor calls
> stop() and stop() calls TimerBase::setNextFireTime() which has:
> RELEASE_ASSERT(canAccessThreadLocalDataForThread(m_thread.get()));

This became a RELEASE_ASSERT() very recently in http://trac.webkit.org/r233821 to try and find cases where we're destroying timers on the wrong thread (As this could explain rdar://problem/33352721).
Comment 5 WebKit Commit Bot 2018-07-16 11:55:14 PDT
Comment on attachment 345098 [details]
Patch

Clearing flags on attachment: 345098

Committed r233857: <https://trac.webkit.org/changeset/233857>
Comment 6 WebKit Commit Bot 2018-07-16 11:55:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2018-07-16 11:56:21 PDT
<rdar://problem/42249803>