Async Clipboard read prevents WebRTC IOSurfaces from being released

• Component: WebRTC
• Status: RESOLVED FIXED

[James Howard]

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.

[Wenson Hsieh]

Yeah, we should definitely not be blocking the web process when requesting paste access.

[Radar WebKit Bug Importer]

<rdar://problem/75601433>

[youenn fablet]

@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.

[Wenson Hsieh]

(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).

[youenn fablet]

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.

[youenn fablet]

Created attachment 423863 [details]
Patch

[youenn fablet]

Created attachment 423877 [details]
Patch

[youenn fablet]

> 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.

[James Howard]

(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

[EWS]

Committed r274778: <https://commits.webkit.org/r274778>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423877 [details].

[Wenson Hsieh]

(FYI — I filed https://bugs.webkit.org/show_bug.cgi?id=223597 as a followup, regarding  the async Clipboard API blocking the web process)

[Darin Adler]

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.

[Eric Carlson]

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.

[youenn fablet]

(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.