Bug 181180 - Safari WebKitWebRTCAudioModule crash during <video> tag update when audio track present in MediaStream
Summary: Safari WebKitWebRTCAudioModule crash during <video> tag update when audio tra...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: Safari 11
Hardware: Mac macOS 10.12
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-28 05:09 PST by pmikolajczak
Modified: 2018-03-24 17:19 PDT (History)
7 users (show)

See Also:


Attachments
Four crash logs (127.43 KB, application/zip)
2017-12-28 05:09 PST, pmikolajczak
no flags Details
Patch (6.22 KB, patch)
2018-03-12 13:49 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (6.29 KB, patch)
2018-03-23 07:56 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description pmikolajczak 2017-12-28 05:09:09 PST
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
Comment 1 youenn fablet 2017-12-28 08:24:23 PST
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.
Comment 2 Radar WebKit Bug Importer 2018-01-04 11:49:31 PST
<rdar://problem/36302375>
Comment 3 youenn fablet 2018-03-12 13:49:52 PDT
Created attachment 335633 [details]
Patch
Comment 4 WebKit Commit Bot 2018-03-12 18:30:24 PDT
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
Comment 5 youenn fablet 2018-03-23 07:56:44 PDT
Created attachment 336371 [details]
Patch
Comment 6 WebKit Commit Bot 2018-03-23 09:55:14 PDT
Comment on attachment 336371 [details]
Patch

Clearing flags on attachment: 336371

Committed r229898: <https://trac.webkit.org/changeset/229898>
Comment 7 WebKit Commit Bot 2018-03-23 09:55:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2018-03-23 20:39:11 PDT
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?
Comment 9 youenn fablet 2018-03-24 10:15:41 PDT
(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>
Comment 10 youenn fablet 2018-03-24 17:19:51 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=183988 to improve on this.