Bug 187702 - Make sure LibWebRTCMediaEndpoint is always destroyed on the main thread
Summary: Make sure LibWebRTCMediaEndpoint is always destroyed on the main thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-16 10:26 PDT by Chris Dumez
Modified: 2018-07-16 11:56 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.47 KB, patch)
2018-07-16 10:32 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>