[WIP] [CI] Fix unsoundess #49
Merged
Conversation
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
This mainly fix issue #41, that causes crashes when a `AudioStream` was drop. That happen because the `AudioStream` was not closed on Drop, but was deleted, causing a use-after-free by the not closed `onDataCallback` thread. Also, the method `AudioStreamBuilder::open_stream` was using the deprecated method `openStream(AudioStream*)`, that do not allow deleting the `AudioStream` safely. Replaced it by `openStream(shared_ptr<AudioStream>)`. The deprecated function allowed a use-after-free by the `onErrorCallback` thread. Also, as noted by issue #45, the bindings for `AudioStream::close()` was wrongly bound to the concrete implementation of the base class, instead of calling the virtual method. Also note that currently there is no safe way to delete the `onErrorCallback` of a `AudioStream` in oboe (see google/oboe#1610), so instead the current implementation leaks the callback on drop. Also, remove some unsound `Drop` implementations and replace them by explicit unsafe delete methods.
katyo
force-pushed
the
fix-unsoundess
branch
3 times, most recently
from
0cf4345
to
0dac6eb
Compare
katyo
force-pushed
the
fix-unsoundess
branch
from
0dac6eb
to
06759f5
Compare
|
I think removing the See: https://github.com/katyo/oboe-rs/runs/8171082948?check_suite_focus=true#step:10:563 |
This allows updating glutin to 0.29, making the oboe-demo work on Android without using a patched version of glutin. Could have updated to 0.30, but that would imply in fixing breaking changes.
This class were already deprecated in oboe 1.5.0, and was splitted in AudioStreamErrorCallback and AudioStreamDataCallback. Removing the use of this class is one step in using the methods setErrorCallback and setDataCallback introduced in the last version, that will allow fixing the workaround were the callback is leaked.
The previous API for setErrorCallback didn't allow soundly deleting the errorCallback, which led oboe-rs to leak the callback. The new API receives holds a shared_ptr, fixing the issue.
Before, the code in Rust side were resposible of freeing the callback. But now that oboe's API receives a shared_ptr, that does automatic memory management, we no longer need to hold the callback in the Rust side.
katyo
force-pushed
the
fix-unsoundess
branch
3 times, most recently
from
cd393f2
to
bb4f893
Compare
|
@katyo I think CI is failing because your |
- Update outdated actions - Remove deprecated ::set-output commands in favor of using $GITHUB_OUTPUT - Add apk signing keystore to repository - Generate temporary apk signing keystore if no password provided via secrets
So we avoids unnecessary allocation when creating stream via builder.
|
@Rodrigodd Course, I added keystore to repo. |
|
@Rodrigodd Could you help me test this pr? |
|
@katyo The demo appears to work okay, but one of my projects is crashing. I will investigate it, and try to find out where is the problem. |
|
@katyo The problem was that I was actually running the project without the patch. With the patch applied everything is working fine. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.