Skip to content

Commit fca89c7

Browse files
sandboxcodersandboxcoder
authored
fix test_osn_global crash / protect against race conditions / check for invalid canvas (#1597)
* set crashpad executable permissions (mac CI config) * fix crashpad errors in the test logs * ignore removing the Crashpad folder test_osn_global: get memoryUsage value (fix typo?) unique_object_manager: fix for possible race condition osn-video.cpp: check for canvas* & fix xcode warning blog() string literal video.cpp: setflag to indicate video device was invalidated test_osn_global -> use synchronous func instead of lamda * dangling source pointer ref crash fix * update global::SetOutputSource() to accept null source so we can properly remove source from output channel
1 parent 722a2a2 commit fca89c7

7 files changed

Lines changed: 62 additions & 30 deletions

File tree

‎.github/workflows/main.yml‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,10 @@ jobs:
131131
with:
132132
name: build-artifacts-mac-${{matrix.Architecture}}
133133
path: .
134-
- name: Set obs64 permissions
135-
run: 'chmod +x streamlabs-build.app/distribute/obs-studio-node/bin/obs64'
134+
- name: Set executable permissions
135+
run: |
136+
chmod +x streamlabs-build.app/distribute/obs-studio-node/bin/obs64
137+
chmod +x streamlabs-build.app/distribute/obs-studio-node/crashpad_*
136138
shell: bash
137139
- name: 'Run tests'
138140
timeout-minutes: 30

‎obs-studio-client/source/video.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ void osn::Video::Destroy(const Napi::CallbackInfo &info)
104104
return;
105105

106106
std::vector<ipc::value> response = conn->call_synchronous_helper("Video", "RemoveVideoContext", {ipc::value((uint64_t)(this->canvasId))});
107-
107+
isLastVideoValid = false;
108108
return;
109109
}
110110

‎obs-studio-server/source/osn-global.cpp‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -93,14 +93,18 @@ void osn::Global::SetOutputSource(void *data, const int64_t id, const std::vecto
9393
}
9494

9595
obs_set_output_source(channel, source);
96-
obs_source_t *newsource = obs_get_output_source(channel);
97-
if (newsource != source) {
96+
if (source) {
97+
obs_source_t *newsource = obs_get_output_source(channel);
98+
if (newsource != source) {
99+
obs_source_release(newsource);
100+
PRETTY_ERROR_RETURN(ErrorCode::Error, "Failed to set output source.");
101+
} else {
102+
rval.push_back(ipc::value((uint64_t)ErrorCode::Ok));
103+
}
98104
obs_source_release(newsource);
99-
PRETTY_ERROR_RETURN(ErrorCode::Error, "Failed to set output source.");
100105
} else {
101106
rval.push_back(ipc::value((uint64_t)ErrorCode::Ok));
102107
}
103-
obs_source_release(newsource);
104108
AUTO_DEBUG;
105109
}
106110

