Bug 247751 – Return after throwing an exception when ConvolerNode.buffer rate is wrong

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: Web Audio

[Ahmad Saleem]

Hi Team,

I was going through Blink Commits and noted that it is something we can also merge:

Blink Commit - https://chromium.googlesource.com/chromium/blink/+/b06aa901cd3e0e1d1d6587d6b7c9739679c5feed

Webkit GitHub - https://github.com/WebKit/WebKit/blob/829dab614cc143c1b8c69f6b2535f44f254ad932/Source/WebCore/Modules/webaudio/ConvolverNode.cpp#L120

We also don't have return after exception.

Just wanted to raise this bug since I couldn't find the related bug. Thanks!

[Karl Dubost]

Ahmad,

it returns an exception currently
https://github.com/WebKit/WebKit/blob/06f9978e52a8408ed6c2c8296afb1e7449c2f1ee/Source/WebCore/Modules/webaudio/ConvolverNode.cpp#L120-L121

You can see it was modified here
https://github.com/WebKit/WebKit/commit/935f2c7e99aa9f6e03ca7e9449eed91235ceb865#diff-e1c0c01c871ffb6653371a1717b09d4f45b6153e6f1e783c8777a9e4a70b7a1dL127-L130


And then before it was
https://github.com/WebKit/WebKit/blame/c996d8b5e859f3dc0090d669eeed82a2370a2aa7/Source/WebCore/Modules/webaudio/ConvolverNode.cpp#L127-L130

with the return.


my interpretation: the chromium code was just throwing the exception before instead of returning it. But I'm not an expert. 

What do you think?

[Ahmad Saleem]

(In reply to Karl Dubost from comment #1)
> Ahmad,
> 
> it returns an exception currently
> https://github.com/WebKit/WebKit/blob/
> 06f9978e52a8408ed6c2c8296afb1e7449c2f1ee/Source/WebCore/Modules/webaudio/
> ConvolverNode.cpp#L120-L121
> 
> You can see it was modified here
> https://github.com/WebKit/WebKit/commit/
> 935f2c7e99aa9f6e03ca7e9449eed91235ceb865#diff-
> e1c0c01c871ffb6653371a1717b09d4f45b6153e6f1e783c8777a9e4a70b7a1dL127-L130
> 
> 
> And then before it was
> https://github.com/WebKit/WebKit/blame/
> c996d8b5e859f3dc0090d669eeed82a2370a2aa7/Source/WebCore/Modules/webaudio/
> ConvolverNode.cpp#L127-L130
> 
> with the return.
> 
> 
> my interpretation: the chromium code was just throwing the exception before
> instead of returning it. But I'm not an expert. 
> 
> What do you think?

If I understand Blink commit then they are adding return because they continued using wrong buffer rate even after throwing exception, while in Webkit code, if I look into from your links, it seems that we use to return earlier because didn't supported the feature by throwing "Not Supported" kind of error message and later when we added supported, we just had the "Exception" message now but we are not returning and continuing with wrong buffer rate but I can be wrong as well but this is my interpretation.
Do you anyone who might be familiar with this code to say whether we need to do anything like early return for this or do nothing because Webkit can handle wrong buffer rate smartly as it is.

[Karl Dubost]

Maybe Darin knows.

[Chris Dumez]

If you look at the test that Blink added in LayoutTests/webaudio/dom-exceptions.html, you'll see that we also have it:
```
      audit.define('convolver', (task, should) => {
        // Convolver buffer rate must match context rate. Create on offline
        // context so we specify the context rate exactly, in case the test is
        // run on platforms with different HW sample rates.
        let oc;

        should(
            () => oc = new OfflineAudioContext(1, 44100, 44100),
            'oc = new OfflineAudioContext(1, 44100, 44100)')
            .notThrow();
        should(() => conv = oc.createConvolver(), 'conv = oc.createConvolver()')
            .notThrow();
        should(() => conv.buffer = {}, 'conv.buffer = {}').throw();
        should(
            () => conv.buffer = oc.createBuffer(1, 100, 22050),
            'conv.buffer = oc.createBuffer(1, 100, 22050)')
            .throw();
        // conv.buffer should be unchanged (null) because the above failed.
        should(conv.buffer, 'conv.buffer').beEqualTo(null); // THIS CHECK HERE

        task.done();
      });
```

and that we are passing it:
```
PASS > [convolver]
PASS   oc = new OfflineAudioContext(1, 44100, 44100) did not throw an exception.
PASS   conv = oc.createConvolver() did not throw an exception.
PASS   conv.buffer = {} threw TypeError: "The ConvolverNode.buffer attribute must be an instance of AudioBuffer".
PASS   conv.buffer = oc.createBuffer(1, 100, 22050) threw NotSupportedError: "Buffer sample rate does not match the context's sample rate".
PASS   conv.buffer is equal to null.
PASS < [convolver] All assertions passed. (total 5 assertions)
```