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: prevent memory leak on macOS, closes #536 #587

Merged
merged 2 commits into from May 20, 2022

Conversation

keiya01
Copy link
Member

@keiya01 keiya01 commented May 15, 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

@keiya01 keiya01 requested a review from a team May 15, 2022 12:27
@lucasfernog
Copy link
Member

I think you meant removing the script message handler for the IPC?

let _: () = msg_send![manager, addScriptMessageHandler:handler name:ipc];

@keiya01
Copy link
Member Author

keiya01 commented May 15, 2022

This PR is based on https://stackoverflow.com/questions/58454042/wkwebview-instances-and-memory-leak.
I'm sorry I don't know detail about this solution but we need to attach webview to scriptMessageHandler. And we can reduce memory usage when I add and remove window(webview).
But maybe this improvement is occurred only on my macOS environment, so please check it on your environment.

@keiya01
Copy link
Member Author

keiya01 commented May 15, 2022

Maybe we can also use removeAllScriptMessageHandlers.

@lucasfernog
Copy link
Member

Sounds like exactly what we need :)

@keiya01 keiya01 force-pushed the fix/multi-window-memory-leak-macos branch from 2b2eb6d to 632d543 Compare May 16, 2022 00:30
@keiya01 keiya01 force-pushed the fix/multi-window-memory-leak-macos branch from 632d543 to d5cdd30 Compare May 19, 2022 00:32
Copy link
Member Author

@keiya01 keiya01 left a comment

Choose a reason for hiding this comment

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

I changed all of commit because I found new solution 🙇

#[cfg(target_os = "macos")]
let _: Id<_> = Id::from_ptr(self.ns_window);
let _: Id<_> = Id::from_ptr(self.manager);
let _: Id<_> = Id::from_retained_ptr(self.webview);
Copy link
Member Author

Choose a reason for hiding this comment

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

I found the Id::from_ptr is not working well, so id object is not dropped.
I fixed these code to use Id::from_retained_ptr with reference to rust-objc-id example.
If we use Id::from_retained_ptr, navigation to about:blank is not needed :)

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Yep, tested it and it indeed cleans up the process we were talking about correctly! nice 🥳

What it still doesn''t cleanup is the Networking process, but since it does the same on Linux it looks like a webkit problem 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I think network connection is closed but network memory is still remained. I think this is because networking process is single process, networking memory is shared between multiple webview 👀

Copy link
Member Author

@keiya01 keiya01 May 19, 2022

Choose a reason for hiding this comment

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

This process is responsible for handling network requests as well as storage management. All WebContent processes in a single session (default vs. private browsing) share a single networking session in the networking process.
https://github.com/WebKit/WebKit/blob/main/Introduction.md

This is webkit description.
I think we can remove cache but all of webview cache will be removed. It will bring to bad page loading performance.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try to use this gist to remove cache in another PR.

wusyong
wusyong previously approved these changes May 20, 2022
@keiya01 keiya01 requested a review from a team as a code owner May 20, 2022 05:04
Copy link
Member

@wusyong wusyong left a comment

Choose a reason for hiding this comment

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

@keiya01 LGTM. Do you want to merge this now?
I suppose networking process will be another PR right? Could you choose an issue and make it the main tracking issue?
Also you can just add a branch in this repo next time.

@keiya01
Copy link
Member Author

keiya01 commented May 20, 2022

Yes, I want to merge this PR now.
I will try to resolve #590 in another PR.

@wusyong wusyong merged commit 16d1924 into tauri-apps:dev May 20, 2022
@github-actions github-actions bot mentioned this pull request May 20, 2022
@keiya01 keiya01 deleted the fix/multi-window-memory-leak-macos branch May 20, 2022 14:04
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.

None yet

4 participants