From 6a710670f0c024fab4f99aa98fe8484d2a4ee697 Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Thu, 8 Jun 2023 06:51:21 -0700 Subject: [PATCH] Handle double SurfaceHandler#stop call (#37716) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/37716 We call `surfaceHandler.stop` both from `SurfaceHandlerBinding` as well as `Binding`. Since we don't check asserts in production, the second one should generally be a no-op, but may be causing a crash due to incorrectly de-referencing a unique_ptr. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D46441620 fbshipit-source-id: e93b9722fd717947ebf772f545f692c2e31f816e --- .../renderer/scheduler/SurfaceHandler.cpp | 4 +++- .../react/renderer/uimanager/UIManager.cpp | 22 +++++++++---------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp b/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp index 6a1cbc8bb3e..ebfeb2b0860 100644 --- a/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp +++ b/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp @@ -108,7 +108,9 @@ void SurfaceHandler::stop() const noexcept { // mounted views, so we need to commit an empty tree to trigger all // side-effects (including destroying and removing mounted views). react_native_assert(shadowTree && "`shadowTree` must not be null."); - shadowTree->commitEmptyTree(); + if (shadowTree) { + shadowTree->commitEmptyTree(); + } } void SurfaceHandler::setDisplayMode(DisplayMode displayMode) const noexcept { diff --git a/packages/react-native/ReactCommon/react/renderer/uimanager/UIManager.cpp b/packages/react-native/ReactCommon/react/renderer/uimanager/UIManager.cpp index 610f4dc1256..291195057ca 100644 --- a/packages/react-native/ReactCommon/react/renderer/uimanager/UIManager.cpp +++ b/packages/react-native/ReactCommon/react/renderer/uimanager/UIManager.cpp @@ -231,19 +231,19 @@ ShadowTree::Unique UIManager::stopSurface(SurfaceId surfaceId) const { // Waiting for all concurrent commits to be finished and unregistering the // `ShadowTree`. auto shadowTree = getShadowTreeRegistry().remove(surfaceId); + if (shadowTree) { + // We execute JavaScript/React part of the process at the very end to + // minimize any visible side-effects of stopping the Surface. Any possible + // commits from the JavaScript side will not be able to reference a + // `ShadowTree` and will fail silently. + runtimeExecutor_([=](jsi::Runtime &runtime) { + SurfaceRegistryBinding::stopSurface(runtime, surfaceId); + }); - // We execute JavaScript/React part of the process at the very end to minimize - // any visible side-effects of stopping the Surface. Any possible commits from - // the JavaScript side will not be able to reference a `ShadowTree` and will - // fail silently. - runtimeExecutor_([=](jsi::Runtime &runtime) { - SurfaceRegistryBinding::stopSurface(runtime, surfaceId); - }); - - if (leakChecker_) { - leakChecker_->stopSurface(surfaceId); + if (leakChecker_) { + leakChecker_->stopSurface(surfaceId); + } } - return shadowTree; }