‎obs-studio-server/source/osn-video.cpp‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,9 @@ void osn::Video::GetEncodedFrames(void *data, const int64_t id, const std::vecto
101101
void osn::Video::GetVideoContext(void *data, const int64_t id, const std::vector<ipc::value> &args, std::vector<ipc::value> &rval)
102102
{
103103
obs_video_info *canvas = osn::Video::Manager::GetInstance().find(args[0].value_union.ui64);
104-
104+
if (!canvas) {
105+
PRETTY_ERROR_RETURN(ErrorCode::Error, "No video context is currently set.");
106+
}
105107
rval.push_back(ipc::value((uint64_t)ErrorCode::Ok));
106108

107109
rval.push_back(ipc::value(canvas->fps_num));
@@ -299,6 +301,9 @@ void osn::Video::SetVideoContext(void *data, const int64_t id, const std::vector
299301
}
300302

301303
obs_video_info *canvas = osn::Video::Manager::GetInstance().find(args[11].value_union.ui64);
304+
if (!canvas) {
305+
PRETTY_ERROR_RETURN(ErrorCode::Error, "No video context is currently set.");
306+
}
302307
obs_video_info video = *canvas;
303308

304309
#ifdef _WIN32
@@ -403,7 +408,7 @@ void osn::Video::RemoveVideoContext(void *data, const int64_t id, const std::vec
403408
ret = obs_remove_video_info(canvas);
404409

405410
} catch (const char *error) {
406-
blog(LOG_ERROR, error);
411+
blog(LOG_ERROR, "Error occurred while removing video %s", error);
407412
}
408413

409414
if (ret != OBS_VIDEO_SUCCESS) {

‎obs-studio-server/source/utility.hpp‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -151,14 +151,23 @@ template<typename T> class unique_object_manager {
151151

152152
void for_each(std::function<void(T *)> for_each_method)
153153
{
154+
std::lock_guard<std::recursive_mutex> lock(internal_mutex);
154155
for (auto it = object_map.begin(); it != object_map.end(); ++it) {
155156
for_each_method(it->second);
156157
}
157158
}
158159

159-
size_t size() { return object_map.size(); }
160+
size_t size()
161+
{
162+
std::lock_guard<std::recursive_mutex> lock(internal_mutex);
163+
return object_map.size();
164+
}
160165

161-
void clear() { object_map.clear(); }
166+
void clear()
167+
{
168+
std::lock_guard<std::recursive_mutex> lock(internal_mutex);
169+
object_map.clear();
170+
}
162171
};
163172

164173
template<typename T> class generic_object_manager {
@@ -234,14 +243,23 @@ template<typename T> class generic_object_manager {
234243

235244
void for_each(std::function<void(T &)> for_each_method)
236245
{
246+
std::lock_guard<std::recursive_mutex> lock(internal_mutex);
237247
for (auto it = object_map.begin(); it != object_map.end(); ++it) {
238248
for_each_method(it->second);
239249
}
240250
}
241251

242-
size_t size() { return object_map.size(); }
252+
size_t size()
253+
{
254+
std::lock_guard<std::recursive_mutex> lock(internal_mutex);
255+
return object_map.size();
256+
}
243257

244-
void clear() { object_map.clear(); }
258+
void clear()
259+
{
260+
std::lock_guard<std::recursive_mutex> lock(internal_mutex);
261+
object_map.clear();
262+
}
245263
};
246264

247265
void ProcessProperties(obs_properties_t *prp, obs_data *settings, std::vector<ipc::value> &rval);

‎tests/osn-tests/src/test_osn_global.ts‎

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,12 @@ describe(testName, () => {
2222
});
2323

2424
// Shutdown OBS process
25-
after(async function() {
25+
after(function() {
2626
obs.shutdown();
2727

2828
if (hasTestFailed === true) {
2929
logInfo(testName, 'One or more test cases failed. Uploading cache');
30-
await obs.uploadTestCache();
30+
obs.uploadTestCache();
3131
}
3232

3333
obs = null;
@@ -42,7 +42,7 @@ describe(testName, () => {
4242
}
4343
});
4444

45-
it('Set source to output channel and get it', () => {
45+
it('Set source to output channel and get it', function () {
4646
// Creating input source
4747
const input = osn.InputFactory.create(EOBSInputTypes.ImageSource, 'test_osn_global_source');
4848

@@ -51,21 +51,23 @@ describe(testName, () => {
5151
expect(input.id).to.equal(EOBSInputTypes.ImageSource, GetErrorMessage(ETestErrorMsg.InputId, EOBSInputTypes.ImageSource));
5252
expect(input.name).to.equal('test_osn_global_source', GetErrorMessage(ETestErrorMsg.InputName, EOBSInputTypes.ImageSource));
5353

54+
const channel = 1;
5455
// Setting input source to output channel
55-
osn.Global.setOutputSource(1, input);
56+
osn.Global.setOutputSource(channel, input);
5657

5758
// Getting input source from output channel
58-
const channel = 1;
5959
const returnSource = osn.Global.getOutputSource(channel);
6060

6161
// Checking if input source returned previously is correct
6262
expect(returnSource).to.not.equal(undefined, GetErrorMessage(ETestErrorMsg.NoInputInChannel, channel.toString()));
6363
expect(returnSource.id).to.equal(EOBSInputTypes.ImageSource, GetErrorMessage(ETestErrorMsg.InputFromChannelId));
6464
expect(returnSource.name).to.equal('test_osn_global_source', GetErrorMessage(ETestErrorMsg.InputFromChannelName));
65+
const nullSource : ISource = null;
66+
osn.Global.setOutputSource(channel, nullSource); // We must clear the channel before deleting the input source to prevent audio thread crash
6567
input.release();
6668
});
6769

68-
it('Get flags (capabilities) of a source type', () => {
70+
it('Get flags (capabilities) of a source type', function () {
6971
let flags: number = undefined;
7072

7173
// For each input type available get their flags and check if they are not undefined
@@ -77,7 +79,7 @@ describe(testName, () => {
7779
});
7880
});
7981

80-
it('Get lagged frames value', () => {
82+
it('Get lagged frames value', function () {
8183
let laggedFrames: number = undefined;
8284

8385
// Getting lagged frames value
@@ -87,7 +89,7 @@ describe(testName, () => {
8789
expect(laggedFrames).to.not.equal(undefined, GetErrorMessage(ETestErrorMsg.LaggedFrames));
8890
});
8991

90-
it('Get total frames value', () => {
92+
it('Get total frames value', function () {
9193
let totalFrames: number = undefined;
9294

9395
// Getting total frames value
@@ -97,7 +99,7 @@ describe(testName, () => {
9799
expect(totalFrames).to.not.equal(undefined, GetErrorMessage(ETestErrorMsg.TotalFrames));
98100
});
99101

100-
it('Set locale and get it', () => {
102+
it('Set locale and get it', function () {
101103
let locale: string;
102104

103105
// Setting locale
@@ -110,7 +112,7 @@ describe(testName, () => {
110112
expect(locale).to.equal('pt-BR', GetErrorMessage(ETestErrorMsg.Locale));
111113
});
112114

113-
it('Get CPU percentage', () => {
115+
it('Get CPU percentage', function () {
114116
let cpuPercent: number = undefined;
115117

116118
// Getting CPU %
@@ -120,7 +122,7 @@ describe(testName, () => {
120122
expect(cpuPercent).to.not.equal(undefined, GetErrorMessage(ETestErrorMsg.CPUPercent));
121123
});
122124

123-
it('Get current frame rate', () => {
125+
it('Get current frame rate', function () {
124126
let frameRate: number = undefined;
125127

126128
// Getting CPU %
@@ -130,7 +132,7 @@ describe(testName, () => {
130132
expect(frameRate).to.not.equal(undefined, GetErrorMessage(ETestErrorMsg.FrameRate));
131133
});
132134

133-
it('Get average time to render', () => {
135+
it('Get average time to render', function () {
134136
let renderTime: number = undefined;
135137

136138
// Getting CPU %
@@ -140,7 +142,7 @@ describe(testName, () => {
140142
expect(renderTime).to.not.equal(undefined, GetErrorMessage(ETestErrorMsg.FrameRenderTime));
141143
});
142144

143-
it('Get available disk space', () => {
145+
it('Get available disk space', function () {
144146
let diskSpace: number = undefined;
145147

146148
// Getting CPU %
@@ -150,17 +152,17 @@ describe(testName, () => {
150152
expect(diskSpace).to.not.equal(undefined, GetErrorMessage(ETestErrorMsg.DiskSpace));
151153
});
152154

153-
it('Get memory usage', () => {
155+
it('Get memory usage', function () {
154156
let mem: number = undefined;
155157

156158
// Getting CPU %
157-
mem = osn.Global.diskSpaceAvailable;
159+
mem = osn.Global.memoryUsage;
158160

159161
// Checking if CPU % was returned correctly
160162
expect(mem).to.not.equal(undefined, GetErrorMessage(ETestErrorMsg.MemUsage));
161163
});
162164

163-
it('Fail test - Get source from empty output channel', () => {
165+
it('Fail test - Get source from empty output channel', function () {
164166
let input: ISource;
165167
let channel: number = 5;
166168

‎tests/osn-tests/util/general.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,8 @@ export function deleteConfigFiles(): void {
8080
let currentFile: string;
8181
let files = fs.readdirSync(configFolderPath);
8282
files.forEach(file => {
83-
if (file !== 'node-obs') {
83+
// Ignore mac Crashpad and node-obs/logs folders.
84+
if (file !== 'node-obs' && file !== 'Crashpad') {
8485
currentFile = file;
8586
try {
8687
fs.unlinkSync(path.join(configFolderPath, file));

0 commit comments

Comments
 (0)