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

Memory leak on windows #137

Closed
pomplesiegel opened this issue Oct 11, 2017 · 26 comments
Closed

Memory leak on windows #137

pomplesiegel opened this issue Oct 11, 2017 · 26 comments

Comments

@pomplesiegel
Copy link

pomplesiegel commented Oct 11, 2017

There appears to be a memory leak on Windows, occurring whenever webview.evaluate_js() is called. I'm using pywebview v1.7 + "pythonnet" on Windows 10 64bit, with python3.6.2. The same issue does not exist on mac.

Code to reproduce (most trivial example I could come up with)

import webview
import threading
import time

threadStopSignal = threading.Event()
threadStopSignal.clear()

def checkOnDOM():
	global threadStopSignal 
	while threadStopSignal.is_set() == False:
		time.sleep(.01)
		webview.evaluate_js("doNothing()" )
		
def main():
	print("Beginning program")
	t = threading.Thread(target=checkOnDOM)
	t.start()

	webview.create_window("This is a window", "index.html")
	threadStopSignal.set()
	t.join(3.0)
main()
<!DOCTYPE html>
<html>
<script type="text/javascript">
	function doNothing() { }
</script>
<head>
</head>
<body>
<div>
</div>
</body>
</html>

Then watch the memory usage of the program grow in the task manager. The memory usage on Mac remains static when running this program.

In order to further investigate, I later changed doNothing to

function doNothing() {
     return document.getElementsByTagName("script").length;
}

which, when printed in python, made it clear that the number of script elements is growing (and never shrinking) with each call to evaluate_js(). Not sure if that's the expected behavior.

Finally, if the value of those script tags is printed, we see something like
function invokecb2c8066aea211e7849010f0050b27cc() {return eval("doNothing()")}
which over time monotonically grows to higher values

function invokef9d07746aea211e780ee10f0050b27cc() {return eval("doNothing()")}
function invokef9d29312aea211e791b410f0050b27cc() {return eval("doNothing()")}
function invokef9d46ef6aea211e78f1f10f0050b27cc() {return eval("doNothing()")}

up and up.... Are these pointers to resources in windows that never get cleaned up, or are they just GUIDs?

Happy to provide any more info.

Really enjoying the library, btw! Hoping to resolve the memory issue on the windows side so I can use it further.

@den-run-ai
Copy link
Contributor

We noticed a memory leak in pythonnet and it is fixed in pull request for most use cases:

pythonnet/pythonnet#541

@shivaprsd
Copy link
Collaborator

shivaprsd commented Oct 11, 2017

@pomplesiegel This is interesting. 🤔 Everything you have reported on
evaluate_js() is expected behaviour (except for the memory leak, of course).
The script elements and UUIDs are part of the implementation. Those are just
plain UUIDs used to name the function that evaluates the JS, nothing to do
with pointers.

Your program is doing an infinite loop here. I reckon you are doing this on
purpose? Because, threadStopSignal.set() will never get called as the call
to create_window blocks the main thread. BTW, have you checked whether
this happens with any other API methods?

If this happens to evaluate_js only, then I think we have indeed hit a bug
here. Not deleting the script elements after use could be the cause. Roman
wrote the code for WinForms, may be he'll look into this, pronto.

@shivaprsd shivaprsd added the bug label Oct 11, 2017
@pomplesiegel
Copy link
Author

pomplesiegel commented Oct 11, 2017

@shivaprsdv Great, yes I believe this is a bug as well. Thank you.

(removed my original end of this post)

@r0x0r
Copy link
Owner

r0x0r commented Oct 12, 2017

You do not need to call destroy_window to close the window by pressing the X button. This case is already handled by webview. As Shiva said Invoke eval stuff is implemented on purpose to get a return value from the evaluated code.

@pomplesiegel
Copy link
Author

Hi @r0x0r, I apologize for getting off topic. I removed the second half of my post.
This issue is focused on the memory leak in windows caused by repeatedly calling evaluate_js(). With the program above, the memory usage on my Windows machine will grow by multiple MB per minute. Do you see this behavior on your machine as well?

@shivaprsd
Copy link
Collaborator

The leak should be real. Although it doesn't happen on Mac as the OP pointed
out, I tried simulating what must be happening; it ate all my RAM, and in the Web
Inspector I found a heap growth:

Normal on Mac:

normal

Simulated:

simulated

The issue is simple: every time we call evaluate_js(), we add a script element
to the doc, and a function to the JS context. The functions would not be garbage
collected, so when we call it a thousand times it blows up the memory.

Leak or not, ideally we should be deleting the script element after it accomplishes
its purpose anyway; it's not good to permanently modify the user's page like this.

@r0x0r
Copy link
Owner

r0x0r commented Oct 12, 2017

I see the problem now, but this is how the Windows implementation works. The problem is that Windows does not provide a way to run arbitrary Javascript, but instead is able to run a named function. So in order to run Javascript code, it must be wrapped into a named function, which will be called (wrapped inside an eval to get its return value). Given all that, the function will stay in memory.
I guess we could fit a del after the function call. I will investigate if it is a feasible way and how it will play with async code.

@pomplesiegel
Copy link
Author

Great, thank you!

@shivaprsd
Copy link
Collaborator

@r0x0r Could add the code to delete the function along with the script element
once _js_result_semaphor() releases.

@r0x0r
Copy link
Owner

r0x0r commented Oct 12, 2017

Gave it a go and found out the following

  1. delete works only on object properties, not top level objects or functions
  2. You cannot delete a <script> element using removeNode or removeChild

The next step will be encapsulating an invoke function inside an object and see if WinForms is able to call that. It is late here though, I will give it a go at a more appropriate time.

@r0x0r
Copy link
Owner

r0x0r commented Oct 12, 2017

Uhm the object property approach is no good either. Any other ideas?
capture

@shivaprsd
Copy link
Collaborator

shivaprsd commented Oct 13, 2017

This is what I found out:

  1. JavaScript is garbage collected, so no need to use delete; set the function to
    null instead.
  2. Set the OuterHTML property of the <script> to empty string.

I would try:

def _evaluate_js():
    ...
    code = 'function {0}() {{ {0} = null; return eval("{1}"); }}'  # 1.
    code = code.format(function_name, _escape_string(script))
    ...
    self._js_result = document.InvokeScript(function_name)
    self._js_result_semaphor.release()
    script_element.OuterHTML = ''  # 2.
    ...

@shivaprsd
Copy link
Collaborator

shivaprsd commented Oct 18, 2017

@pomplesiegel Can you check whether this solves the problem? I have no way to test it.

@pomplesiegel
Copy link
Author

Hi @shivaprsdv, thanks for the update! I just checked out and ran this branch on windows and unfortunately the memory leak still persists on Windows. I'm happy to try anything further.

@shivaprsd
Copy link
Collaborator

shivaprsd commented Oct 18, 2017

I must have overlooked something. Seems like the functions aren't being garbage
collected, even after being set to null. Now, have you tried printing the script tags
this time, like you did earlier? Can you check whether it still grows?

@pomplesiegel
Copy link
Author

pomplesiegel commented Oct 18, 2017

Sure @shivaprsdv, i just ran it w/ the program printing a return value of

return document.getElementsByTagName("script").length;

and this value is growing rapidly.

If I print the last value of the script array as it grows, i get something like

function invokee3399462b44a11e7a86110f0050b27cc() { invokee3399462b44a11e7a86110f0050b27cc = null; return eval("doNothing()"); }
function invokee33b6676b44a11e78de910f0050b27cc() { invokee33b6676b44a11e78de910f0050b27cc = null; return eval("doNothing()"); }
function invokee33d7938b44a11e7af7010f0050b27cc() { invokee33d7938b44a11e7af7010f0050b27cc = null; return eval("doNothing()"); }
function invokee33f8988b44a11e7805810f0050b27cc() { invokee33f8988b44a11e7805810f0050b27cc = null; return eval("doNothing()"); }
function invokee341662eb44a11e7852210f0050b27cc() { invokee341662eb44a11e7852210f0050b27cc = null; return eval("doNothing()"); }
function invokee34348dab44a11e7a62610f0050b27cc() { invokee34348dab44a11e7a62610f0050b27cc = null; return eval("doNothing()"); }
function invokee3456474b44a11e79fde10f0050b27cc() { invokee3456474b44a11e79fde10f0050b27cc = null; return eval("doNothing()"); }
function invokee34773e6b44a11e79c7210f0050b27cc() { invokee34773e6b44a11e79c7210f0050b27cc = null; return eval("doNothing()"); }

@shivaprsd
Copy link
Collaborator

@r0x0r Do we need all this? The guy at this page says:

dsdf

I remember the talk we had on this on the original evaluate-js issue; nevertheless,
had you tried this before deciding to settle on the invoke3399... idea?

@r0x0r
Copy link
Owner

r0x0r commented Oct 19, 2017

Good find. I haven't thought of using eval directly, I will give it a go.

@r0x0r
Copy link
Owner

r0x0r commented Oct 19, 2017

I pushed a revised version to the master. For some reason it refuses to work with the escaped script, but the original script works like a charm (with line breaks and line comments)
@pomplesiegel Could you verify that it does not leak memory?

@pomplesiegel
Copy link
Author

Hi @r0x0r, thanks for the update!

This is an improvement, as the size of the scripts is no longer increasing. However I do still see a fairly fast leak with my empty program from above: About 1MB every 20s.

Do you see this on your side as well using Task Manager?

@shivaprsd
Copy link
Collaborator

but the original script works like a charm

@r0x0r Have you checked there is no need to escape even double quotes? 😃

I do still see a fairly fast leak with my empty program from above: About 1MB
every 20s.

@pomplesiegel May be this is normal, seeing you're running an intensive infinite
loop. To pin point the issue, can you call any other API function (say load_html)
instead of evaluate_js, and compare the memory usage?

@pomplesiegel
Copy link
Author

Hi @shivaprsdv, i just checked this. The program does not grow when calling load_html(""). Also, on the Mac side the program does not grow when calling evaluate_js(), so this issue appears to be linked to just evaluate_js() and just on Windows.

@shivaprsd
Copy link
Collaborator

Is there any way to check the behaviour of this program (or something similar) on
Internet Explorer? It could well be an MSHTML issue, for all I can say.. 🤔

@r0x0r
Copy link
Owner

r0x0r commented Oct 20, 2017

Here is what I found out.

  1. As it was pointed above, your original program has got an infinite loop. Remember that create_window blocks execution, until the window is destroyed
  2. I have modified the script to invoke doNothing repeatedly without invoking evaluate_js and memory grows in the same fashion as with evaluate_js. Furthermore IE does the same.
<!DOCTYPE html>
<html>
<script type="text/javascript">
	function doNothing() { }

	for(var i =0; i < 100000; i++) {
	    doNothing();
    }
</script>
<head>
</head>
<body>
<div>
    <button onclick="doNothing()">Test</button>
</div>
</body>
</html>
  1. All in all, it seems this is how IE garbage collection works and there is nothing we can do about that. A good thing that we got rid script elements and invoke functions, though!

@pomplesiegel
Copy link
Author

Great, thanks for looking into this. I'm happy with this as a result - was just trying to be thorough. Shall we consider this closed?

@r0x0r
Copy link
Owner

r0x0r commented Oct 20, 2017 via email

@shivaprsd shivaprsd removed the bug label Oct 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants