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

feat(linux): add headers to URL scheme request #721

Merged
merged 8 commits into from
Oct 17, 2022
Merged

feat(linux): add headers to URL scheme request #721

merged 8 commits into from
Oct 17, 2022

Conversation

wusyong
Copy link
Member

@wusyong wusyong commented Oct 14, 2022

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@wusyong wusyong requested a review from a team as a code owner October 14, 2022 08:40
@wusyong
Copy link
Member Author

wusyong commented Oct 14, 2022

There's a race condition could cause page freeze right now.
Run custom_protocol example and hit the button quickly to constantly changing pages. You will get stuck at some point.
Sometimes it will even be blank page on start.
It will also stuck even if I just call http_headers and do nothing.

@wusyong
Copy link
Member Author

wusyong commented Oct 15, 2022

I finally found the cause. So it seems the binding from webkit2gtk-rs will take header's ownership:
https://github.com/tauri-apps/webkit2gtk-rs/blob/crate/src/auto/uri_scheme_request.rs#L113
And the type in header field is a unique_ptr!
https://searchfox.org/wubkat/source/Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp#73
This cause a race condition that can result into use after free...

@wusyong
Copy link
Member Author

wusyong commented Oct 16, 2022

@lucasfernog These methods require webkit2gtk 2.36. I think I probably need to add a feature flag for it. How do you think tauri should do? Also a flag to use it?

@lucasfernog
Copy link
Member

@wusyong can we detect the webkit2gtk version at runtime and not call the API if the version is below 2.36? I think that would be the safest approach.

@wusyong
Copy link
Member Author

wusyong commented Oct 16, 2022

Yes we can. I'll try this approach then.

Wu Yu Wei added 3 commits October 17, 2022 11:34
Add a config flag is_2_36 to detect if the webkit2gtk can call
advanced methods. Now we can add http headers and method in
request, status code in response.
remove 2d canvas acceleration config since it's deprecated and
it doesn't really accelerate either.
@lucasfernog lucasfernog merged commit 2944d91 into dev Oct 17, 2022
@lucasfernog lucasfernog deleted the hdr branch October 17, 2022 11:23
@github-actions github-actions bot mentioned this pull request Oct 17, 2022
@chairmanwow
Copy link

chairmanwow commented Oct 17, 2022

let http_request = match http_request.body(Vec::new()) {

It looks like this doesn't support POST with bodies, say for example JSON in the POST request body. Is there a reason why the body is being set to the empty vector? Does WebKitURISchemeRequest have no way of getting the body?

I tried this in an app sending POST's with JSON and the custom scheme handler can't see those. I'm happy to dig into this and try to get to a pull request if there's interest from the tauri side.

@wusyong
Copy link
Member Author

wusyong commented Oct 18, 2022

@chairmanwow WebKitURISchemeRequest doesn't expose it yet. That's why there's still issue #666 for it. If you are interested in this, I suppose you can open a PR in Webkit directly to add it.

@chairmanwow
Copy link

Yikes, understood. Thanks for the effort and for clarifying the why for me.

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.

4 participants