addref to obs_source_t* before getProperties#1701
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses a rare crash when retrieving properties from a source (notably browser_source) by holding a strong reference to the obs_source_t during GetProperties, and adds a regression test intended to exercise the release-vs-properties race.
Changes:
- Update
Source::GetPropertiesto use anOBSSourcestrong reference when looking up the source by id. - Add a Mocha test that races
input.propertiesagainstinput.release()across multiple iterations to detect crashes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
obs-studio-server/source/osn-source.cpp |
Uses a strong source reference during GetProperties to reduce lifetime/race issues. |
tests/osn-tests/src/test_osn_source.ts |
Adds a concurrency-focused regression test for BrowserSource properties retrieval vs release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* add concurrency test * addref to obs_source_t* before calling getProperties to handle the rare race where the browser_source was destroyed right after the null check on the url but before we could get the path. Typically, obs adds a ref but externally we cannot invoke obs_source_addref() directly. So instead we will use OBSSource * update skipSource() so these tests can be run locally on Darwin
sandboxcoder
force-pushed
the
rno/browser-src-getprops
branch
from
55716cc to
167efab
Compare
| // Attempt to find the source asked to load. Get a strong reference to help | ||
| // guarantee the source is not destroyed while we are attempting to retrieve | ||
| // properties. | ||
| OBSSource src = osn::Source::Manager::GetInstance().find(args[0].value_union.ui64); |
There was a problem hiding this comment.
good suggestion. However,osn::Source::Manager is a generic Singleton container. This suggestion means the ref counting belongs at the container level where we increment the ref count upon assignment (actually, the suggestion actually means just add a new findStrongRef function)
There was a problem hiding this comment.
Actually I do not think I need to add this since OBSSource should implicitly handle this.
| it('Get properties of browser source while releasing concurrently does not crash', async function() { | ||
| if (obs.skipSource(EOBSInputTypes.BrowserSource)) { this.skip(); } // Skip if browser source is not supported | ||
| const iterations = 20; | ||
| const promises: Promise<void>[] = []; | ||
|
|
||
| for (let i = 0; i < iterations; i++) { | ||
| const input = osn.InputFactory.create(EOBSInputTypes.BrowserSource, 'browser_concurrent_' + i); | ||
| expect(input).to.not.equal(undefined, GetErrorMessage(ETestErrorMsg.CreateInput, EOBSInputTypes.BrowserSource)); | ||
|
|
||
| // Race getProperties against release on separate async ticks | ||
| const getProps = new Promise<void>(resolve => { | ||
| setImmediate(() => { | ||
| try { input.properties; } catch (_) {} | ||
| resolve(); | ||
| }); | ||
| }); | ||
|
|
||
| const releaseSource = new Promise<void>(resolve => { | ||
| setImmediate(() => { | ||
| try { input.release(); } catch (_) {} | ||
| resolve(); | ||
| }); | ||
| }); | ||
|
|
||
| promises.push(getProps, releaseSource); | ||
| } | ||
|
|
||
| // If the race condition is present, one of these will crash the OBS server process | ||
| await Promise.all(promises); |
There was a problem hiding this comment.
This is true, it appears this is mostly a stress test in its' current form. Would likely need to convert this into a C++ Catch2 test
Description
NULLpointer somehow gets inside ofstrchreven though we already checked for NULL. The theory is the browser_source is released by another thread while we are getting properties looking at the call stack from sentry.obs_source_addref()directly. So instead we will use OBSSourceMotivation and Context
Fix for a crash report on sentry. The most likely root cause is a race condition: the browser source is released (its private data freed) between the null check at line 254 and the actual obs_source_properties call at line 261, so
browser_source_get_properties sees a partially-freed struct where the string field is null (see slobs repo-
obs-browser-plugin.cpp).How Has This Been Tested?
Adding test case that will validate this code.
Types of changes
Checklist: