Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix crash when setting datastore and add methods to fetch all datastores and to remove a datastore #1512

Merged
merged 7 commits into from
Mar 8, 2025

Conversation

Simon-Laux
Copy link
Contributor

@Simon-Laux Simon-Laux commented Mar 4, 2025

Progress

@Simon-Laux Simon-Laux force-pushed the datastore-improvements branch from 896f3b6 to a465b95 Compare March 4, 2025 22:50
@Simon-Laux Simon-Laux force-pushed the datastore-improvements branch from a465b95 to 4c09f2c Compare March 4, 2025 22:54
Copy link
Contributor

github-actions bot commented Mar 4, 2025

Package Changes Through 64d4869

There are 1 changes which include wry with patch

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
wry 0.50.3 0.50.4

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

@Simon-Laux Simon-Laux force-pushed the datastore-improvements branch from 020b3a7 to 352fa40 Compare March 7, 2025 02:56
@Simon-Laux Simon-Laux marked this pull request as ready for review March 7, 2025 02:56
@Simon-Laux Simon-Laux requested a review from a team as a code owner March 7, 2025 02:56
lucasfernog
lucasfernog previously approved these changes Mar 7, 2025
Copy link
Member

@lucasfernog lucasfernog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved your method to the WebView struct (via ext trait) and made it synchronously (so it's easier to use in tauri)
great work as usual!

@lucasfernog
Copy link
Member

lucasfernog commented Mar 7, 2025

oh I noticed remove_data_store is giving me a segfault.
the error is probably because of this:

Release any [WKWebView](https://developer.apple.com/documentation/webkit/wkwebview) instances using the data store before you call this method. If the system cannot complete removal of the data store, this throws an error.

but it shouldn't segfault, I tried to map the error manually but it cannot go past err.code()

EDIT: even if i close the webview first, I get the error. So the data store is probably still around

// Copyright 2020-2023 Tauri Programme within The Commons Conservancy
// SPDX-License-Identifier: Apache-2.0
// SPDX-License-Identifier: MIT

use objc2_foundation::NSUUID;
use tao::{
  event::{Event, WindowEvent},
  event_loop::{ControlFlow, EventLoop},
  window::WindowBuilder,
};
use wry::{WebView, WebViewBuilder, WebViewBuilderExtDarwin, WebViewExtDarwin};

fn main() -> wry::Result<()> {
  let event_loop = EventLoop::new();
  let window = WindowBuilder::new().build(&event_loop).unwrap();

  let bytes = NSUUID::new().as_bytes();
  let builder = WebViewBuilder::new()
    .with_url("http://tauri.app")
    .with_data_store_identifier(bytes);

  let _webview = builder.build(&window)?;

  let mut w = Some(_webview);
  let mut ww = Some(window);

  let p = event_loop.create_proxy();

  event_loop.run(move |event, _, control_flow| {
    *control_flow = ControlFlow::Wait;

    if let Event::UserEvent(_) = event {
      WebView::remove_data_store(&bytes).unwrap();
      println!("{:?}", WebView::fetch_data_store_identifiers());
    }

    if let Event::WindowEvent {
      event: WindowEvent::CloseRequested,
      ..
    } = event
    {
      w.take();
      ww.take();

      let p = p.clone();
      std::thread::spawn(move || {
        std::thread::sleep_ms(3000);
        p.send_event(());
      });
      //*control_flow = ControlFlow::Exit;
    }
  });
}

@lucasfernog lucasfernog force-pushed the datastore-improvements branch from 7e8087a to 2f537b4 Compare March 7, 2025 13:20
Copy link
Member

@lucasfernog lucasfernog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotta fix the remove function :(

@Simon-Laux
Copy link
Contributor Author

Simon-Laux commented Mar 7, 2025

I believe all IO functions that potentially need to wait for disk should be async and I already solved the tauri part, so I'm not sure why we should change it to be blocking.
Especially because this runs in the main thread, it should not be blocking IMO. Blocking main thread means that the UI of the app freezes, which is super bad UX, so we should avoid it where possible.

@Simon-Laux
Copy link
Contributor Author

Simon-Laux commented Mar 7, 2025

EDIT: even if i close the webview first, I get the error. So the data store is probably still around

correct there is segfault if the web view is still active, thanks for finding that. But it works for me when the web view is closed, I just tried it in my example https://github.com/Simon-Laux/tauri-experiments/tree/mac_data_store_experiments3/examples/data_store_tauri.

await window.api.new_window()
// then delete it, this works if you closed the new window, but if it is still open this segfaults
let i = await window.api.get_ids()
await window.api.delete_id(i[0])

@lucasfernog
Copy link
Member

I'll merge this because it seems unrelated to this PR, but something about the webview drop is not releasing the data store properly.

@lucasfernog lucasfernog merged commit 349dfe3 into tauri-apps:dev Mar 8, 2025
13 checks passed
@Simon-Laux
Copy link
Contributor Author

but something about the webview drop is not releasing the data store properly.

are you sure? because my example worked where I removed the data store of different web views, maybe you call it too early in your testing code?

nevertheless the error should return an error and not crash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants