android: add Robolectric coverage for the AudioRecord stop-path race - #279
Open
KD6763 wants to merge 5 commits into
Open
android: add Robolectric coverage for the AudioRecord stop-path race#279KD6763 wants to merge 5 commits into
KD6763 wants to merge 5 commits into
Conversation
…read stopRecordingIfNeededImpl joined the reader thread while holding audioRecordStateLock and never stopped the AudioRecord first. Two consequences: - The reader acquires that lock at the top of every loop iteration, so a stop racing a loop boundary deadlocks against the join until its 2s timeout: the thread waits for the lock while the join waits for the thread. Observed in production as a ~2s stall on every capture stop cycle under churn. - A reader stuck inside a blocking AudioRecord.read() (stalled capture HAL, e.g. Bluetooth SCO route churn) has nothing to unblock it, so the join times out structurally. Stop the record inside the locked phase (a blocking read() returns promptly once the record leaves the recording state), detach it from the shared field so a concurrent init/start builds fresh state instead of touching a record the stopping thread may still be reading, and perform the join outside the lock. The stop entry points clear their request flags under the lock and run the join outside it; a concurrent stop that loses the race now returns cleanly instead of tripping the audioThread assert.
When the stop path's join timed out it released the AudioRecord anyway, while the abandoned reader thread could still be inside AudioRecord.read() on that record. Releasing a record under an in-flight read corrupts the platform client proxy accounting and aborts the process: releaseBuffer: mUnreleased out of range, !(stepCount:480 <= mUnreleased:0 <= mFrameCount:3840) On join timeout, transfer release responsibility to the reader through an atomic handoff slot (orphanedRecord + cleanupDone): the reader releases the record when it finally exits, and if it has already passed its cleanup point during the handoff the stop path reclaims and releases it. Exactly one side performs the release in every interleaving.
…state With the join no longer serializing everything under audioRecordStateLock, a reader that is being stopped could observe state belonging to the next recording session and corrupt it: - The loop-top snapshot re-reads the shared audioRecord field each iteration, so a reader that passed the while(keepAlive) check could adopt a successor record installed during the stop's join window, read it concurrently with the new reader, and stop it on exit. keepAlive is now re-checked against the snapshot (the stop path publishes keepAlive=false before detaching, under the same lock), and exit cleanup stops only the record this thread actually read (activeRecord), never the shared field. - The startup preamble read the shared field twice without the lock and asserted on its recording state, which now races the stop's detach+stop: taken under the lock with a keepAlive guard. - The mid-loop re-init leg could re-create a record after a stop had already finished, leaving a record behind that no stop path would ever release; the re-init, restart, and state verification now run in one critical section gated on a keepAlive re-check, and a record stopped by shutdown is no longer misreported as a start failure (which permanently disabled useAudioRecord). - A read unblocked by the stop path's own AudioRecord.stop() exits through the loop condition instead of logging a spurious read error.
Covers the stop-path contract fixed in the stop/join rework: a stop must never release the AudioRecord under a reader still inside a blocking read(); a join timeout hands release ownership to the reader (orphan handoff, released exactly once); a leaked reader confines exit cleanup to the record it was reading and never touches a successor session's record; shutdown races never misreport a start failure or permanently disable useAudioRecord; and concurrent stop callers both succeed. The AudioRecord is a Mockito fake with a deterministic state machine whose read() blocks like a stalled capture HAL; record and buffer are injected by reflection because initRecordingImpl needs a direct ByteBuffer with a backing array, which the test JVM does not provide.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #266, contributing the test harness in the tree's Robolectric setup as suggested there.
Coverage
AudioRecord.stop()instead of burning the join timeout, no leaked reader threads, record released exactly once.stop()unblocks): the stop path returns without releasing under the reader; release ownership transfers to the reader, which releases the orphan exactly once on exit.useAudioRecordnever disabled).Approach
The
AudioRecordis a Mockito fake with a deterministic state machine whoseread()blocks like a stalled capture HAL (same style as the siblingLowLatencyAudioBufferManagerTestmockingAudioTrack). The record and capture buffer are injected by reflection becauseinitRecordingImplneeds a directByteBufferwith a backing array, which exists on ART but not on the test JVM; the start/stop machinery under test runs unmodified.Verification
I don't have a GN/Chromium checkout that can run
android_sdk_junit_testslocally, so the exact file (modulo the package statement and theAndroidJUnit4import swapped toRobolectricTestRunner) was run under Robolectric 4.12.2 + Mockito 5.12 against the sameWebRtcAudioRecordcode in two configurations:NeverWantedButInvoked: audioRecord.stop()fromAudioRecordThread.runon the successor's record (the leaked-reader exit path), and leaked reader threads in the cycling tests.Based on the #266 branch; the suite needs those changes to pass.