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: log postmessage errors #1426

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

fix: log postmessage errors #1426

wants to merge 2 commits into from

Conversation

FabianLars
Copy link
Member

ref tauri-apps/tauri#10546

i still don't know what's happening tbh, i don't think we can realistically fill the queue. For the appWindow.close() panic i think this is just a timing issue. Anyway, panicking like this is toxic af and now we also print the error code which hopefully tells us a bit about the actual issue.

@FabianLars FabianLars requested a review from a team as a code owner November 23, 2024 15:44
Copy link
Contributor

github-actions bot commented Nov 23, 2024

Package Changes Through bb5acc2

There are 1 changes which include wry with minor

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.47.2 0.48.0

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


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

@wonfen
Copy link

wonfen commented Nov 25, 2024

Please merge is PR, We eagerly wait to test and see if the problem is fixed.

@FabianLars
Copy link
Member Author

FabianLars commented Nov 25, 2024

add this to cargo.toml

[patch.crates-io]
wry = { git = "https://github.com/tauri-apps/wry", branch = "fix/postmessage-assert" }

then run cargo update in the src-tauri dir. It worked if it does not print a warning with something like "Patch was not used in crate graph". Then you can test this PR.

I think i may be alone this week and i can't merge my own PRs in this repo (nor do i want to because the tracing/debug_assertions impl i did is a bit weird)

src/webview2/mod.rs Outdated Show resolved Hide resolved
@Legend-Master
Copy link
Contributor

From this example provided from tauri-apps/tauri#10546 (comment), the error seems to be invalid window handle

@FabianLars
Copy link
Member Author

as i understood it for some the error shows up without closing windows/webviews. there's so much confusion going around in that issue though so i may have misunderstood something 🤷

but like i said, this pr is not trying to fix the actual issue just to get rid of the panic (which we should get rid of either way).
that said, if everyone is just seeing the window closing issue then this kinda does fix the issue (not much difference in validating the hwnd before sending vs trying to send the msg and ignore the result 🤷 )

@Legend-Master
Copy link
Contributor

Yeah, I agree we should get rid of this panic and log the error instead, just trying to understand why this part even gets called after both window and webview get destroyed

@FabianLars
Copy link
Member Author

FabianLars commented Nov 26, 2024

i really didn't look into this at all, but we've had a similar-ish issue on macOS where we used the webview and/or tasks in the custom protocol responses even after closing the window/webview (cause we can't cancel the rust handler function). in that impl we couldn't just ignore the error so had to add a bunch of validity checks instead.

i think the main trigger in tauri was the closerequested or destroyed events? idk, we did send something when closing the window

@Legend-Master
Copy link
Contributor

Oh I get it now, our IPC uses custom protocol to handle message from the webview, so basically it's that we received the command through the custom protocol handler, and we handle the command, while handling it, the webview gets destroyed, and when we finished and want to send the result back, this error happens

I think we should put something to indicate if the webview gets destroyed and when it happens, don't run the handler

@FabianLars
Copy link
Member Author

if the behavior is similar to what we had to deal with on macos then "don't run the handler" is not good enough because of timing issues but feel free to look into it, maybe it's better here.

@Legend-Master
Copy link
Contributor

Legend-Master commented Nov 26, 2024

This is kinda tricky, I'm not sure if we should call deferral.Complete here or not when the webview is destroyed and also does deferral.Complete even work when it's not on the main thread

@Legend-Master
Copy link
Contributor

I think we just call deferral.Complete() here, shouldn't matter if we're not on main thread or not when the webview is already gone, just need to call it to drop the memory, at least I can't observe any side effects doing so

https://learn.microsoft.com/en-us/uwp/api/windows.foundation.deferral.complete?view=winrt-26100#remarks

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

Successfully merging this pull request may close these issues.

3 participants