-
Notifications
You must be signed in to change notification settings - Fork 29
fix(macos): avoid camera-permission deadlock when opening off the main thread #56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
fc618ea
fix(macos): avoid camera-permission deadlock when opening off the mai…
wysaid bb7f44b
test(macos): harden permission deadlock test against hangs and UAF
wysaid bd4e03c
ci(windows): fix VS builds after windows-latest moved to the VS 2026 …
wysaid d21f8d5
ci(windows): fix VS2026 shared-link test vcvars path
wysaid 7f30f19
test(playback): make GetCurrentTimeProgression robust to CI timing
wysaid dda3119
ci(windows): make VS2026 configure idempotent for build-cache hits
wysaid 150f212
fix(macos): notify condition_variable under lock; address review find…
wysaid d78dbe8
refactor(macos): drop the C++ wait helper, keep the minimal GCD fix
wysaid File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| /** | ||
| * @file ccap_apple_async.h | ||
| * @author wysaid (this@wysaid.org) | ||
| * @brief Run a callback-based asynchronous request and block until it completes, | ||
| * without bouncing the request onto the main dispatch queue. | ||
| * | ||
| * macOS callback APIs such as `AVCaptureDevice requestAccessForMediaType:` deliver | ||
| * their completion on an internal queue, not on the caller's run loop. ccap used to | ||
| * dispatch the permission request onto the main queue for non-main-thread callers, | ||
| * which deadlocks whenever nothing is servicing that queue -- e.g. a ccap::Provider | ||
| * opened from a worker thread in a process that has no CFRunLoop on its main thread | ||
| * (a Node.js / Electron addon, a head-less multi-threaded service, ...). | ||
| * | ||
| * runBlockingAsyncRequest() starts the request on the *calling* thread and blocks on a | ||
| * portable condition variable until the supplied continuation is invoked, so it is | ||
| * safe to call from any thread regardless of run-loop state. | ||
| * | ||
| * Covered by tests/test_apple_permission.cpp. | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #if defined(__APPLE__) | ||
|
|
||
| #include <condition_variable> | ||
| #include <functional> | ||
| #include <mutex> | ||
|
|
||
| namespace ccap | ||
| { | ||
|
|
||
| /** | ||
| * Invoke @p start on the current thread and block until the continuation that | ||
| * @p start receives (its `done` argument) is called. @p start may invoke `done` from | ||
| * any thread or queue. The request is never dispatched to the main queue, so this | ||
| * cannot deadlock when no run loop is servicing it. | ||
| */ | ||
| inline void runBlockingAsyncRequest(const std::function<void(const std::function<void()>& done)>& start) | ||
| { | ||
| std::mutex mutex; | ||
| std::condition_variable cv; | ||
| bool finished = false; | ||
|
|
||
| start([&mutex, &cv, &finished]() { | ||
| // Notify while holding the lock: `mutex`/`cv`/`finished` are stack-locals of the | ||
| // (possibly different) waiting thread. If we unlocked before notifying, the waiter | ||
| // could wake (e.g. spuriously), see finished == true, return, and destroy `cv` | ||
| // before notify_one() ran -- a use-after-free. Holding the lock makes the waiter | ||
| // block re-acquiring it until notify_one() has completed. | ||
| std::lock_guard<std::mutex> lock(mutex); | ||
| finished = true; | ||
| cv.notify_one(); | ||
| }); | ||
|
|
||
| std::unique_lock<std::mutex> lock(mutex); | ||
| cv.wait(lock, [&finished]() { return finished; }); | ||
| } | ||
|
|
||
| } // namespace ccap | ||
|
|
||
| #endif // __APPLE__ |
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| /** | ||
| * @file test_apple_permission.cpp | ||
| * @brief Contract test for ccap::runBlockingAsyncRequest() -- the helper that | ||
| * ProviderApple::open() delegates its camera-permission wait to on macOS. | ||
| * | ||
| * Background: open()'s permission request used to be dispatched onto the main dispatch | ||
| * queue, which deadlocks when nothing services that queue (a worker thread in a | ||
| * Node.js / Electron addon, or a head-less service). The fix extracted the | ||
| * "start an async request and block until it completes" step into | ||
| * runBlockingAsyncRequest(), which runs the request on the calling thread. | ||
| * | ||
| * Scope: this pins that helper's contract -- a blocking wait whose completion is | ||
| * delivered on another thread must finish without deadlocking or missing the signal -- | ||
| * using a *simulated* async request (a background-thread countdown standing in for | ||
| * AVCaptureDevice requestAccessForMediaType:). It deliberately does NOT drive | ||
| * open()/AVFoundation end to end: that needs a real camera and TCC state and cannot run | ||
| * deterministically in CI. The helper is the unit where the deadlock lived once the | ||
| * main-queue hop was removed, so guarding its contract is what is testable here. | ||
| * | ||
| * On non-Apple platforms this file compiles to an empty translation unit. | ||
| */ | ||
|
|
||
| #if defined(__APPLE__) | ||
|
|
||
| #include <gtest/gtest.h> | ||
|
|
||
| #include <chrono> | ||
| #include <functional> | ||
| #include <future> | ||
| #include <memory> | ||
| #include <thread> | ||
|
|
||
| #include "ccap_apple_async.h" | ||
|
|
||
| namespace | ||
| { | ||
|
|
||
| // Runs `scenario` (which performs the runBlockingAsyncRequest call under test) on a | ||
| // dedicated worker thread and reports whether it finished within `timeout`. Keeping the | ||
| // call on a worker thread -- with the watchdog on the calling thread -- means a | ||
| // regression that deadlocks fails the test with a clean timeout instead of hanging the | ||
| // whole test binary. The completion state lives on the heap and is shared with the | ||
| // worker, so a late completion after a timeout/detach can never touch freed state. | ||
| bool finishesWithinTimeout(std::function<void()> scenario, std::chrono::milliseconds timeout) | ||
| { | ||
| auto finished = std::make_shared<std::promise<void>>(); | ||
| std::future<void> future = finished->get_future(); | ||
|
|
||
| std::thread worker([scenario = std::move(scenario), finished]() { | ||
| scenario(); | ||
| finished->set_value(); | ||
| }); | ||
|
|
||
| const bool ok = future.wait_for(timeout) == std::future_status::ready; | ||
| if (ok) { | ||
| worker.join(); | ||
| } else { | ||
| worker.detach(); // never block the test process; the heap state keeps detach safe | ||
| } | ||
| return ok; | ||
| } | ||
|
|
||
| // Stand-in for AVCaptureDevice requestAccessForMediaType:completionHandler:: fires the | ||
| // completion asynchronously from a *background* thread after a short countdown, exactly | ||
| // like the real API delivers its completion off the caller's run loop. | ||
| void completeAsynchronously(const std::function<void()>& done) | ||
| { | ||
| auto completion = std::make_shared<std::function<void()>>(done); | ||
| std::thread([completion]() { | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(50)); // countdown | ||
| (*completion)(); | ||
| }).detach(); | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| // Regression: the permission wait must not deadlock when run off the main thread with | ||
| // no run loop servicing the main queue (e.g. a ccap::Provider opened from a Node.js | ||
| // addon worker thread). This hung with the old dispatch-to-main-queue implementation. | ||
| TEST(AppleCameraPermission, AsyncCompletionOffMainThreadDoesNotDeadlock) | ||
| { | ||
| EXPECT_TRUE(finishesWithinTimeout([] { ccap::runBlockingAsyncRequest(&completeAsynchronously); }, | ||
| std::chrono::seconds(5))) | ||
| << "runBlockingAsyncRequest() deadlocked -- the request was likely bounced onto " | ||
| "an unserviced main dispatch queue."; | ||
| } | ||
|
|
||
| // The completion may also fire synchronously (e.g. authorization already determined); | ||
| // the blocking wait must still observe the signal rather than miss it. | ||
| TEST(AppleCameraPermission, SynchronousCompletionDoesNotDeadlock) | ||
| { | ||
| EXPECT_TRUE(finishesWithinTimeout( | ||
| [] { ccap::runBlockingAsyncRequest([](const std::function<void()>& done) { done(); }); }, | ||
| std::chrono::seconds(5))) | ||
| << "runBlockingAsyncRequest() missed a synchronous completion."; | ||
| } | ||
|
|
||
| #endif // __APPLE__ |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pin
vcvars64.batdiscovery to VS 2026 to avoid false-positive link tests.At Line 404,
find ... | head -n1may pick a non-2026 installation (for example VS2022) when multiple VS versions are present, so this “VS2026” link test can silently validate the wrong toolchain.Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents