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: js bindings must be kept by browser, not by process #536

Merged
merged 1 commit into from
Oct 30, 2014

Conversation

sylvain-hamel
Copy link
Contributor

When renderer-process-limit is set to a low value (1) and multiple browser instances are used, all browser instances end up using the same JS binding object (the last one registered).

The root cause is that the binding is kept per process (CefAppUnmanagedWrapper) instead of per browser instance (CefBrowserWrapper).

The fix is to move the bindings from CefAppUnmanagedWrapper to CefBrowserWrapper class. I deleted the Bind method because it was no longer used.


According to this comment the number of processes is a calculated value.

On our PCs we always get one process per browser instance if we use the defaut value for renderer-process-limit. Each process consumes at least 20 MB. Because our app may open up to 20 browser instances, we've decided to set renderer-process-limit to 5 to limit the memory consumed by our app.

We think this bug may occur even if you don't change renderer-process-limit because at some point, the number of browser instances can probably exceed the calculated limit to the number of renderer processes.

@amaitland
Copy link
Member

@sylvain-hamel Welcome to CefSharp 😄

I've had a quick look at your PR and it looks fairly solid 👍 Are you able to fix up the formatting? You'll see under the Files Changed tab that things are out. It's just a simple matter of changing the tabs to spaces.

Contributing.Md has more information should you require it.
https://github.com/cefsharp/CefSharp/blob/master/CONTRIBUTING.md

@amaitland amaitland added this to the 33.1.0-pre milestone Oct 29, 2014
@amaitland amaitland added the cef3 label Oct 29, 2014
@amaitland
Copy link
Member

I believe this PR will address the issue raised in #477

@amaitland
Copy link
Member

@jornh Looking at the AppVeyor build for this PR and it appears that it's trying to run the test cases. Is there some setting we can tweak to disable for now?

@sylvain-hamel
Copy link
Contributor Author

@amaitland I had accidentally inserted tabs instead of spaces. It's fixed now. However, CefAppUnmanagedWrapper.h was using tabs whereas all other files seem to use spaces so I fixed this one too.

@amaitland
Copy link
Member

@sylvain-hamel Thanks, makes the review much easier as it filters out all the changes which are just whitespace.

CefAppUnmanagedWrapper.h was using tabs whereas all other files seem to use spaces so I fixed this one too.

Thanks, That one would be my mistake 😄

@amaitland amaitland merged commit 29e5e61 into cefsharp:JsBinding_WIP Oct 30, 2014
@amaitland
Copy link
Member

@sylvain-hamel Thanks for the contribution, code has been merged and will be included in the next -pre release.

@sylvain-hamel
Copy link
Contributor Author

@amaitland Great news! When will this branch be merged into master?

@amaitland
Copy link
Member

See #509 (comment) for our discussion on that topic.

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

Successfully merging this pull request may close these issues.

2 participants