Bug 181180 – Safari WebKitWebRTCAudioModule crash during <video> tag update when audio track present in MediaStream

• Component: WebRTC
• Status: RESOLVED FIXED

Created attachment 330233 [details]
Four crash logs

I use RTCPeerConnection to stream audio and video from Chrome (63.0.3239.84) to Safari (Version 11.0.2 (12604.4.7.1.4)). Connection is established, audio and video are present on both sides. Then after a while in Safari, in my HTML app I update <video> tag that contains remote MediaStream (in srcObject) received from Chrome. By update I mean that I add and remove that video tag from HTML few times in row. It is done by some template rendering engine. During that update process Safari crashes (not always, but eventually will after several tries).

Other few things:
 - video tag has autoplay, muted, playsinline attributes and couple of css rules.
 - updating video with only MediaStreamTrack kind "video" won't trigger crash. Audio track has to be present.
 - I am able to reproduce it locally by calling several times something like setInterval(updateVideo, 0), where updateVideo is:
   $parent.removeChild($video); $parent.appendChild($video); $video.srcObject = stream; Note: This is test case only, in my app updateVideo is not called that many times.
 - crash logs in attachments. One of them is from High Sierra, Safari Technology Preview. Other from Sierra 10.12.6. This can be found in logs:
   Crashed Thread: WebKitWebRTCAudioModule, abort() called, *** error for object 0x7fd6e61ac4d0: pointer being freed was not allocated

[youenn fablet]

Thanks for the report pmikolajczak.

In the crash logs, AudioTrackPrivateMediaStreamCocoa is destroyed in a background thread since AudioTrackPrivateMediaStreamCocoa::audioSamplesAvailable is taking a ref from a background thread. Removing the tracks might remove all other refs of the AudioTrackPrivateMediaStreamCocoa.

[Radar WebKit Bug Importer]

<rdar://problem/36302375>

[youenn fablet]

Created attachment 335633 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 335633 [details]
Patch

Rejecting attachment 335633 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 335633, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
 (offset 1 line).
1 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp.rej
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/webrtc/video-update-often-expected.txt
patching file LayoutTests/webrtc/video-update-often.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Eric Carlson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/6927626

[youenn fablet]

Created attachment 336371 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 336371 [details]
Patch

Clearing flags on attachment: 336371

Committed r229898: <https://trac.webkit.org/changeset/229898>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

This seems like a fragile fix. If we ever add a deref on a background thread by accident, the bug will be reintroduced. Is there some way we can fix it in a way that does not require us to be careful about every deref?

[youenn fablet]

(In reply to Darin Adler from comment #8)
> This seems like a fragile fix. If we ever add a deref on a background thread
> by accident, the bug will be reintroduced. Is there some way we can fix it
> in a way that does not require us to be careful about every deref?

Given the hierarchy of this class, doing some cleaning of the members in the destructor does not seem great.

One approach would be to introduce a ThreadSafeRefCounted that would always destroy itself in the MainThread, something like:
class TrackPrivateBase : public ThreadSafeRefCounted<TrackPrivateBase, Destruction::InMainThread>

[youenn fablet]

Filed https://bugs.webkit.org/show_bug.cgi?id=183988 to improve on this.