fix(tauri-runtime-wry): stop leaking ObjC retains in with_webview - #15224
Conversation
|
Hey @Legend-Master Just a small fix for the Apple WithWebview pointer handoff. Swapped Retained::into_raw for a scoped Retained + as_ptr to keep ownership balanced. Also fixed the .changes metadata so the checks pass cleanly. No API changes or anything big. Attached a stress run with stable RSS. Lemme know! |
Package Changes Through 9c3ef2dThere are 14 changes which include tauri with minor, tauri-bundler with minor, tauri-cli with minor, @tauri-apps/cli with minor, tauri-utils with minor, tauri-build with minor, tauri-macos-sign with minor, tauri-runtime-wry with minor, tauri-runtime with minor, tauri-codegen with minor, tauri-macros with minor, tauri-plugin with minor, tauri-driver with minor, @tauri-apps/api with minor Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
|
Transforming a |
|
Just a follow-up, any updates? |
|
Hi @krishpranav — are you still working on this? Asking because we're seeing the same leak in a shipping app. On macOS each On @sftse's cast note — we borrowed with Would you be able to update the PR along those lines? |
There was a problem hiding this comment.
Thanks, just a bit of nitpicks and you'll need to sign your commits for me to merge this
Also, let's add a todo comment on those fields to change the types to *const std::ffi::c_void in v3, as well as documenting you should not try to mutate those pointer (there shouldn't be a case in normal use cases for you to mutate them anyways)
| let ns_window = webview.ns_window(); | ||
| f(Webview { | ||
| webview: Retained::into_raw(webview.webview()) as *mut objc2::runtime::AnyObject | ||
| webview: Retained::as_ptr(&platform_webview) as *const std::ffi::c_void |
There was a problem hiding this comment.
| webview: Retained::as_ptr(&platform_webview) as *const std::ffi::c_void | |
| webview: Retained::as_ptr(&platform_webview).cast_mut() |
Also the other ones
|
Hi, sure will do those changes. |
WebviewMessage::WithWebview converted Retained<T> values to raw pointers with Retained::into_raw on Apple targets, transferring ownership without a corresponding release in this path. Use scoped Retained bindings and Retained::as_ptr for pointer handoff to the callback payload. This preserves borrowing semantics for callback lifetime and keeps Objective-C retain/release balanced when the retained values drop after callback return. Fixes tauri-apps#15210
Format .changes/fix-objc-retain-leak.md with valid covector front matter and package bump metadata. This resolves CI failures in check-change-tags and covector status for this PR.
Add a doc/TODO note on the Apple with_webview pointer fields (webview/manager/ns_window/view_controller): they're borrowed from ObjC Retained handles and must not be mutated through, and should become *const c_void in v3.
ea0d25d to
7bc0dc5
Compare
|
@Legend-Master rebased and signed all commits, added that TODO/doc comment you asked for should be good for a re-review whenever you get a chance! |
|
In case you didn't see |
Apply Legend-Master's suggestion: use .cast_mut() instead of casting through *const c_void first, across all macOS/iOS with_webview fields.
|
@Legend-Master it's done. |
|
This stops the leak for sure because it doesn't take "ownership" of the object anymore, but it's not super clear to me whether this occurs at the cost of a crash. The |
|
The lifetime is a problem here for sure... Although you should not do, you can send the Since We can also deprecate the old methods and adding in new safe |
Yeah, was thinking along those lines. There's a lot of channels involved with Tauri, and I honestly don't have the full picture of what the lifecycle of the objects are. As a rule of thumb, borrowing and channels interact poorly, it's either borrowing with apis like in |
|
Actually, |
…uri-apps#15224) * tauri-runtime-wry: avoid leaking ObjC retains in with_webview WebviewMessage::WithWebview converted Retained<T> values to raw pointers with Retained::into_raw on Apple targets, transferring ownership without a corresponding release in this path. Use scoped Retained bindings and Retained::as_ptr for pointer handoff to the callback payload. This preserves borrowing semantics for callback lifetime and keeps Objective-C retain/release balanced when the retained values drop after callback return. Fixes tauri-apps#15210 * chore: add changes file for tauri-apps#15210 * changes: fix covector metadata for objc leak patch Format .changes/fix-objc-retain-leak.md with valid covector front matter and package bump metadata. This resolves CI failures in check-change-tags and covector status for this PR. * chore(tauri-runtime-wry): document Apple Webview pointer fields Add a doc/TODO note on the Apple with_webview pointer fields (webview/manager/ns_window/view_controller): they're borrowed from ObjC Retained handles and must not be mutated through, and should become *const c_void in v3. * refactor(tauri-runtime-wry): simplify pointer cast in with_webview Apply Legend-Master's suggestion: use .cast_mut() instead of casting through *const c_void first, across all macOS/iOS with_webview fields.
fix(tauri-runtime-wry): stop leaking ObjC retains in with_webview
Problem
WebviewMessage::WithWebviewusedRetained::into_rawon Apple targets, transferring ownership to raw pointers in this path without reclaiming it.Fix
Retained::into_rawwith scoped retained bindings +Retained::as_ptrin the macOS/iOSWithWebviewbranches..changes/fix-objc-retain-leak.mdwith valid covector front matter ("tauri-runtime-wry": patch:bug).Testing
cargo fmt --check cargo clippy -p tauri-runtime-wry --all-features -- -D warnings cargo check -p tauri-runtime-wry --all-features cargo test -p tauri-runtime-wry --lib cargo clippy -p tauri-runtime-wry --target aarch64-apple-darwin --all-features -- -D warningsManual stress run (
with_webviewloop, 3 min) shows stable RSS without unbounded growth: 116.5 MB -> 116.9 MB (+0.4 MB).Visual Verification
App running (active loop)
RSS monitor (terminal)
Fixes #15210