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

Chris Dumez
Reported 2018-07-16 10:26:49 PDT
Make sure LibWebRTCMediaEndpoint is always destroyed on the main thread since it has a Timer data member.
Attachments
Patch (2.47 KB, patch)
2018-07-16 10:32 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2018-07-16 10:32:37 PDT
Created attachment 345098 [details] Patch
youenn fablet
Comment 2 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?
Chris Dumez
Comment 3 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()));
Chris Dumez
Comment 4 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).
WebKit Commit Bot
Comment 5 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>
WebKit Commit Bot
Comment 6 2018-07-16 11:55:16 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 7 2018-07-16 11:56:21 PDT
<rdar://problem/42249803>
Note You need to log in before you can comment on or make changes to this bug.