Summary: | RealtimeOutgoingAudioSource and RealtimeOutgoingVideoSource should be destroyed on the main thread | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, eric.carlson, ews-watchlist, webkit-bug-importer, youennf | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | Other | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
youenn fablet
2018-03-08 14:50:28 PST
Created attachment 335358 [details]
Patch
Attachment 335358 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:39: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 335359 [details]
Patch
Comment on attachment 335359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335359&action=review > Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:79 > + RefPtr<Thread> m_thread; Is this OK? Are we sure this does not create a cycle? > Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:84 > + // Free senders in a different thread like could be done with actual peer connection. So we can just free senders on any thread but not the current one? > Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:86 > + auto threadFunction = [this, keeper = keeper.copyRef(), senders = WTFMove(m_senders)] { }; Why is this capture this? Any use of this would likely crash. Comment on attachment 335359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335359&action=review >> Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:79 >> + RefPtr<Thread> m_thread; > > Is this OK? Are we sure this does not create a cycle? It should not create a cycle, since the thread entry function is deleted after being called. That said, looking at it further, there is probably no need for ThreadKeeper at all. >> Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:84 >> + // Free senders in a different thread like could be done with actual peer connection. > > So we can just free senders on any thread but not the current one? Since it is a mock, this would always be freed in the main thread. And we want to test the case of sources being derefed in a non main thread. >> Source/WebCore/testing/MockLibWebRTCPeerConnection.cpp:86 >> + auto threadFunction = [this, keeper = keeper.copyRef(), senders = WTFMove(m_senders)] { }; > > Why is this capture this? Any use of this would likely crash. Not needed indeed. Created attachment 335427 [details]
Patch
Comment on attachment 335427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335427&action=review > Source/WebCore/testing/MockLibWebRTCPeerConnection.h:217 > + bool SetTrack(webrtc::MediaStreamTrackInterface* track) final > + { > + m_track = track; > + return true; > + } Is this related? A note in the ChangeLog about why this was changed would be helpful. Created attachment 335432 [details]
Patch for landing
(In reply to Eric Carlson from comment #8) > Comment on attachment 335427 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335427&action=review > > > Source/WebCore/testing/MockLibWebRTCPeerConnection.h:217 > > + bool SetTrack(webrtc::MediaStreamTrackInterface* track) final > > + { > > + m_track = track; > > + return true; > > + } > > Is this related? A note in the ChangeLog about why this was changed would be > helpful. Thanks for the review. The track change ensures that we keep a reference if we use SetTrack so that it can be freed later on a wrong thread. This is not strictly needed but might help in the future. Comment on attachment 335432 [details] Patch for landing Clearing flags on attachment: 335432 Committed r229491: <https://trac.webkit.org/changeset/229491> All reviewed patches have been landed. Closing bug. |