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

CefBrowserHost::CreateBrowser call failed when trying to reuse BrowserSettings instance #2643

Closed
PitySOFT opened this issue Feb 5, 2019 · 21 comments

Comments

@PitySOFT
Copy link

PitySOFT commented Feb 5, 2019

  • What version of the product are you using?

    • Nuget 71.0.0
  • What architecture x86 or x64?

    • x86
  • On what operating system?

    • Win 8 and Win10
  • Are you using WinForms, WPF or OffScreen?

    • WPF with WindowsFormsHost
  • What steps will reproduce the problem?

    • I have two instances of CefSharp with different CefSettings, because I can't change the settings on runtime. Depending on some data I choose what instance to show. Till version 71.0 the code where si make the swap between those two instances works great. But with version 71.0 it breaks with the exception attached below.

    • I try to set the Child of a WindowsFormsHost with an instance of a ChromiumWebBrowser with the following code MyFormsHost.Child = my ChromiumWebBrowser;

  • What is the expected output? What do you see instead?

    • The application crashes because on unhandled exception.
  • Please provide any additional information below.

    • System.InvalidOperationException
      HResult=0x80131509
      Message=CefBrowserHost::CreateBrowser call failed, review the CEF log file for more details.
      Source=CefSharp.Core
      StackTrace:
      at CefSharp.ManagedCefBrowserAdapter.CreateBrowser(IWindowInfo windowInfo, BrowserSettings browserSettings, RequestContext requestContext, String address) in c:\projects\cefsharp\cefsharp.core\managedcefbrowseradapter.cpp:line 29

    • The debug.log file is empty

  • Does this problem also occur in the CEF Sample Application from.

    • The examples are not relevant to this because I used 2 instances of CefSharp
@amaitland
Copy link
Member

Can you please fork https://github.com/cefsharp/CefSharp.MinimalExample and provide an example that reproduce your problem.

Using the WinForms version in a WPF app using WindowsFormsHost works as expected for me.

Without an example that actually reproduces your problem there is anything I can do.

@amaitland amaitland added the unable-to-reproduce The problem could not be reproduced by developers label Feb 7, 2019
@cefsharp cefsharp deleted a comment from welcome bot Feb 8, 2019
@amaitland
Copy link
Member

@PitySOFT Are you able to provide an example that reproduces your problem? I'll be releasing 71.0.1 bug fix packages in a few days, if you can provide an example in the next 48hrs I'll see if this is fixable, otherwise it'll have to wait for the next major release.

@PitySOFT
Copy link
Author

Hi, didn't had time to make the example. I will come back with it when I'm ready to reproduce. Please release the 17.0.1 don't wait for me. Thanks.

@PitySOFT
Copy link
Author

PitySOFT commented Feb 12, 2019

Hi,

I manage to reproduce the crash in a minimal project.

You have the project here.

Binary file removed

With Cef 69.0.0 will work fine, but using 71.0.0 will crash.

@merceyz
Copy link
Member

merceyz commented Feb 12, 2019

No binary files, fork the minimal project instead and provide that as a link

@PitySOFT
Copy link
Author

@merceyz
Copy link
Member

merceyz commented Feb 12, 2019

Yes, thank you.

The issue is you're reusing the browser settings, the settings are disposed of after the browser is created at:

if (browserSettings != null)
{
browserSettings.Dispose();
browserSettings = null;
}

This was added in 4a41fdf for WinForms

@merceyz merceyz removed the unable-to-reproduce The problem could not be reproduced by developers label Feb 12, 2019
@amaitland
Copy link
Member

Reusing browser settings is problematic, so making them single use only.

Need an additional check and throw exception if they're disposed, will add that shortly..

Thanks for taking the time to reproduce 👍

@merceyz
Copy link
Member

merceyz commented Feb 12, 2019

The OffScreen version already checks this[1], seems the check is missing for WPF and WinForms

1:

else if(browserSettings.IsDisposed)
{
throw new ObjectDisposedException("browserSettings", "The BrowserSettings reference you have passed has already been disposed. You cannot reuse the BrowserSettings class");
}

@amaitland
Copy link
Member

amaitland commented Feb 13, 2019

Now that browser creation is common between WPF, informs and OffScreen checks can be made in

if (browserSettings == nullptr)

In WinForms it might make sense to remove the setter for browser settings.

Will provide full analysis for discussion when I am in front of a computer

@PitySOFT
Copy link
Author

Thanks for information, I confirm that not using the same BrowserSettings works.

For the same example code I have some issues with onscreen keyboard for touch screens. I know that it's hard to test because the lack of the touch screens but maybe you can check something in the code.

On CefSharp 65.0.1 the keyboard works perfect, but:

  • on 67.0.0 when the onscreen keyboard appears the sides of the keyboard are transparent but after a short period the sides becomes white.
  • on 69.0.0 and 71.0.0 keyboard appears with transparent on sides, then this becomes white and then the keyboard will disappear.

I can provide videos for all situations.

@amaitland
Copy link
Member

Any problems with the onscreen keyboard would need to be fixed in CEF/Chromium, see https://github.com/cefsharp/CefSharp/blob/master/CONTRIBUTING.md#cefsharp-vs-chromium-embedded-frameworkcef

@merceyz merceyz changed the title CefBrowserHost::CreateBrowser call failed when try to set a WindowsFormsHost child as ChromiumWebBrowser CefBrowserHost::CreateBrowser call failed when trying to reuse BrowserSettings instance Feb 17, 2019
@amaitland
Copy link
Member

Firstly it's possible to reuse a CefBrowserSettings instance, it's just a struct, the problem lies with the BrowserSettings wrapper and how we control the freeing of unmanaged resources. Typically I've taken the approach to be fairly aggressive with cleaning up resources, occasionally this makes things harder to use, reduces the likeliness of hard to track down memory leaks. (CEF has just made some improvements for tracking down stray references, hopefully this improves things going forward, see https://bitbucket.org/chromiumembedded/cef/issues/2593/improvements-for-debugging-cefrefptr).

Every so often I would see users attempting to reuse a BrowserSettings instance in the following scenario.

  • Create BrowserSettings
  • Assign to ChromiumWebBrowser
  • Dispose of ChromiumWebBrowser
  • Assign BrowserSettings to new ChromiumWebBrowser instance, browser creation fails (slightly different set of steps to reproduce, same outcome as this particular issue).

My initial reaction was to simply things and just have a one to one mapping of BrowserSettings to ChromiumWebBrowser instance.

How important is it to reuse a BrowserSettings instance? Open to discussion on this one. We can remove the Dispose calls from the browser and rely on the User/Garbage Collector to handle the object, it's a wrapper around a every basic struct, doesn't hold any external references, so the GC should deal with it nicely, unlike other objects, having it leak won't stop CEF from shutting down properly. So perhaps the simplest and most flexible option is to remove all these additional checks and Dispose calls?

@amaitland
Copy link
Member

Having through about it I'm think of removing the restrictions and remove the code to Dispose of the BrowserSettings from the ChromiumWebBrowser instances. Will rely on GC/User to Dispose of the BrowserSettings object.

@amaitland
Copy link
Member

Ended up going with what I think should be a happy medium. If CefSharp creates the BrowserSettings instance for you then it will automatically be Disposed. If you create the BrowserSettings instance yourself then either call Dispose explicitly or let the GC collect the object.

Changes were made to WPF, WinForms and OffScreen so they are now all consistent. Passing an already Disposed instance of BrowserSettings to the underlying CreateBrowser method will result in an ObjectDisposedException.

@merceyz
Copy link
Member

merceyz commented Mar 21, 2019

Changes were made to WPF, WinForms and OffScreen so they are now all consistent

@amaitland Any reason it wasn't implemented in the underlying CreateBrowser method?

amaitland added a commit that referenced this issue Mar 22, 2019
…BrowserAdapter::CreateBrowser

- Improve BrowserSettings disposed check message

Follow up to #2643
@amaitland
Copy link
Member

@amaitland Any reason it wasn't implemented in the underlying CreateBrowser method?

@merceyz Excellent suggestion 👍 Follow up commit d2d1e6f

Error message improved slightly.

@merceyz
Copy link
Member

merceyz commented Mar 22, 2019

Excellent, however I was thinking it would be better to move all of it into that method. So that the same logic isn't repeated over all implementations

@amaitland
Copy link
Member

Excellent, however I was thinking it would be better to move all of it into that method. So that the same logic isn't repeated over all implementations

What logic are you referring to?

@merceyz
Copy link
Member

merceyz commented Mar 22, 2019

if (browserSettings == null)
{
browserSettings = new BrowserSettings(frameworkCreated: true);
}

This could be handled in the managedCefBrowserAdapter, meaning the new constructor and property can be removed

@amaitland
Copy link
Member

This could be handled in the managedCefBrowserAdapter, meaning the new constructor and property can be removed

Yes it could. That wasn't the plan though. that would be a breaking change forcing users to always create BrowserSettings objects.

Code like https://github.com/cefsharp/CefSharp/blob/cefsharp/67/CefSharp.Wpf.Example/Views/BrowserTabView.xaml.cs#L28 would all of a sudden be a NullReferenceException.

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

3 participants