Skip to content

Commit

Permalink
fix(macOS): race conditions around WKURLSchemeTask
Browse files Browse the repository at this point in the history
Fixes two issues which could cause race conditions when navigating between pages while custom protocol requests are in progress on macOS and iOS:
- The `obc2/catch-all` feature turns all Objective-C exceptions into panics, preventing `obc2::exception::catch(...)` from working.
- The `webview` and `task` objects passed to `start_task` were not retained, leading to use-after-free issues in the `responder`.

Closes #1189
  • Loading branch information
alexmoon committed Feb 10, 2025
1 parent cf18194 commit 2139170
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 12 deletions.
5 changes: 5 additions & 0 deletions .changes/objc-exceptions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
wry: major
---

Removed `obj-exception` feature.
3 changes: 1 addition & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,8 @@ rustc-args = ["--cfg", "docsrs"]
rustdoc-args = ["--cfg", "docsrs"]

[features]
default = ["drag-drop", "objc-exception", "protocol", "os-webview"]
default = ["drag-drop", "protocol", "os-webview"]
serde = ["dpi/serde"]
objc-exception = ["objc2/catch-all"]
drag-drop = []
protocol = []
devtools = []
Expand Down
22 changes: 12 additions & 10 deletions src/wkwebview/class/url_scheme_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use http::{
use objc2::{
rc::Retained,
runtime::{AnyClass, AnyObject, ClassBuilder, ProtocolObject},
AllocAnyThread, ClassType,
AllocAnyThread, ClassType, Message,
};
use objc2_foundation::{
NSData, NSHTTPURLResponse, NSMutableDictionary, NSObject, NSObjectProtocol, NSString, NSURL,
Expand Down Expand Up @@ -54,8 +54,8 @@ pub fn create(name: &str) -> &AnyClass {
extern "C" fn start_task(
this: &AnyObject,
_sel: objc2::runtime::Sel,
webview: &'static WryWebView,
task: &'static ProtocolObject<dyn WKURLSchemeTask>,
webview: &WryWebView,
task: &ProtocolObject<dyn WKURLSchemeTask>,
) {
unsafe {
#[cfg(feature = "tracing")]
Expand Down Expand Up @@ -180,12 +180,14 @@ extern "C" fn start_task(
// send response
match http_request.body(sent_form_body) {
Ok(final_request) => {
let webview = webview.retain();
let task = task.retain();
let responder: Box<dyn FnOnce(HttpResponse<Cow<'static, [u8]>>)> =
Box::new(move |sent_response| {
// Consolidate checks before calling into `did*` methods.
let validate = || -> crate::Result<()> {
check_webview_id_valid(webview_id)?;
check_task_is_valid(webview, task_key, task_uuid.clone())?;
check_task_is_valid(&webview, task_key, task_uuid.clone())?;
Ok(())
};

Expand All @@ -198,9 +200,9 @@ extern "C" fn start_task(

unsafe fn response(
// FIXME: though we give it a static lifetime, it's not guaranteed to be valid.
task: &'static ProtocolObject<dyn WKURLSchemeTask>,
task: Retained<ProtocolObject<dyn WKURLSchemeTask>>,
// FIXME: though we give it a static lifetime, it's not guaranteed to be valid.
webview: &'static WryWebView,
webview: Retained<WryWebView>,
task_key: usize,
task_uuid: Retained<NSUUID>,
webview_id: &str,
Expand All @@ -209,7 +211,7 @@ extern "C" fn start_task(
) -> crate::Result<()> {
// Validate
check_webview_id_valid(webview_id)?;
check_task_is_valid(webview, task_key, task_uuid.clone())?;
check_task_is_valid(&webview, task_key, task_uuid.clone())?;

let content = sent_response.body();
// default: application/octet-stream, but should be provided by the client
Expand Down Expand Up @@ -253,7 +255,7 @@ extern "C" fn start_task(

// Re-validate before calling didReceiveResponse
check_webview_id_valid(webview_id)?;
check_task_is_valid(webview, task_key, task_uuid.clone())?;
check_task_is_valid(&webview, task_key, task_uuid.clone())?;

// Use map_err to convert Option<Retained<Exception>> to crate::Error
objc2::exception::catch(AssertUnwindSafe(|| {
Expand All @@ -273,15 +275,15 @@ extern "C" fn start_task(

// Check validity again
check_webview_id_valid(webview_id)?;
check_task_is_valid(webview, task_key, task_uuid.clone())?;
check_task_is_valid(&webview, task_key, task_uuid.clone())?;

objc2::exception::catch(AssertUnwindSafe(|| {
task.didReceiveData(&data);
}))
.map_err(|_e| crate::Error::CustomProtocolTaskInvalid)?;

check_webview_id_valid(webview_id)?;
check_task_is_valid(webview, task_key, task_uuid.clone())?;
check_task_is_valid(&webview, task_key, task_uuid.clone())?;

objc2::exception::catch(AssertUnwindSafe(|| {
task.didFinish();
Expand Down

0 comments on commit 2139170

Please sign in to comment.