Bug 216584

Summary: OfflineAudioContext constructor should not throw when given a bad buffer length
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Web AudioAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, cgarcia, darin, eric.carlson, ews-watchlist, ggaren, glenn, jer.noble, philipj, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 216579, 216583, 216599    
Bug Blocks: 212611, 216587    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch none

Chris Dumez
Reported 2020-09-15 14:37:54 PDT
OfflineAudioContext constructor should not throw when given a bad buffer length. We should instead throw later on when trying to start rendering.
Attachments
Patch (10.33 KB, patch)
2020-09-15 14:40 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (16.78 KB, patch)
2020-09-15 16:08 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (16.90 KB, patch)
2020-09-16 08:28 PDT, Chris Dumez 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (17.48 KB, patch)
2020-09-16 08:48 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2020-09-15 14:40:25 PDT
Created attachment 408864 [details] Patch
Chris Dumez
Comment 2 2020-09-15 16:08:48 PDT
Created attachment 408875 [details] Patch
Chris Dumez
Comment 3 2020-09-15 16:35:03 PDT
The GTK/WPE error does not make much sense to me. Maybe it needs a clean build.
youenn fablet
Comment 4 2020-09-15 23:56:41 PDT
Comment on attachment 408875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408875&action=review > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:44 > +OfflineAudioDestinationNode::OfflineAudioDestinationNode(BaseAudioContext& context, unsigned numberOfChannels, AudioBuffer* renderTarget) could be a RefPtr<AudioBuffer>&&? > Source/WebCore/Modules/webaudio/WebKitOfflineAudioContext.cpp:39 > +inline WebKitOfflineAudioContext::WebKitOfflineAudioContext(Document& document, Ref<AudioBuffer>&& renderTarget) inline not needed
youenn fablet
Comment 5 2020-09-15 23:57:09 PDT
retrying gtk build in case it works.
Adrian Perez
Comment 6 2020-09-16 05:50:22 PDT
(In reply to Chris Dumez from comment #3) > The GTK/WPE error does not make much sense to me. Maybe it needs a clean > build. It was a build issue masked by unified builds; after updating the patch from bug #216599 it was possible for me to add this patch on top and have builds complete for both the GTK and WPE ports.
Carlos Garcia Campos
Comment 7 2020-09-16 05:56:51 PDT
Comment on attachment 408875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408875&action=review > Source/WebCore/Modules/webaudio/WebKitAudioContext.cpp:88 > +WebKitAudioContext::WebKitAudioContext(Document& document, Ref<AudioBuffer>&& renderTarget) > + : BaseAudioContext(document, renderTarget->numberOfChannels(), WTFMove(renderTarget)) I think this could be problematic, is it defined that the move happens after the ->numberOfChannels(). This has caused issues in the past with GCC. I think it would be safer if this receives the number of channels as a parameter too.
Chris Dumez
Comment 8 2020-09-16 08:03:49 PDT
(In reply to Carlos Garcia Campos from comment #7) > Comment on attachment 408875 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=408875&action=review > > > Source/WebCore/Modules/webaudio/WebKitAudioContext.cpp:88 > > +WebKitAudioContext::WebKitAudioContext(Document& document, Ref<AudioBuffer>&& renderTarget) > > + : BaseAudioContext(document, renderTarget->numberOfChannels(), WTFMove(renderTarget)) > > I think this could be problematic, is it defined that the move happens after > the ->numberOfChannels(). This has caused issues in the past with GCC. I > think it would be safer if this receives the number of channels as a > parameter too. A WTFMove() is just a cast though so it should not matter, even if the order is not guaranteed. The parameter type is an rvalue reference so the move constructor will not run until the constructor actually saves it into its data member.
Chris Dumez
Comment 9 2020-09-16 08:28:31 PDT
Created attachment 408923 [details] Patch
Chris Dumez
Comment 10 2020-09-16 08:37:06 PDT
(In reply to Adrian Perez from comment #6) > (In reply to Chris Dumez from comment #3) > > The GTK/WPE error does not make much sense to me. Maybe it needs a clean > > build. > > It was a build issue masked by unified builds; after updating the patch > from bug #216599 it was possible for me to add this patch on top and have > builds complete for both the GTK and WPE ports. It does not look like it did the trick. GTK/WPE bots still seem unhappy.
Chris Dumez
Comment 11 2020-09-16 08:48:31 PDT
Created attachment 408924 [details] Patch
Chris Dumez
Comment 12 2020-09-16 08:53:44 PDT
Comment on attachment 408924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408924&action=review > Source/WebCore/Modules/webaudio/WebKitOfflineAudioContext.h:35 > + ~WebKitOfflineAudioContext(); Looks like adding this destructor fixed the WPE build.
Chris Dumez
Comment 13 2020-09-16 09:00:25 PDT
Comment on attachment 408924 [details] Patch Clearing flags on attachment: 408924 Committed r267147: <https://trac.webkit.org/changeset/267147>
Chris Dumez
Comment 14 2020-09-16 09:00:27 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 15 2020-09-16 09:01:26 PDT
<rdar://problem/68992445>
Note You need to log in before you can comment on or make changes to this bug.