Created attachment 423674 [details] Minimal repro. Also hosted at https://james-howard.github.io/rtcpaste/ Overview: Async Clipboard read blocks WebRTC decoded video IOSurfaces from being released. Steps to reproduce: - Have a page with a video element that is streaming video from a remote RTCPeerConnection - Use the async clipboard readText API, triggering the "Paste" callout. - See https://james-howard.github.io/rtcpaste/ for a minimal repro (also attached here) Results: - On a Mac, the WebContent process will grow 420v IOSurfaces from the decoder (as reported by vmmap). while true; do vmmap --summary 24137 | grep IOSurface; sleep 5; done IOSurface 39.4M 1688K 1688K 0K 0K 1688K 18.3M 15 IOSurface 39.4M 1688K 1688K 0K 0K 1688K 18.3M 15 IOSurface 70.0M 10.0M 10.0M 0K 0K 10.0M 22.3M 36 # Paste clicked here IOSurface 147.2M 86.8M 86.8M 0K 0K 86.8M 40.5M 211 IOSurface 201.8M 164.1M 164.1M 0K 0K 164.1M 18.3M 383 IOSurface 279.0M 241.3M 241.3M 0K 0K 241.3M 18.3M 558 IOSurface 356.3M 318.6M 318.6M 0K 0K 318.6M 18.3M 733 IOSurface 40.2M 2592K 2592K 0K 0K 2592K 18.3M 17 # Callout/context menu dismissed here - On an iOS device, the WebContent process will grow IOSurfaces and then jetsam after a short time. - JavaScript execution is paused until the "Paste" callout is tapped or dismissed (no timers or other events can fire). - Video does not freeze; it does continue to play Expected Results: - Process does not jetsam - JavaScript execution is not blocked on the "Paste" callout (timers and other events can still fire). System Info: Repros on all of the following tested configurations: - Mac (11.2 20D5042d) - Safari: Version 14.0.3 (16610.4.3) - Safari Technology Preview: Release 122 (Safari 14.2, WebKit 16612.1.6.2) - iPad OS 14.4 (18D52) Additional Info: - If a single page is both the sender and receiver of the video (e.g. https://webrtc.github.io/samples/src/content/peerconnection/pc1/), memory will not grow while the paste callout is up, but what will happen is the video will freeze (sending is blocked?). - Does not repro if the video element has a URL as its src (i.e. not playing a video from a WebRTC stream), video continues to play and memory does not grow.
Yeah, we should definitely not be blocking the web process when requesting paste access.
<rdar://problem/75601433>
@wenson, do you know what is happening. Even if clipboard is blocking the main thread, I would not expect IOSurfaces to be kept in a queue.
(In reply to youenn fablet from comment #3) > @wenson, do you know what is happening. > Even if clipboard is blocking the main thread, I would not expect IOSurfaces > to be kept in a queue. Unfortunately, I don't know why this would cause IOSurfaces to remain alive. I thought it might've just been a byproduct of hanging the web process' main thread... I'm curious if this still reproduces if we simply busy loop for a few seconds or so (in lieu of using the clipboard API).
Thinking about it more, we are storing in main thread one sample for canvas painting. If async clipboard is busy looping main thread, we might be storing the samples in the dispatch queue to main thread. I guess I can change this and use locks to get the sample instead.
Created attachment 423863 [details] Patch
Created attachment 423877 [details] Patch
> Additional Info: > - If a single page is both the sender and receiver of the video (e.g. https://webrtc.github.io/samples/src/content/peerconnection/pc1/), memory will not grow while the paste callout is up, but what will happen is the video will freeze (sending is blocked?). I cannot reproduce this. Not sure what happens there.
(In reply to youenn fablet from comment #8) > > Additional Info: > > - If a single page is both the sender and receiver of the video (e.g. https://webrtc.github.io/samples/src/content/peerconnection/pc1/), memory will not grow while the paste callout is up, but what will happen is the video will freeze (sending is blocked?). > > I cannot reproduce this. Not sure what happens there. I can't reproduce it either on Mac, but I can on iOS. Try this minimal example: https://james-howard.github.io/rtcpaste/pc.html
Committed r274778: <https://commits.webkit.org/r274778> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423877 [details].
(FYI — I filed https://bugs.webkit.org/show_bug.cgi?id=223597 as a followup, regarding the async Clipboard API blocking the web process)
Comment on attachment 423877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423877&action=review > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:280 > + if (!weakThis) > + return; Do we need to protect this here for the rest of the lambda’s duration? I’d think we would if there’s any way it can be released on a non-main thread.
Comment on attachment 423877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423877&action=review >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:280 >> + return; > > Do we need to protect this here for the rest of the lambda’s duration? I’d think we would if there’s any way it can be released on a non-main thread. MediaPlayerPrivateMediaStreamAVFObjC is owned MediaPlayer, which is owned by HTMLMediaElement, so it shouldn't be possible for it to be released on a non-main thread.
(In reply to Eric Carlson from comment #13) > Comment on attachment 423877 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423877&action=review > > >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:280 > >> + return; > > > > Do we need to protect this here for the rest of the lambda’s duration? I’d think we would if there’s any way it can be released on a non-main thread. > > MediaPlayerPrivateMediaStreamAVFObjC is owned MediaPlayer, which is owned by > HTMLMediaElement, so it shouldn't be possible for it to be released on a > non-main thread. Right, if MediaPlayerPrivateMediaStreamAVFObjC could be destroyed on non main thread, the weakThis check here would be meaningless and we would probably need to move it ref counted.