[WebAudio] Handle OOM in AudioArray [chromium/src : main]
Hongchan Choi (Gerrit)
Hongchan Choi added 1 comment![]( "Open in Gerrit")
PTAL at t/b/r/m/webaudio/*
Because we cannot directly throw exceptions from the (Offline)AudioContext constructor, I’ve introduced an internal state flag. This allows us to defer and gracefully throw the exception at the end of the factory method (e.g., ::Create()).
In my view, this is the most efficient approach to handle allocation failures without requiring a large-scale refactor/redesign of all AudioContext and AudioNode constructors.
The main goal of this CL is the introduction of TryAllocate() within the memory infrastructure. The fixes to the upper layers are an additional benefit of this change, and further improvements can be done in follow-up work.
Related details
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
bool is_allocation_failed_ = false;nit: Blink Style Guide: Naming. Variable names should match the names of the getter/setter. Consider renaming 'is_allocation_failed_' to 'has_allocation_failed_' to match the getter 'HasAllocationFailed()'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wilson (Gerrit)
Michael Wilson added 7 comments![]( "Open in Gerrit")
void Initialize(unsigned number_of_channels,Is there any situation where we want to call `Initialize` instead of `TryInitialize`?
DOMExceptionCode::kNotSupportedError,I think `UnknownError` is more appropriate here: https://webidl.spec.whatwg.org/#unknownerror
"The operation failed for an unknown transient reason (e.g. out of memory)."
internal_summing_bus_ = AudioBus::Create(1, 1);Technically this could also fail. We could possibly statically allocate a fallback bus instead, which would always be available, but if we do any sort of deletion on `internal_summing_bus_` that wouldn't work.
Perhaps doing `TryCreate` with a `CHECK` afterward would be better though? Then at least we will crash early in extreme OOM situations.
DOMExceptionCode::kNotSupportedError,As above, UnknownError may be more appropriate here.
render_bus_ = AudioBus::Create(render_target->numberOfChannels(), 1);Similar comment here as in audio_node_input.cc
return nullptr;It seems like the normal AudioBus::Create can also fail. Would it be better to migrate all creation to `TryCreate` here too?
void Allocate(uint32_t length) { CHECK(TryAllocate(length)); }Is this actually used anywhere? If not, let's remove it.
Related details
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi voted and added 9 comments![]( "Open in Gerrit")
Votes added by Hongchan Choi
| Commit-Queue | +1 |
9 comments
Please note that this is a reland of crrev.com/c/7586396, plus small changes on the Blink WebAudio classes.
PTAL nuskos@ - t/b/r/p/wtf/allocator/*
PTAL haraken@ - everything else outside of t/b/r/p/audio/
Is there any situation where we want to call `Initialize` instead of `TryInitialize`?
Good catch. Removed.
I think `UnknownError` is more appropriate here: https://webidl.spec.whatwg.org/#unknownerror
"The operation failed for an unknown transient reason (e.g. out of memory)."
I'd prefer to avoid "unknown" here — we know the reason, so the error should reflect that. I’m leaning toward `NotSupportedError` for a few reasons:
1. https://www.w3.org/TR/webaudio-1.1/#AudioBuffer-constructors
- "If any of the values in options lie outside its nominal range, throw a NotSupportedError exception and abort the following steps."
2. https://tc39.es/ecma262/#sec-createbytedatablock
- "If it is impossible to create such a Data Block, throw a RangeError exception."
3. Another justification of NotSupportedError:
- When the implementation fails to allocate memory as "the requested size is not supported by the system".
Let’s stick to the AudioBuffer pattern and go with NotSupportedError.
Technically this could also fail. We could possibly statically allocate a fallback bus instead, which would always be available, but if we do any sort of deletion on `internal_summing_bus_` that wouldn't work.
Perhaps doing `TryCreate` with a `CHECK` afterward would be better though? Then at least we will crash early in extreme OOM situations.
Done
nit: Blink Style Guide: Naming. Variable names should match the names of the getter/setter. Consider renaming 'is_allocation_failed_' to 'has_allocation_failed_' to match the getter 'HasAllocationFailed()'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
As above, UnknownError may be more appropriate here.
Acknowledged
render_bus_ = AudioBus::Create(render_target->numberOfChannels(), 1);Similar comment here as in audio_node_input.cc
Done.
It seems like the normal AudioBus::Create can also fail. Would it be better to migrate all creation to `TryCreate` here too?
Done.
void Allocate(uint32_t length) { CHECK(TryAllocate(length)); }Is this actually used anywhere? If not, let's remove it.
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wilson (Gerrit)
Michael Wilson voted and added 3 comments![]( "Open in Gerrit")
Votes added by Michael Wilson
| Code-Review | +1 |
3 comments
LGTM overall, please see comment about create methods in AudioBus.
DOMExceptionCode::kNotSupportedError,Hongchan ChoiI think `UnknownError` is more appropriate here: https://webidl.spec.whatwg.org/#unknownerror
"The operation failed for an unknown transient reason (e.g. out of memory)."
I'd prefer to avoid "unknown" here — we know the reason, so the error should reflect that. I’m leaning toward `NotSupportedError` for a few reasons:
1. https://www.w3.org/TR/webaudio-1.1/#AudioBuffer-constructors
- "If any of the values in options lie outside its nominal range, throw a NotSupportedError exception and abort the following steps."2. https://tc39.es/ecma262/#sec-createbytedatablock
- "If it is impossible to create such a Data Block, throw a RangeError exception."3. Another justification of NotSupportedError:
- When the implementation fails to allocate memory as "the requested size is not supported by the system".Let’s stick to the AudioBuffer pattern and go with NotSupportedError.
Acknowledged. If it turns out to be a problem we can work with the Audio Working Group to specify it and change the error type in the future.
return nullptr;Hongchan ChoiIt seems like the normal AudioBus::Create can also fail. Would it be better to migrate all creation to `TryCreate` here too?
Done.
Sorry, I wasn't clear with my comment.
Since the normal `Create` method can actually return `nullptr`, it is effectively already a `TryCreate`. So, I think for clarity it would be better to remove the `Create` method and only use the `TryCreate` method for this class.
Related details
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi added 2 comments![]( "Open in Gerrit")
This is a re-land of crrev.com/c/7586396. The additional change is only in the Blink WebAudio (t/b/r/modules/webaudio) and already reviewed by mjwilson@, and **other changes remain the same**.
PTAL haraken@ - everything else outside of t/b/r/p/audio/*
PTAL nuskos@ - t/b/r/p/wft/allocator/*
return nullptr;Hongchan ChoiIt seems like the normal AudioBus::Create can also fail. Would it be better to migrate all creation to `TryCreate` here too?
Michael WilsonDone.
Sorry, I wasn't clear with my comment.
Since the normal `Create` method can actually return `nullptr`, it is effectively already a `TryCreate`. So, I think for clarity it would be better to remove the `Create` method and only use the `TryCreate` method for this class.
I’d prefer to replace the original create method in a follow-up CL. This change is already quite vertical, and I want to keep the scope of this CL focused to make it more manageable.
Related details
- Kentaro Hara
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kentaro Hara (Gerrit)
Kentaro Hara voted Code-Review+1![]( "Open in Gerrit")
- Hongchan Choi
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 3 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
3 comments
// Similar to Initialize, but returns false if allocation fails.nit: Initialize seems to have been removed this comment needs updating.
internal_summing_bus_ = AudioBus::TryCreate(1, 1);Are we introducing any bounds potentially errors where it assumes the size of the buffer?
I.E. is AudioBus properly spanified already so this sort of incorect mismatch (the size doesn't match RenderQuantumFrames anymore) won't become an OOB security bug? (I'd prefer crashing to a security bug).
Should we have some sort of "dummy" AudioBus if this has to be a valid pointer, but that checks if anyone accesses it?
Feel free to ignore this if you don't consider this a problem, but I wanted to raise the concern for you to consider.
render_bus_ = AudioBus::TryCreate(render_target->numberOfChannels(), 1);Similar question here about OOB potential.
Related details
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi voted and added 4 comments![]( "Open in Gerrit")
Votes added by Hongchan Choi
| Commit-Queue | +1 |
4 comments
// Similar to Initialize, but returns false if allocation fails.nit: Initialize seems to have been removed this comment needs updating.
Good catch! Done.
Are we introducing any bounds potentially errors where it assumes the size of the buffer?
I.E. is AudioBus properly spanified already so this sort of incorect mismatch (the size doesn't match RenderQuantumFrames anymore) won't become an OOB security bug? (I'd prefer crashing to a security bug).
Should we have some sort of "dummy" AudioBus if this has to be a valid pointer, but that checks if anyone accesses it?
Feel free to ignore this if you don't consider this a problem, but I wanted to raise the concern for you to consider.
Thanks for raising this important point. To address the OOB concern, I've updated the fallback to allocate a 0-frame AudioBus instead of a 1-frame one.
This keeps the pointer valid to satisfy C++ assumptions during object destruction, but ensures that any processing loop (which iterates based on the bus length) will safely and immediately exit, completely avoiding any OOB writes.
Furthermore, if this fallback is triggered, HasAllocationFailed() is set to true. This causes the context creation to immediately throw a NotSupportedError and abort. As a result, the audio thread is never started and this dummy bus is never actually accessed for processing.
render_bus_ = AudioBus::TryCreate(render_target->numberOfChannels(), 1);Similar question here about OOB potential.
Similarly with the node input above. Done.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |