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

Javascript Binding - Store JavascriptObjects/Settings per CefBrowser #2306

Open
amaitland opened this issue Mar 8, 2018 · 18 comments · May be fixed by #4475
Open

Javascript Binding - Store JavascriptObjects/Settings per CefBrowser #2306

amaitland opened this issue Mar 8, 2018 · 18 comments · May be fixed by #4475

Comments

@amaitland
Copy link
Member

amaitland commented Mar 8, 2018

Currently gcroot<Dictionary<String^, JavascriptObject^>^> _javascriptObjects; is stored per sub process, when multiple browsers share the same sub process this limits the flexibility somewhat.

Look at storing the objects at a per browser level.

@amaitland
Copy link
Member Author

https://github.com/cefsharp/CefSharp/blob/master/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h#L33

This change would maybe allow for limiting the number of sub processes to 1 (would have to test to be sure).

@amaitland
Copy link
Member Author

Legacy binding will require the objects to be bound at the process level for popups to function.

@amaitland
Copy link
Member Author

Might be worth making this configurable, keeping the current behavior.

@amaitland
Copy link
Member Author

LegacyBinding with multiple ChromiumWebBrowser instances that share the same render process and use the same bound object names require this level of separation, details in #2301

@jsoldi

This comment has been minimized.

@amaitland

This comment has been minimized.

@amaitland

This comment has been minimized.

@Termit1975
Copy link

We're migrating from 37, I'm willing to implement this fix like I did with @sylvain-hamel implementing PR: #536.
I suspect it will be fairly the same work. Any hint or considerations you have in mind that I should know of?

Thanks

@amaitland
Copy link
Member Author

@Termit1975 What exactly is your use case? You can use a separate RequestContext for each browser to force isolation, you can have RequestContext's share the same cache.

The chromium process model has changed dramatically since this was originally opened see https://www.chromium.org/Home/chromium-security/site-isolation

We'd need a fairly extensive set of integration tests to accompany any PR for it to be considered for merging.

@ebelanger
Copy link

Greetings @amaitland,

I am a collegue of @Termit1975 and I'll be taking over. We tried the separate RequestContext approach but this causes the creation of a new render process on each browser instantiation. In our case, we need to limit the number of render processes being used to have smaller memory footprint for our clients.

We did some tests while limiting the number of render processes on the sample application JS binding test page. When set to a limit of 2, the first test page would work but the second would just hang and this seems to be the same behavior we see in our application.

Would you consider the JS binding test page a valid set of integration tests?

@amaitland
Copy link
Member Author

@ebelanger Can you please elaborate on the specifics of your use case. How many browsers are you using, are they different origins

Is there a reason you cannot use the newer binding method? In a lot of cases it's very easy to inject some javascript upon V8Context creation that starts initiates binding and minimal other changes are required.

In our case, we need to limit the number of render processes being used to have smaller memory footprint for our clients.

Assuming you are referring to the renderer-process-limit command line arg. Technically this isn't a supported option, I have seen at least one other report of problems https://magpcss.org/ceforum/viewtopic.php?f=6&t=17346

It's very unlikely any bugs you experience when using this option would be fixed in CEF/Chromium. If you haven't already then I'd suggest doing some further research in the broader Chromium/CEF context to determine if there are any other reported issues.

We did some tests while limiting the number of render processes on the sample application JS binding test page. When set to a limit of 2, the first test page would work but the second would just hang and this seems to be the same behavior we see in our application.

Can you please provide additional details, exact changes to the code and steps to reproduce this.

Did you debug the renderer process and confirm the issue you are seeing is actually related to this particular issue?

Would you consider the JS binding test page a valid set of integration tests?

By itself no. We'll need some automated tests that cover the different scenarios, newer binding, legacy binding, popups, cross origin navigations.

To set a realistic set of expectations here it would likely be months before a PR made it into a Release branch.

@ebelanger
Copy link

ebelanger commented May 29, 2020

@amaitland

Can you please elaborate on the specifics of your use case. How many browsers are you using, are they different origins

The browser instances used in our application are of the same origin except in rare cases; with no external navigation. There's potentially an unlimited number. We could consider them like tabs in Chrome.

Is there a reason you cannot use the newer binding method?

We are now using Javascript Binding v2 instead of the legacy API if this is what you are referring to.

Can you please provide additional details, exact changes to the code and steps to reproduce this.

  • Modify CefSharp.WinForms.Example/Program.cs CefSettings to add renderer-process-limit of 2.
  • Start the application
  • Open the JS Binding test page
  • Keep opening new tabs for the JS Binding test page until it doesn't work
  • This test works when there is no renderer-process-limit

Did you debug the renderer process and confirm the issue you are seeing is actually related to this particular issue?

I did not debug CefSharp code yet but it's on my list of things to do. From the debugging I did, I could see that the binding object was present in the browser and that the call was made but it never triggered our code, thus making the conclusion it was between CEF or CefSharp at this point. This issue seemed most relevent and since RequestContext fixed the bug by introducing one renderer process per browser instance, we decided to seek help at this point.

If there are alternative methods or avenues we might try, I am open to suggestions and trying them out.

@amaitland
Copy link
Member Author

@ebelanger Thanks for the clarification 👍

The browser instances used in our application are of the same origin except in rare cases; with no external navigation. There's potentially an unlimited number. We could consider them like tabs in Chrome.

If they are in the same origin then they should by default already share the same render process. Under what scenario are you seeing additional render processes spawned?

Please make sure you've read https://bitbucket.org/chromiumembedded/cef/issues/2498/add-support-for-site-per-process

With site-per-process enabled a spare renderer process will be created for use with a future browser or navigation.

As per https://bitbucket.org/chromiumembedded/cef/issues/2498/add-support-for-site-per-process#comment-54186905

You should be able to disable the spare render process using the following command line arg.

--disable-features=SpareRendererForSitePerProcess
  • Modify CefSharp.WinForms.Example/Program.cs CefSettings to add renderer-process-limit of 2.

If you skip this step then does it behave as expected? Are you seeing additional render processes created?

Versions of CefSharp prior to 63 relied on process-per-tab for javascript binding, this forced a render process per ChromiumWebBrowser instance. This is no longer the case, the default Site Isolation model is used, same origin websites should share the same domain. Are you basing your assumption that you need to limit the render processes on past experience or current observation?

your sample simply by moving the two private field

@Termit1975 I have no idea what you mean by this. Please provide context/link etc

@Termit1975
Copy link

We still have an issue with our application which we haven't figured yet but we managed to fix the scenario Eric refers to simple by moving

        gcroot<Dictionary<String^, JavascriptObject^>^> _javascriptObjects;
        gcroot<RegisterBoundObjectRegistry^> _registerBoundObjectRegistry;

from CefAppUnmanagedWrapper to CefBrowserWrapper, we obviously did the same as you did with JavascriptRootObjectWrapper encapsulating into dictionaries index by FrameId this way

        property ConcurrentDictionary<int64, Dictionary<String^, JavascriptObject^>^>^ JavascriptScriptObjects;
        property ConcurrentDictionary<int64, RegisterBoundObjectRegistry^>^ RegisterBoundObjectRegistries;

we also did some small refactoring such as moving

        JavascriptRootObjectWrapper^ CefAppUnmanagedWrapper::GetJsRootObjectWrapper(int browserId, int64 frameId)

to CefBrowserWrapper which handles the lazy creation by FrameId

and removing duplicate code refering to some weird bug that requires lazy creation as we centralized that behavior in CefBrowserWrapper

last thing we did is that we forced frameId to -1 (we chose a random value which is not relevant) when we were in legacyMode (which we aren't) but we saw that everything was shared at that point (that path would need to be tested obviously, but sample passes (not sure at this point if this scenario is covered)

@amaitland
Copy link
Member Author

@Termit1975 Its' really not workable to provide code in this format, I'd have to dig through code and take guesses at what you meant. I quite simply don't have time for that.

Based on the described scenario above it's not clear you actually need to limit the number of render processes.

@amaitland
Copy link
Member Author

I would highly recommend debugging the code as is to determine what causes the original issue you were seeing. In the context of the example projects the same set of objects should be reused, they in theory should just be cached. There's a chance what you are seeing is an unexpected behaviour from Chromium.

@ebelanger
Copy link

Hello @amaitland,

After debugging, we found out the problem was not related to this issue. We now seem to hit a limit on the number of browser instances (around 80) before binding calls return failed empty string promises. Creating the browser instance with a RequestContext seems to fix this problem.

Sorry again for polluting this thread.

@amaitland amaitland changed the title Javascript Binding v2 Store JavascriptObjects at the Browser level Javascript Binding - Store JavascriptObjects/Settings per CefBrowser Jun 11, 2021
@amaitland
Copy link
Member Author

Other settings like _jsBindingApiEnabled, _jsBindingPropertyName, _jsBindingPropertyNameCamelCase, _focusedNodeChangedEnabled need also be stored at the CefBrowser level to ensure they are unique per ChromiumWebBrowser instance.

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

Successfully merging a pull request may close this issue.

4 participants