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

Description Chris Dumez 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.
Comment 1 Chris Dumez 2020-09-15 14:40:25 PDT
Created attachment 408864 [details]
Patch
Comment 2 Chris Dumez 2020-09-15 16:08:48 PDT
Created attachment 408875 [details]
Patch
Comment 3 Chris Dumez 2020-09-15 16:35:03 PDT
The GTK/WPE error does not make much sense to me. Maybe it needs a clean build.
Comment 4 youenn fablet 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
Comment 5 youenn fablet 2020-09-15 23:57:09 PDT
retrying gtk build in case it works.
Comment 6 Adrian Perez 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.
Comment 7 Carlos Garcia Campos 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.
Comment 8 Chris Dumez 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.
Comment 9 Chris Dumez 2020-09-16 08:28:31 PDT
Created attachment 408923 [details]
Patch
Comment 10 Chris Dumez 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.
Comment 11 Chris Dumez 2020-09-16 08:48:31 PDT
Created attachment 408924 [details]
Patch
Comment 12 Chris Dumez 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.
Comment 13 Chris Dumez 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>
Comment 14 Chris Dumez 2020-09-16 09:00:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2020-09-16 09:01:26 PDT
<rdar://problem/68992445>