obsproject / obs-browser

CEF-based OBS Studio browser plugin
GNU General Public License v2.0
776 stars 220 forks source link

Add lock for modify BrowserSource::cefBrowser #325

Closed walker-WSH closed 2 years ago

walker-WSH commented 2 years ago

Description

I meet a crash that caused by threadsafe of BrowserSource::cefBrowser.

_obs_graphicsthread is getting this shared-ptr variable in BrowserSource::ExecuteOnBrowser. While calling virtual AddRef(), app crashed at _purecall().

Check the CEF main thread, I find code location stay at BrowserSource::CreateBrowser(): cefBrowser = CefBrowserHost::CreateBrowserSync(...)

Obviously, while changing cefBrowser, lock is need.

Motivation and Context

make sure threadsafe

How Has This Been Tested?

changing url/resolution frequently

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

WizardCM commented 2 years ago

Hi there, please rebase this PR.

Additionally, is this only an issue in browser sources & not browser docks/panels?

I wonder if a better solution would be to not destroy/recreate the entire source when changing URL or resizing, though having this solution as backup in other situations may be a good idea regardless.

walker-WSH commented 2 years ago

Hi there, please rebase this PR.

Additionally, is this only an issue in browser sources & not browser docks/panels?

I wonder if a better solution would be to not destroy/recreate the entire source when changing URL or resizing, though having this solution as backup in other situations may be a good idea regardless.

While cefBrowser in docks/panels is creating, if it may be accessed outside CEF's main thread, I think it also needs a lock. This should be confirm because I am not familiar enough to docks/panels.

The way you said that not destroy/recreate the entire source can only reduce this issue.