Created
December 6, 2021 13:34
-
-
Save atmouse-/b0c6a4b93a228bb0dafe32f0b7f94d76 to your computer and use it in GitHub Desktop.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| From 07749a072830864211d045ea196490b2f56c4e1d Mon Sep 17 00:00:00 2001 | |
| From: jingjiezhuang <jingjie.zhuang@igg.com> | |
| Date: Sun, 5 Dec 2021 19:23:35 +0800 | |
| Subject: [PATCH] Use an AtomicPtr for PulseStream's drain_timer | |
| There is a race condition between `drained_cb` and `PulseStream::stop` that | |
| happens reliably on Firefox CI with rust 1.56 beta (LLVM 13) and PGO | |
| instrumentation. Here's how it goes: | |
| - in the Firefox AudioIPC Server RPC thread, `PulseStream::stop` is | |
| called | |
| - `PulseStream::stop enters the loop waiting for drain, and blocks on | |
| `mainloop.wait` | |
| - Later, some other thread calls `drained_cb`, which resets `drain_timer`, | |
| and signals the mainloop. | |
| - Back the other AudioIPC Server RPC thread, `mainloop.wait` returns, | |
| looping back to the test for `drain_timer`... which this thread | |
| doesn't know had been updated yet, so it blocks on `mainloop.wait` | |
| again. | |
| --- | |
| third_party/rust/cubeb-pulse/.cargo-checksum.json | 2 +- | |
| third_party/rust/cubeb-pulse/src/backend/stream.rs | 33 +++++++++++++++++++-------------- | |
| 2 file changed, 20 insertions(+), 15 deletions(-) | |
| diff --git a/third_party/rust/cubeb-pulse/src/backend/stream.rs b/third_party/rust/cubeb-pulse/src/backend/stream.rs | |
| index a210e2d..b9d4580 100644 | |
| --- a/third_party/rust/cubeb-pulse/src/backend/stream.rs | |
| +++ b/third_party/rust/cubeb-pulse/src/backend/stream.rs | |
| @@ -13,7 +13,7 @@ use std::{mem, ptr}; | |
| use std::ffi::{CStr, CString}; | |
| use std::os::raw::{c_long, c_void}; | |
| use std::slice; | |
| -use std::sync::atomic::{AtomicUsize, Ordering}; | |
| +use std::sync::atomic::{AtomicPtr, AtomicUsize, Ordering}; | |
| use ringbuf::RingBuffer; | |
| use self::RingBufferConsumer::*; | |
| @@ -267,7 +267,7 @@ pub struct PulseStream<'ctx> { | |
| input_stream: Option<pulse::Stream>, | |
| data_callback: ffi::cubeb_data_callback, | |
| state_callback: ffi::cubeb_state_callback, | |
| - drain_timer: *mut pa_time_event, | |
| + drain_timer: AtomicPtr<pa_time_event>, | |
| output_sample_spec: pulse::SampleSpec, | |
| input_sample_spec: pulse::SampleSpec, | |
| output_frame_count: AtomicUsize, | |
| @@ -399,7 +399,7 @@ impl<'ctx> PulseStream<'ctx> { | |
| data_callback: data_callback, | |
| state_callback: state_callback, | |
| user_ptr: user_ptr, | |
| - drain_timer: ptr::null_mut(), | |
| + drain_timer: AtomicPtr::new(ptr::null_mut()), | |
| output_sample_spec: pulse::SampleSpec::default(), | |
| input_sample_spec: pulse::SampleSpec::default(), | |
| output_frame_count: AtomicUsize::new(0), | |
| @@ -564,9 +564,10 @@ impl<'ctx> PulseStream<'ctx> { | |
| self.context.mainloop.lock(); | |
| { | |
| if let Some(stm) = self.output_stream.take() { | |
| - if !self.drain_timer.is_null() { | |
| + let drain_timer = self.drain_timer.load(Ordering::Acquire); | |
| + if !drain_timer.is_null() { | |
| /* there's no pa_rttime_free, so use this instead. */ | |
| - self.context.mainloop.get_api().time_free(self.drain_timer); | |
| + self.context.mainloop.get_api().time_free(drain_timer); | |
| } | |
| stm.clear_state_callback(); | |
| stm.clear_write_callback(); | |
| @@ -626,7 +627,7 @@ impl<'ctx> StreamOps for PulseStream<'ctx> { | |
| self.context.mainloop.lock(); | |
| self.shutdown = true; | |
| // If draining is taking place wait to finish | |
| - while !self.drain_timer.is_null() { | |
| + while !self.drain_timer.load(Ordering::Acquire).is_null() { | |
| self.context.mainloop.wait(); | |
| } | |
| self.context.mainloop.unlock(); | |
| @@ -974,11 +975,12 @@ impl<'ctx> PulseStream<'ctx> { | |
| u: *mut c_void, | |
| ) { | |
| let stm = unsafe { &mut *(u as *mut PulseStream) }; | |
| - debug_assert_eq!(stm.drain_timer, e); | |
| + let drain_timer = stm.drain_timer.load(Ordering::Acquire); | |
| + debug_assert_eq!(drain_timer, e); | |
| stm.state_change_callback(ffi::CUBEB_STATE_DRAINED); | |
| /* there's no pa_rttime_free, so use this instead. */ | |
| - a.time_free(stm.drain_timer); | |
| - stm.drain_timer = ptr::null_mut(); | |
| + a.time_free(drain_timer); | |
| + stm.drain_timer.store(ptr::null_mut(), Ordering::Release); | |
| stm.context.mainloop.signal(); | |
| } | |
| @@ -1070,13 +1072,16 @@ impl<'ctx> PulseStream<'ctx> { | |
| /* pa_stream_drain is useless, see PA bug# 866. this is a workaround. */ | |
| /* arbitrary safety margin: double the current latency. */ | |
| - debug_assert!(self.drain_timer.is_null()); | |
| + debug_assert!(self.drain_timer.load(Ordering::Acquire).is_null()); | |
| let stream_ptr = self as *const _ as *mut _; | |
| if let Some(ref context) = self.context.context { | |
| - self.drain_timer = context.rttime_new( | |
| - pulse::rtclock_now() + 2 * latency, | |
| - drained_cb, | |
| - stream_ptr, | |
| + self.drain_timer.store( | |
| + context.rttime_new( | |
| + pulse::rtclock_now() + 2 * latency, | |
| + drained_cb, | |
| + stream_ptr, | |
| + ), | |
| + Ordering::Release, | |
| ); | |
| } | |
| self.shutdown = true; | |
| diff --git a/third_party/rust/cubeb-pulse/.cargo-checksum.json b/third_party/rust/cubeb-pulse/.cargo-checksum.json | |
| index f93b0fe..1da0be0 100644 | |
| --- a/third_party/rust/cubeb-pulse/.cargo-checksum.json | |
| +++ b/third_party/rust/cubeb-pulse/.cargo-checksum.json | |
| @@ -1 +1 @@ | |
| -{"files":{".editorconfig":"bf047bd1da10cabb99eea666d1e57c321eba4716dccb3e4ed0e2c5fe3ca53858",".travis.yml":"0394e2adb041175457685cde5ee05ff04bdab8885fd8a62551f2ff43d9e48872","AUTHORS":"0e0ac930a68ce2f6b876126b195add177f0d3886facb9260f4d9b69f1988f0cc","Cargo.toml":"56e90cb82ec36ead07e551a28fc2455fa658fa8308c3d73f8d856d85bfcd2122","LICENSE":"44c6b5ae5ec3fe2fbc608b00e6f4896f4d2d5c7e525fcbaa3eaa3cf2f3d5a983","README.md":"e6a98ee5630b9ce1a096a2907d095454f2770e298a5b0976ab552cc53ca96cfc","src/backend/context.rs":"33d9fdf1504fe1ae43d301e288daf6eaeabeb47aa0ef86efa135c6d984425fc4","src/backend/cork_state.rs":"4a0f1afc7d9f333dac89218cc56d7d32fbffb487cd48c1c9a4e03d79cb3b5e28","src/backend/intern.rs":"374a9a3bd79fddc47739dda1dbfc5929aea5a91946794fe65fba3c8d130fbda9","src/backend/mod.rs":"06ce9250865abf0ea461f215b128470636d072a6776821efef3caf5a7b992fb9","src/backend/stream.rs":"b8700fffb4d1537bc2fd3f0e26e7bbb16bc6e7cc7a803723e06704610ca1f6f5","src/capi.rs":"b2c1be8128cadd36caa65c80950440f9d6f2aa0c24cc7bae6a9eaf6347ac454d","src/lib.rs":"7282560d84b134b09acfd8d6282600982e42fb3557f72454c535637cc26c7bf6"},"package":null} | |
| \ No newline at end of file | |
| +{"files":{".editorconfig":"bf047bd1da10cabb99eea666d1e57c321eba4716dccb3e4ed0e2c5fe3ca53858",".travis.yml":"0394e2adb041175457685cde5ee05ff04bdab8885fd8a62551f2ff43d9e48872","AUTHORS":"0e0ac930a68ce2f6b876126b195add177f0d3886facb9260f4d9b69f1988f0cc","Cargo.toml":"56e90cb82ec36ead07e551a28fc2455fa658fa8308c3d73f8d856d85bfcd2122","LICENSE":"44c6b5ae5ec3fe2fbc608b00e6f4896f4d2d5c7e525fcbaa3eaa3cf2f3d5a983","README.md":"e6a98ee5630b9ce1a096a2907d095454f2770e298a5b0976ab552cc53ca96cfc","src/backend/context.rs":"33d9fdf1504fe1ae43d301e288daf6eaeabeb47aa0ef86efa135c6d984425fc4","src/backend/cork_state.rs":"4a0f1afc7d9f333dac89218cc56d7d32fbffb487cd48c1c9a4e03d79cb3b5e28","src/backend/intern.rs":"374a9a3bd79fddc47739dda1dbfc5929aea5a91946794fe65fba3c8d130fbda9","src/backend/mod.rs":"06ce9250865abf0ea461f215b128470636d072a6776821efef3caf5a7b992fb9","src/backend/stream.rs":"07a29b8086774123e22dff2252a5cd2ee4c337a55e72beee1d60c4164b75280e","src/capi.rs":"b2c1be8128cadd36caa65c80950440f9d6f2aa0c24cc7bae6a9eaf6347ac454d","src/lib.rs":"7282560d84b134b09acfd8d6282600982e42fb3557f72454c535637cc26c7bf6"},"package":null} | |
| \ No newline at end of file | |
| -- | |
| 2.32.0 | |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment