feat: auto-reload Apple webviews on content process termination#15162
feat: auto-reload Apple webviews on content process termination#15162polw1 wants to merge 3 commits intotauri-apps:devfrom
Conversation
8d9c907 to
bd371cd
Compare
Package Changes Through aa8fc7eThere are 9 changes which include tauri-macos-sign with patch, tauri-build with patch, tauri with minor, tauri-bundler with minor, tauri-cli with minor, @tauri-apps/cli with minor, tauri-runtime with minor, tauri-runtime-wry with minor, tauri-utils 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 |
bd371cd to
26c539e
Compare
|
Thanks for the PR! I didn't take a proper look yet but i think we should expose this on tauri::Builder as well (or only there?) so that windows created in JS or tauri.conf can be handled as well. |
26c539e to
aa8fc7e
Compare
|
Thank you, @FabianLars Good point about webviews created through JS or tauri.conf. I do not see a strong reason for this behavior to differ per webview, so |
|
Following @FabianLars' feedback — here's a proposal for how to plumb the handler globally. Priority chain:
Touch points: // 1. Builder<R> — new field + setter (crates/tauri/src/app.rs)
#[cfg(any(target_os = "macos", target_os = "ios"))]
on_web_content_process_terminate: Option<Arc<OnWebContentProcessTerminateHandler<R>>>,
// 2. WebviewManager — store the global handler (crates/tauri/src/manager/webview.rs)
// Passed through Builder::build() → AppManager::with_handlers() → WebviewManager
#[cfg(any(target_os = "macos", target_os = "ios"))]
pub on_web_content_process_terminate: Option<Arc<OnWebContentProcessTerminateHandler<R>>>,
// 3. Webview creation — resolve the priority chain (crates/tauri/src/webview/mod.rs)
// In into_pending_webview():
let handler = per_webview_handler
.or_else(|| global_handler_from_webview_manager.clone());
pending.handler = match handler {
Some(h) => wrap_custom_handler(manager, label, h),
None => build_default_reload_handler(manager, label),
};
// 4. Simplify map_web_content_process_terminate_handler — remove the
// install_default_reload_handler bool param (always true today)This follows the same pattern as What are your thoughts @FabianLars? |
|
I agree with your proposal. |
|
So I dug deeper and found out that @velocitysystems and @polw1 are collaborators in repos under the organization @silvermine. The weird part is that @velocitysystems started a review of the original PR (#14523) earlier this month, then went cold until this PR (with most of the code of the original PR copied over) popped up a few days ago. This is surely not in the spirit of open source, but if this is how you want to manage contributions to Tauri, I'm sure you're free to do so. |
|
This is totally my bad, i didn't want to give the impression that this would simply discard your PR. For a bit more transparency, this is their internally used/tested version of your PR and they asked me whether it made sense to show it quickly like this instead of keep adding comments on your PR which sounded reasonable to me, way quicker to take a quick look than discussing it for days. I wouldn't mind pulling in the changes in this PR in yours first and then merge yours in the main branch (or attribute you as a Co-authored-by here). Completely discarding #15162 would be a bad move too considering it does add value in the retry logic and the unit tests. Also, with the comments above, both PRs are missing the tauri::Builder hook. |
|
@JeffTsang Our apologies for any confusion. To be fully transparent: this issue is a priority for us to resolve, and after reviewing your PR I had concerns about the That said, your PR is the foundation — you identified the root cause, got the wry dependency landed, and wired up the handler. Per @FabianLars's suggestion, I propose we combine efforts: pull the rate-limiting logic, tests, and get the @polw1 has been working on the builder pattern and will push these changes shortly to this branch. |
|
Clearly, the proper way forward would be to finish the original PR (#14523) where I already reverted to reloading as requested. But you can do as you wish. In writing the original PR, I followed the conventions of the existing code. If there were unit tests for similar functions, I would have added some myself. If the tests were really necessary, it would have been trivially easy to add them. The tauri::Builder hook would have also been trivially easy to add, but it was never requested in the original PR. With regards to this PR, it assumes that multiple reloads with rate limiting works where a single reload fails. Have you had this experience with an app in production that was tested over an extended period of time? I have macOS and iOS apps that have been in production for almost a year now, and I solved the blank webview problem by loading a url instead of reloading. |
|
If you haven't actually experienced an endless loop of web content process terminations, you're adding a lot of needless complexity that won't accomplish anything. Also, the logging isn't particularly useful. In fact, none of the other functions in In the original PR (#14523), I placed the default handler in tauri-runtime-wry. This is the most appropriate place for the default handler because it is the final place where the handler is actually added to the webview. |
|
Just a heads up that I've moved the function to tauri::Builder in the original PR (#14523). If there's anything else that needs to be done on the original PR, let me know. |
aa8fc7e to
b959470
Compare
|
Thanks for the detailed feedback, @JeffTsang . Your original PR was important in identifying and moving this issue forward. Talking a bit about the design decisions made here: Rate limiting and complexity Why this policy lives in the Tauri layer Logging Tests I’m happy to keep iterating if needed. It’s completely fine if we disagree on some of these points — healthy discussion helps us arrive at better solutions. |
|
The discussion ended up being continued in the original PR (#14523 (comment)). When I was talking about testing, I was referring to the lack of tests in the original PR. The tests you wrote for the rate-limited reloads are fine. |
Summary
Fixes #14371
On iOS (and macOS), the WebKit content process can be terminated by the OS (e.g. due to memory pressure or backgrounding). When this happens, the webview goes blank and becomes unresponsive. This PR adds automatic recovery by reloading the webview when the content process terminates.
on_web_content_process_terminateon bothWebviewBuilderandWebviewWindowBuilderfor custom handlingTest plan
cargo fmt -p tauri -- --checkpassescargo clippy -p tauri -- -W warningspassescargo test -p tauri --lib webview::tests)