From 89f6954fa3fff68a306576fc336b01fdfeb27e2b Mon Sep 17 00:00:00 2001 From: William Candillon Date: Fri, 10 Jul 2026 12:07:28 +0200 Subject: [PATCH] =?UTF-8?q?fix(=F0=9F=8D=8E):=20plug=20makeImageFromView?= =?UTF-8?q?=20snapshot=20leak=20via=20mapper=20and=20view=20registry=20(#3?= =?UTF-8?q?932)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On iOS (and Android), every image drawn in a Canvas — most visibly makeImageFromView snapshots at ~W*H*4 bytes each — could stay resident after dispose(), GC, and unmount: 1. NativeReanimatedContainer never stopped its Reanimated mapper on unmount; stopMapper only ran at the start of the next redraw, which never comes for an unmounted Canvas. The leaked mapper closure permanently retained the recorder (whose commands hold sk_sp copies of every drawn image) and the SkPicture, so the CFData pixel copy made in ViewScreenshotService was never released. Stop the mapper in unmount() (native and web containers). 2. The leaked mapper (or the queued first-frame runOnUI) kept calling setJsiProperty(nativeId, "picture", ...) after the native view had unregistered, and ViewRegistry::withViewInfo recreated the erased entry, storing the sk_sp in the process-global registry under a nativeId that is never reused — a permanent leak. Tombstone unregistered ids so late property updates get a transient info object instead of resurrecting the entry (registration paths revive the id, which also keeps ids valid across JS reloads). 3. requestRedraw/makeImageSnapshot(Async)/size created empty registry entries for unknown ids; they now use a read-only lookup. --- packages/skia/cpp/rnskia/RNSkJsiViewApi.h | 70 ++++++++++++++-------- packages/skia/src/sksg/Container.native.ts | 12 ++++ packages/skia/src/sksg/Container.web.ts | 12 ++++ 3 files changed, 70 insertions(+), 24 deletions(-) diff --git a/packages/skia/cpp/rnskia/RNSkJsiViewApi.h b/packages/skia/cpp/rnskia/RNSkJsiViewApi.h index 9bcf60ed68..b48bd686b3 100644 --- a/packages/skia/cpp/rnskia/RNSkJsiViewApi.h +++ b/packages/skia/cpp/rnskia/RNSkJsiViewApi.h @@ -6,13 +6,14 @@ #include #include #include +#include #include #include -#include "jsi/JsiHostObject.h" #include "RNSkPictureView.h" #include "RNSkPlatformContext.h" #include "RNSkView.h" +#include "jsi/JsiHostObject.h" #include "jsi/ViewProperty.h" #include @@ -39,33 +40,56 @@ class ViewRegistry { void removeViewInfo(size_t id) { std::unique_lock lock(_mutex); _registry.erase(id); + // Remember that this id was explicitly unregistered. Property updates + // can arrive after unregistration (e.g. a Reanimated worklet setting the + // picture racing with unmount); without a tombstone they would recreate + // the entry and its props (an SkPicture retaining every image it draws) + // would stay in this global registry forever. + _unregistered.insert(id); } - // Execute a function while holding the registry lock + // Execute a function while holding the registry lock. + // When `revive` is true (registration paths), a previously unregistered id + // becomes valid again; otherwise calls for unregistered ids receive a + // transient info object that is not stored in the registry. template - auto withViewInfo(size_t id, F &&func) + auto withViewInfo(size_t id, F &&func, bool revive = false) -> decltype(func(std::shared_ptr())) { std::unique_lock lock(_mutex); + if (revive) { + _unregistered.erase(id); + } auto it = _registry.find(id); std::shared_ptr info; if (it != _registry.end()) { info = it->second; } else { info = std::make_shared(); - _registry[id] = info; + if (_unregistered.find(id) == _unregistered.end()) { + _registry[id] = info; + } } return func(info); } + // Read-only lookup: never creates a registry entry. + std::shared_ptr getView(size_t id) { + std::shared_lock lock(_mutex); + auto it = _registry.find(id); + return it != _registry.end() ? it->second->view : nullptr; + } + void clear() { std::unique_lock lock(_mutex); _registry.clear(); + _unregistered.clear(); } private: ViewRegistry() = default; mutable std::shared_mutex _mutex; std::unordered_map> _registry; + std::unordered_set _unregistered; }; class RNSkJsiViewApi : public RNJsi::JsiHostObject, @@ -137,13 +161,10 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, // find Skia View int nativeId = arguments[0].asNumber(); - ViewRegistry::getInstance().withViewInfo( - nativeId, [](std::shared_ptr info) { - if (info->view != nullptr) { - info->view->requestRedraw(); - } - return nullptr; - }); + auto view = ViewRegistry::getInstance().getView(nativeId); + if (view != nullptr) { + view->requestRedraw(); + } return jsi::Value::undefined(); } @@ -164,9 +185,8 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, // find Skia view int nativeId = arguments[0].asNumber(); sk_sp image; - std::shared_ptr view = ViewRegistry::getInstance().withViewInfo( - nativeId, - [](std::shared_ptr info) { return info->view; }); + std::shared_ptr view = + ViewRegistry::getInstance().getView(nativeId); if (view != nullptr) { if (count > 1 && !arguments[1].isUndefined() && !arguments[1].isNull()) { auto rect = JsiSkRect::fromValue(runtime, arguments[1]); @@ -202,9 +222,8 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, // find Skia view int nativeId = arguments[0].asNumber(); - std::shared_ptr view = ViewRegistry::getInstance().withViewInfo( - nativeId, - [](std::shared_ptr info) { return info->view; }); + std::shared_ptr view = + ViewRegistry::getInstance().getView(nativeId); auto context = _platformContext; auto bounds = count > 1 && !arguments[1].isUndefined() && !arguments[1].isNull() @@ -248,9 +267,8 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, // find Skia View int nativeId = arguments[0].asNumber(); - std::shared_ptr view = ViewRegistry::getInstance().withViewInfo( - nativeId, - [](std::shared_ptr info) { return info->view; }); + std::shared_ptr view = + ViewRegistry::getInstance().getView(nativeId); if (view != nullptr) { auto pixelDensity = _platformContext->getPixelDensity(); auto sizeObj = jsi::Object(runtime); @@ -293,7 +311,8 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, */ void registerSkiaView(size_t nativeId, std::shared_ptr view) { ViewRegistry::getInstance().withViewInfo( - nativeId, [&](std::shared_ptr info) { + nativeId, + [&](std::shared_ptr info) { info->view = view; info->view->setNativeId(nativeId); @@ -301,7 +320,8 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, info->props.clear(); return nullptr; - }); + }, + /* revive= */ true); } /** @@ -320,7 +340,8 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, */ void setSkiaView(size_t nativeId, std::shared_ptr view) { ViewRegistry::getInstance().withViewInfo( - nativeId, [&](std::shared_ptr info) { + nativeId, + [&](std::shared_ptr info) { if (view != nullptr) { info->view = view; info->view->setNativeId(nativeId); @@ -330,7 +351,8 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject, info->view = view; // Set to nullptr } return nullptr; - }); + }, + /* revive= */ view != nullptr); } private: diff --git a/packages/skia/src/sksg/Container.native.ts b/packages/skia/src/sksg/Container.native.ts index 08c16a9b2e..0f849f9d15 100644 --- a/packages/skia/src/sksg/Container.native.ts +++ b/packages/skia/src/sksg/Container.native.ts @@ -39,9 +39,21 @@ class NativeReanimatedContainer extends Container { this.picture = Skia.Picture.MakePicture(null)!; } + unmount() { + super.unmount(); + if (this.mapperId !== null) { + // The mapper closure retains the recorder and its resources (e.g. + // images) on the UI runtime and keeps updating the picture of an + // unmounted view — stop it or it leaks for the lifetime of the app. + Rea.stopMapper(this.mapperId); + this.mapperId = null; + } + } + redraw() { if (this.mapperId !== null) { Rea.stopMapper(this.mapperId); + this.mapperId = null; } if (this.unmounted) { return; diff --git a/packages/skia/src/sksg/Container.web.ts b/packages/skia/src/sksg/Container.web.ts index d71fc97eed..aecaebd57a 100644 --- a/packages/skia/src/sksg/Container.web.ts +++ b/packages/skia/src/sksg/Container.web.ts @@ -40,9 +40,21 @@ class ReanimatedContainer extends Container { super(Skia); } + unmount() { + super.unmount(); + if (this.mapperId !== null) { + // The mapper closure retains the recording and keeps updating the + // picture of an unmounted view — stop it or it leaks for the + // lifetime of the app. + Rea.stopMapper(this.mapperId); + this.mapperId = null; + } + } + redraw() { if (this.mapperId !== null) { Rea.stopMapper(this.mapperId); + this.mapperId = null; } if (this.unmounted) { return;