From 02aa31048c3692685c13d1528fe6649e1d8a3622 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Fri, 5 Aug 2022 15:25:26 -0700 Subject: [PATCH] browser(firefox): cross thread sync in screencast (#16320) * nsIScreencastServiceClient is not thread safe refcounted so we make nsScreencastService::Session a thread safe refcounted object and keep it alive while there are inflight frames. Once such frames get handled on the main thread we check if the session has been stopped. * Removed mCaptureCallbackCs in favor of atomic counter (mClient is not accessed only on the main thread). * HeadlessWindowCapturer now holds RefPtr to the headless window object to avoid use after free when clearing it as a listener on the widget. * ScreencastEncoder is not ref counted anymore. Pretty-diff: https://github.com/yury-s/gecko-dev/commit/5f5042ff1e489ae2142e5b9b25bc340eced83d46 --- browser_patches/firefox-beta/BUILD_NUMBER | 4 +- .../screencast/HeadlessWindowCapturer.h | 2 +- .../juggler/screencast/ScreencastEncoder.cpp | 4 +- .../juggler/screencast/ScreencastEncoder.h | 6 +- .../screencast/nsScreencastService.cpp | 67 +++++++++++-------- .../juggler/screencast/nsScreencastService.h | 2 +- browser_patches/firefox/BUILD_NUMBER | 4 +- .../screencast/HeadlessWindowCapturer.h | 2 +- .../juggler/screencast/ScreencastEncoder.cpp | 4 +- .../juggler/screencast/ScreencastEncoder.h | 6 +- .../screencast/nsScreencastService.cpp | 67 +++++++++++-------- .../juggler/screencast/nsScreencastService.h | 2 +- 12 files changed, 96 insertions(+), 74 deletions(-) diff --git a/browser_patches/firefox-beta/BUILD_NUMBER b/browser_patches/firefox-beta/BUILD_NUMBER index c543861d3c..2486d7d893 100644 --- a/browser_patches/firefox-beta/BUILD_NUMBER +++ b/browser_patches/firefox-beta/BUILD_NUMBER @@ -1,2 +1,2 @@ -1342 -Changed: yurys@chromium.org Thu Aug 4 18:33:22 PDT 2022 +1343 +Changed: yurys@chromium.org Fri Aug 5 15:17:14 PDT 2022 diff --git a/browser_patches/firefox-beta/juggler/screencast/HeadlessWindowCapturer.h b/browser_patches/firefox-beta/juggler/screencast/HeadlessWindowCapturer.h index 5c4498f776..9f67565c04 100644 --- a/browser_patches/firefox-beta/juggler/screencast/HeadlessWindowCapturer.h +++ b/browser_patches/firefox-beta/juggler/screencast/HeadlessWindowCapturer.h @@ -55,7 +55,7 @@ class HeadlessWindowCapturer : public webrtc::VideoCaptureModuleEx { private: void NotifyFrameCaptured(const webrtc::VideoFrame& frame); - mozilla::widget::HeadlessWidget* mWindow = nullptr; + RefPtr mWindow; rtc::RecursiveCriticalSection _callBackCs; std::set*> _dataCallBacks; std::set _rawFrameCallbacks; diff --git a/browser_patches/firefox-beta/juggler/screencast/ScreencastEncoder.cpp b/browser_patches/firefox-beta/juggler/screencast/ScreencastEncoder.cpp index 582813e35b..5891953392 100644 --- a/browser_patches/firefox-beta/juggler/screencast/ScreencastEncoder.cpp +++ b/browser_patches/firefox-beta/juggler/screencast/ScreencastEncoder.cpp @@ -299,7 +299,7 @@ ScreencastEncoder::~ScreencastEncoder() { } -RefPtr ScreencastEncoder::create(nsCString& errorString, const nsCString& filePath, int width, int height, const gfx::IntMargin& margin) +std::unique_ptr ScreencastEncoder::create(nsCString& errorString, const nsCString& filePath, int width, int height, const gfx::IntMargin& margin) { vpx_codec_iface_t* codec_interface = vpx_codec_vp8_cx(); if (!codec_interface) { @@ -340,7 +340,7 @@ RefPtr ScreencastEncoder::create(nsCString& errorString, cons std::unique_ptr vpxCodec(new VPXCodec(std::move(codec), cfg, file)); // fprintf(stderr, "ScreencastEncoder initialized with: %s\n", vpx_codec_iface_name(codec_interface)); - return new ScreencastEncoder(std::move(vpxCodec), margin); + return std::make_unique(std::move(vpxCodec), margin); } void ScreencastEncoder::flushLastFrame() diff --git a/browser_patches/firefox-beta/juggler/screencast/ScreencastEncoder.h b/browser_patches/firefox-beta/juggler/screencast/ScreencastEncoder.h index 07b99f2fdc..883ad01011 100644 --- a/browser_patches/firefox-beta/juggler/screencast/ScreencastEncoder.h +++ b/browser_patches/firefox-beta/juggler/screencast/ScreencastEncoder.h @@ -19,22 +19,20 @@ class VideoFrame; namespace mozilla { class ScreencastEncoder { - NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ScreencastEncoder) public: static constexpr int fps = 25; - static RefPtr create(nsCString& errorString, const nsCString& filePath, int width, int height, const gfx::IntMargin& margin); + static std::unique_ptr create(nsCString& errorString, const nsCString& filePath, int width, int height, const gfx::IntMargin& margin); class VPXCodec; ScreencastEncoder(std::unique_ptr, const gfx::IntMargin& margin); + ~ScreencastEncoder(); void encodeFrame(const webrtc::VideoFrame& videoFrame); void finish(std::function&& callback); private: - ~ScreencastEncoder(); - void flushLastFrame(); std::unique_ptr m_vpxCodec; diff --git a/browser_patches/firefox-beta/juggler/screencast/nsScreencastService.cpp b/browser_patches/firefox-beta/juggler/screencast/nsScreencastService.cpp index 1c673ee11b..20a766dc37 100644 --- a/browser_patches/firefox-beta/juggler/screencast/nsScreencastService.cpp +++ b/browser_patches/firefox-beta/juggler/screencast/nsScreencastService.cpp @@ -78,11 +78,10 @@ nsresult generateUid(nsString& uid) { class nsScreencastService::Session : public rtc::VideoSinkInterface, public webrtc::RawFrameCallback { - public: Session( nsIScreencastServiceClient* client, rtc::scoped_refptr&& capturer, - RefPtr&& encoder, + std::unique_ptr encoder, int width, int height, int viewportWidth, int viewportHeight, gfx::IntMargin margin, @@ -97,6 +96,20 @@ class nsScreencastService::Session : public rtc::VideoSinkInterface Create( + nsIScreencastServiceClient* client, + rtc::scoped_refptr&& capturer, + std::unique_ptr encoder, + int width, int height, + int viewportWidth, int viewportHeight, + gfx::IntMargin margin, + uint32_t jpegQuality) { + return do_AddRef(new Session(client, std::move(capturer), std::move(encoder), width, height, viewportWidth, viewportHeight, margin, jpegQuality)); + } bool Start() { webrtc::VideoCaptureCapability capability; @@ -119,6 +132,11 @@ class nsScreencastService::Session : public rtc::VideoSinkInterfaceDeRegisterCaptureDataCallback(this); else @@ -128,23 +146,23 @@ class nsScreencastService::Session : public rtc::VideoSinkInterfacefinish([client = std::move(mClient)] { + mEncoder->finish([this, protect = RefPtr{this}] { NS_DispatchToMainThread(NS_NewRunnableFunction( - "NotifyScreencastStopped", [client = std::move(client)]() -> void { - client->ScreencastStopped(); + "NotifyScreencastStopped", [this, protect = std::move(protect)]() -> void { + mClient->ScreencastStopped(); })); }); } else { - rtc::CritScope lock(&mCaptureCallbackCs); mClient->ScreencastStopped(); - mClient = nullptr; } } void ScreencastFrameAck() { - rtc::CritScope lock(&mCaptureCallbackCs); - --mFramesInFlight; + if (mFramesInFlight.load() == 0) { + fprintf(stderr, "ScreencastFrameAck is called while there are no inflight frames\n"); + return; + } + mFramesInFlight.fetch_sub(1); } // These callbacks end up running on the VideoCapture thread. @@ -167,15 +185,8 @@ class nsScreencastService::Session : public rtc::VideoSinkInterface mViewportHeight) pageHeight = mViewportHeight; - { - rtc::CritScope lock(&mCaptureCallbackCs); - if (mFramesInFlight >= kMaxFramesInFlight) { - return; - } - ++mFramesInFlight; - if (!mClient) - return; - } + if (mFramesInFlight.load() >= kMaxFramesInFlight) + return; int screenshotWidth = pageWidth; int screenshotHeight = pageHeight; @@ -256,21 +267,23 @@ class nsScreencastService::Session : public rtc::VideoSinkInterface void { + "NotifyScreencastFrame", [this, protect = RefPtr{this}, base64, pageWidth, pageHeight]() -> void { + if (mStopped) + return; NS_ConvertUTF8toUTF16 utf16(base64); - client->ScreencastFrame(utf16, pageWidth, pageHeight); + mClient->ScreencastFrame(utf16, pageWidth, pageHeight); })); } private: RefPtr mClient; rtc::scoped_refptr mCaptureModule; - RefPtr mEncoder; + std::unique_ptr mEncoder; uint32_t mJpegQuality; - rtc::RecursiveCriticalSection mCaptureCallbackCs; - uint32_t mFramesInFlight = 0; + bool mStopped = false; + std::atomic mFramesInFlight = 0; int mWidth; int mHeight; int mViewportWidth; @@ -322,7 +335,7 @@ nsresult nsScreencastService::StartVideoRecording(nsIScreencastServiceClient* aC margin.top += offsetTop; nsCString error; - RefPtr encoder; + std::unique_ptr encoder; if (isVideo) { encoder = ScreencastEncoder::create(error, PromiseFlatCString(aVideoFileName), width, height, margin); if (!encoder) { @@ -336,7 +349,7 @@ nsresult nsScreencastService::StartVideoRecording(nsIScreencastServiceClient* aC NS_ENSURE_SUCCESS(rv, rv); sessionId = uid; - auto session = std::make_unique(aClient, std::move(capturer), std::move(encoder), width, height, viewportWidth, viewportHeight, margin, isVideo ? 0 : quality); + auto session = Session::Create(aClient, std::move(capturer), std::move(encoder), width, height, viewportWidth, viewportHeight, margin, isVideo ? 0 : quality); if (!session->Start()) return NS_ERROR_FAILURE; mIdToSession.emplace(sessionId, std::move(session)); diff --git a/browser_patches/firefox-beta/juggler/screencast/nsScreencastService.h b/browser_patches/firefox-beta/juggler/screencast/nsScreencastService.h index 082416ca74..419603e0e6 100644 --- a/browser_patches/firefox-beta/juggler/screencast/nsScreencastService.h +++ b/browser_patches/firefox-beta/juggler/screencast/nsScreencastService.h @@ -23,7 +23,7 @@ class nsScreencastService final : public nsIScreencastService { ~nsScreencastService(); class Session; - std::map> mIdToSession; + std::map> mIdToSession; }; } // namespace mozilla diff --git a/browser_patches/firefox/BUILD_NUMBER b/browser_patches/firefox/BUILD_NUMBER index 5f4422dba3..397ac5e794 100644 --- a/browser_patches/firefox/BUILD_NUMBER +++ b/browser_patches/firefox/BUILD_NUMBER @@ -1,2 +1,2 @@ -1343 -Changed: yurys@chromium.org Thu Aug 4 18:29:49 PDT 2022 +1344 +Changed: yurys@chromium.org Fri Aug 5 15:15:08 PDT 2022 diff --git a/browser_patches/firefox/juggler/screencast/HeadlessWindowCapturer.h b/browser_patches/firefox/juggler/screencast/HeadlessWindowCapturer.h index 5c4498f776..9f67565c04 100644 --- a/browser_patches/firefox/juggler/screencast/HeadlessWindowCapturer.h +++ b/browser_patches/firefox/juggler/screencast/HeadlessWindowCapturer.h @@ -55,7 +55,7 @@ class HeadlessWindowCapturer : public webrtc::VideoCaptureModuleEx { private: void NotifyFrameCaptured(const webrtc::VideoFrame& frame); - mozilla::widget::HeadlessWidget* mWindow = nullptr; + RefPtr mWindow; rtc::RecursiveCriticalSection _callBackCs; std::set*> _dataCallBacks; std::set _rawFrameCallbacks; diff --git a/browser_patches/firefox/juggler/screencast/ScreencastEncoder.cpp b/browser_patches/firefox/juggler/screencast/ScreencastEncoder.cpp index 582813e35b..5891953392 100644 --- a/browser_patches/firefox/juggler/screencast/ScreencastEncoder.cpp +++ b/browser_patches/firefox/juggler/screencast/ScreencastEncoder.cpp @@ -299,7 +299,7 @@ ScreencastEncoder::~ScreencastEncoder() { } -RefPtr ScreencastEncoder::create(nsCString& errorString, const nsCString& filePath, int width, int height, const gfx::IntMargin& margin) +std::unique_ptr ScreencastEncoder::create(nsCString& errorString, const nsCString& filePath, int width, int height, const gfx::IntMargin& margin) { vpx_codec_iface_t* codec_interface = vpx_codec_vp8_cx(); if (!codec_interface) { @@ -340,7 +340,7 @@ RefPtr ScreencastEncoder::create(nsCString& errorString, cons std::unique_ptr vpxCodec(new VPXCodec(std::move(codec), cfg, file)); // fprintf(stderr, "ScreencastEncoder initialized with: %s\n", vpx_codec_iface_name(codec_interface)); - return new ScreencastEncoder(std::move(vpxCodec), margin); + return std::make_unique(std::move(vpxCodec), margin); } void ScreencastEncoder::flushLastFrame() diff --git a/browser_patches/firefox/juggler/screencast/ScreencastEncoder.h b/browser_patches/firefox/juggler/screencast/ScreencastEncoder.h index 07b99f2fdc..883ad01011 100644 --- a/browser_patches/firefox/juggler/screencast/ScreencastEncoder.h +++ b/browser_patches/firefox/juggler/screencast/ScreencastEncoder.h @@ -19,22 +19,20 @@ class VideoFrame; namespace mozilla { class ScreencastEncoder { - NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ScreencastEncoder) public: static constexpr int fps = 25; - static RefPtr create(nsCString& errorString, const nsCString& filePath, int width, int height, const gfx::IntMargin& margin); + static std::unique_ptr create(nsCString& errorString, const nsCString& filePath, int width, int height, const gfx::IntMargin& margin); class VPXCodec; ScreencastEncoder(std::unique_ptr, const gfx::IntMargin& margin); + ~ScreencastEncoder(); void encodeFrame(const webrtc::VideoFrame& videoFrame); void finish(std::function&& callback); private: - ~ScreencastEncoder(); - void flushLastFrame(); std::unique_ptr m_vpxCodec; diff --git a/browser_patches/firefox/juggler/screencast/nsScreencastService.cpp b/browser_patches/firefox/juggler/screencast/nsScreencastService.cpp index 1c673ee11b..20a766dc37 100644 --- a/browser_patches/firefox/juggler/screencast/nsScreencastService.cpp +++ b/browser_patches/firefox/juggler/screencast/nsScreencastService.cpp @@ -78,11 +78,10 @@ nsresult generateUid(nsString& uid) { class nsScreencastService::Session : public rtc::VideoSinkInterface, public webrtc::RawFrameCallback { - public: Session( nsIScreencastServiceClient* client, rtc::scoped_refptr&& capturer, - RefPtr&& encoder, + std::unique_ptr encoder, int width, int height, int viewportWidth, int viewportHeight, gfx::IntMargin margin, @@ -97,6 +96,20 @@ class nsScreencastService::Session : public rtc::VideoSinkInterface Create( + nsIScreencastServiceClient* client, + rtc::scoped_refptr&& capturer, + std::unique_ptr encoder, + int width, int height, + int viewportWidth, int viewportHeight, + gfx::IntMargin margin, + uint32_t jpegQuality) { + return do_AddRef(new Session(client, std::move(capturer), std::move(encoder), width, height, viewportWidth, viewportHeight, margin, jpegQuality)); + } bool Start() { webrtc::VideoCaptureCapability capability; @@ -119,6 +132,11 @@ class nsScreencastService::Session : public rtc::VideoSinkInterfaceDeRegisterCaptureDataCallback(this); else @@ -128,23 +146,23 @@ class nsScreencastService::Session : public rtc::VideoSinkInterfacefinish([client = std::move(mClient)] { + mEncoder->finish([this, protect = RefPtr{this}] { NS_DispatchToMainThread(NS_NewRunnableFunction( - "NotifyScreencastStopped", [client = std::move(client)]() -> void { - client->ScreencastStopped(); + "NotifyScreencastStopped", [this, protect = std::move(protect)]() -> void { + mClient->ScreencastStopped(); })); }); } else { - rtc::CritScope lock(&mCaptureCallbackCs); mClient->ScreencastStopped(); - mClient = nullptr; } } void ScreencastFrameAck() { - rtc::CritScope lock(&mCaptureCallbackCs); - --mFramesInFlight; + if (mFramesInFlight.load() == 0) { + fprintf(stderr, "ScreencastFrameAck is called while there are no inflight frames\n"); + return; + } + mFramesInFlight.fetch_sub(1); } // These callbacks end up running on the VideoCapture thread. @@ -167,15 +185,8 @@ class nsScreencastService::Session : public rtc::VideoSinkInterface mViewportHeight) pageHeight = mViewportHeight; - { - rtc::CritScope lock(&mCaptureCallbackCs); - if (mFramesInFlight >= kMaxFramesInFlight) { - return; - } - ++mFramesInFlight; - if (!mClient) - return; - } + if (mFramesInFlight.load() >= kMaxFramesInFlight) + return; int screenshotWidth = pageWidth; int screenshotHeight = pageHeight; @@ -256,21 +267,23 @@ class nsScreencastService::Session : public rtc::VideoSinkInterface void { + "NotifyScreencastFrame", [this, protect = RefPtr{this}, base64, pageWidth, pageHeight]() -> void { + if (mStopped) + return; NS_ConvertUTF8toUTF16 utf16(base64); - client->ScreencastFrame(utf16, pageWidth, pageHeight); + mClient->ScreencastFrame(utf16, pageWidth, pageHeight); })); } private: RefPtr mClient; rtc::scoped_refptr mCaptureModule; - RefPtr mEncoder; + std::unique_ptr mEncoder; uint32_t mJpegQuality; - rtc::RecursiveCriticalSection mCaptureCallbackCs; - uint32_t mFramesInFlight = 0; + bool mStopped = false; + std::atomic mFramesInFlight = 0; int mWidth; int mHeight; int mViewportWidth; @@ -322,7 +335,7 @@ nsresult nsScreencastService::StartVideoRecording(nsIScreencastServiceClient* aC margin.top += offsetTop; nsCString error; - RefPtr encoder; + std::unique_ptr encoder; if (isVideo) { encoder = ScreencastEncoder::create(error, PromiseFlatCString(aVideoFileName), width, height, margin); if (!encoder) { @@ -336,7 +349,7 @@ nsresult nsScreencastService::StartVideoRecording(nsIScreencastServiceClient* aC NS_ENSURE_SUCCESS(rv, rv); sessionId = uid; - auto session = std::make_unique(aClient, std::move(capturer), std::move(encoder), width, height, viewportWidth, viewportHeight, margin, isVideo ? 0 : quality); + auto session = Session::Create(aClient, std::move(capturer), std::move(encoder), width, height, viewportWidth, viewportHeight, margin, isVideo ? 0 : quality); if (!session->Start()) return NS_ERROR_FAILURE; mIdToSession.emplace(sessionId, std::move(session)); diff --git a/browser_patches/firefox/juggler/screencast/nsScreencastService.h b/browser_patches/firefox/juggler/screencast/nsScreencastService.h index 082416ca74..419603e0e6 100644 --- a/browser_patches/firefox/juggler/screencast/nsScreencastService.h +++ b/browser_patches/firefox/juggler/screencast/nsScreencastService.h @@ -23,7 +23,7 @@ class nsScreencastService final : public nsIScreencastService { ~nsScreencastService(); class Session; - std::map> mIdToSession; + std::map> mIdToSession; }; } // namespace mozilla