When calling `decodeAudioData` on a detached `ArrayBuffer`, throw.

RESOLVED FIXED in Firefox 54

Status

()

Core
Web Audio
P3
normal
Rank:
35
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: padenot, Assigned: beekill, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla54
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

We kind of do this already, but we could throw much earlier.

Spec changes at https://github.com/webaudio/web-audio-api/commit/2ed34b9e2073587d67ecd32ce420b9e6ae23c12d.
Rank: 35

Updated

2 years ago
Mentor: dminor
Keywords: good-first-bug
(Assignee)

Comment 1

a year ago
Hi :dminor, I'm relatively new to the community and would like to work on this bug. 
I have set up my environment and have finished my first bug. I think this could be my next bug and hope you assign it to me.
Flags: needinfo?(dminor)
Thanks for having a look at this!

If you have any questions, please let me know.
Assignee: nobody → nnn_bikiu0707
Flags: needinfo?(dminor)
(Assignee)

Comment 3

a year ago
Thanks for letting me work on this bug. 
I'm not familiar with the project. Can you show me which files should I look at?
The implementation of DecodeAudioData is here [1]. You need to find a way of detecting whether the passed in buffer is detached. There is a JavaScript implementation of this here [2] that might be helpful, but you'll need to code up a C++ version.

[1] http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/dom/media/webaudio/AudioContext.cpp#488.
[2] http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/js/src/builtin/TypedArray.js#16
(Assignee)

Comment 5

a year ago
So what I have to do are:
+ Code a function to detect whether the buffer passed in is detached
+ Use that function in DecodeAudioData [1]
+ If the buffer is detached, then throw a TypeError

Please correct me if I'm wrong. 

[1] http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/dom/media/webaudio/AudioContext.cpp#488.
(In reply to Bao Quan [:beekill95] from comment #5)
> So what I have to do are:
> + Code a function to detect whether the buffer passed in is detached
> + Use that function in DecodeAudioData [1]
> + If the buffer is detached, then throw a TypeError
> 
> Please correct me if I'm wrong. 
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 790b2cb423ea1ecb5746722d51633caec9bab95a/dom/media/webaudio/AudioContext.
> cpp#488.

Yes, that sounds good!
(Assignee)

Comment 7

a year ago
I found that there are two ways to find out whether the buffer is detached:
 + The first one is from this ECMAScript [1]. Basically, we have to check whether the content of the passed in buffer is null or not.
 + The second one is from this TypedArray.js [2]. In this method, there are flags that we can get from the buffer. We can check if these flags contain a detached flag. I'm not so sure if I understand this method correctly because I don't know what UnsafeGetInt32FromReservedSlot do and I can't find its implementation.

So which one should I follow?

[1]: https://tc39.github.io/ecma262/#sec-isdetachedbuffer
[2]: http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/js/src/builtin/TypedArray.js#16
The reference from ECMAScript is what is specified by the WebAudio spec, so that is fine to follow. It looks like you can just get the Data from [1] and check if it is null.

It would probably make sense to write up a test case now with a detached buffer and double check that this is working as expected. You'll eventually need to write a test case for this anyway, as it doesn't appear we currently have something that covers this already written. This test might be helpful [2] but if you can come up with something simpler, go for it.

[1] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/bindings/TypedArray.h#152
[2] http://searchfox.org/mozilla-central/source/dom/media/webaudio/test/test_audioBufferSourceNodeDetached.html
(Assignee)

Comment 9

a year ago
Created attachment 8828371 [details] [diff] [review]
detach_buffer.patch

Please let me know if the patch has any issues.

I'll write a test case after I get used to writing test case in Firefox.
Attachment #8828371 - Flags: feedback?(dminor)
Comment on attachment 8828371 [details] [diff] [review]
detach_buffer.patch

Review of attachment 8828371 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, this just needs a test case.

For new tests, we try to write web platform tests were possible [1]. If it's easier, don't worry about the test framework for now and just write some JavaScript that can test your patch. I can help you modify it into a test case once we're sure things are working properly.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests

::: dom/media/webaudio/AudioContext.cpp
@@ +522,4 @@
>      return nullptr;
>    }
>  
> +  if (IsDetachedBuffer(aBuffer)) {

I wouldn't bother with adding a separate function for this, just use
if (aBuffer.Data()) {

The Mozilla coding style says to avoid using "== nullptr" when testing a pointer.
Attachment #8828371 - Flags: feedback?(dminor) → feedback+
(Assignee)

Comment 11

a year ago
Created attachment 8829082 [details] [diff] [review]
detach_buffer.patch

I removed function IsDetachedBuffer and used aBuffer.Data() instead. 

I also moved the check for detached buffer below aBuffer.ComputeLengthAndData(). Looks like the function must be called first or else aBuffer.Data() will always return nullptr.
Attachment #8828371 - Attachment is obsolete: true
Attachment #8829082 - Flags: feedback?(dminor)
(Assignee)

Comment 12

a year ago
Created attachment 8829106 [details]
test_decodeAudioOnDetachedBuffer.html

I created some tests. 
The test testDecodeAudioDataOnDetachedBuffer throws a TypeError exception but the try catch block can't catch it. I don't know what the problem is.

Let me know if something needs to change.
Attachment #8829106 - Flags: feedback?(dminor)
Sorry for the delay, I'm looking at the patches this morning.
Status: NEW → ASSIGNED
Comment on attachment 8829082 [details] [diff] [review]
detach_buffer.patch

Review of attachment 8829082 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!
Attachment #8829082 - Flags: feedback?(dminor) → feedback+
Comment on attachment 8829106 [details]
test_decodeAudioOnDetachedBuffer.html

I'd suggest making a copy of this test: http://searchfox.org/mozilla-central/source/dom/media/webaudio/test/test_bug1056032.html and modifying it to call decodeAudioBuffer twice like you did here.

If you use the promise form of decodeAudioData you should end up with your exception when the promise is rejected. I tested that quickly this morning and it seems to work fine.
Attachment #8829106 - Flags: feedback?(dminor) → feedback+
Just in case you're not aware, you'll need to add your new test to dom/media/webaudio/test/mochitest.ini in order for it to run.
(Assignee)

Comment 17

a year ago
Created attachment 8829894 [details] [diff] [review]
detach_buffer_testcase.patch

I wrote the test similar to test_bug1056032.html [1]. I retained only the second test: testDecodeAudioDataOnDetachedBuffer.

Should I add success/error callback to the first call to decodeAudioData in order to test for successful detaching?

[1]: http://searchfox.org/mozilla-central/source/dom/media/webaudio/test/test_bug1056032.html
Attachment #8829106 - Attachment is obsolete: true
Attachment #8829894 - Flags: feedback?(dminor)
Comment on attachment 8829894 [details] [diff] [review]
detach_buffer_testcase.patch

This looks good. It would be a nice idea to add a check that the buffer is in fact detached after the first call to decodeAudioBuffer.

Do you have access to the "try" server to run this in automation?
Attachment #8829894 - Flags: feedback?(dminor) → feedback+
(Assignee)

Comment 19

a year ago
I will add your suggestion to the test case.

No, I don't have access to the try server. But I'm requesting for commit access level 1 [1].

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1333793
(Assignee)

Comment 20

a year ago
Created attachment 8830373 [details] [diff] [review]
detach_buffer_testcase.patch

I modified the test case like you suggested.

Do I have to submit all the patches to MozReview for you to review and then push to the "try" server?
Attachment #8829894 - Attachment is obsolete: true
Attachment #8830373 - Flags: feedback?(dminor)
Once you have your level 1 access (I just vouched for you) you can use this to get your try syntax: https://mozilla-releng.net/trychooser/. If you use the use the mach command, like "./mach try -b do -p all -u all -t none" you can push to try without using MozReview. If you want to use MozReview, that's fine, it's up to you.

Because I'm not yet a Web Audio peer, you will need to get review from someone else besides me prior to landing. Please ask for review from both me and :padenot. Thanks!
Comment on attachment 8830373 [details] [diff] [review]
detach_buffer_testcase.patch

Review of attachment 8830373 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: dom/media/webaudio/test/mochitest.ini
@@ +112,4 @@
>  [test_currentTime.html]
>  [test_decodeMultichannel.html]
>  [test_decodeAudioDataPromise.html]
> +[test_decodeAudioDataOnDetachedBuffer.html]

Please fix this list to be in alphabetical order.

::: dom/media/webaudio/test/test_decodeAudioDataOnDetachedBuffer.html
@@ +15,5 @@
> +
> +  // make the buffer detached
> +  context.decodeAudioData(buffer);
> +
> +  // check that the buffer is detached

I think you can just do:

is(buffer.data, null, "Buffer should be detached")

here. I wouldn't worry about ending the test early, this shouldn't ever fail, and if it does, the little bit of time spent on the second decode isn't that big of a deal.

@@ +30,5 @@
> +  context.decodeAudioData(buffer).then(function(b) {
> +    ok(false, "We should not be able to decode the detached buffer but we do");
> +    SimpleTest.finish();
> +  }, function(r) {
> +    if (r.message === "Argument of AudioContext.decodeAudioData can't be a detached buffer") {

In this case, I think you can just do:

is(r.message, "Argument of AudioContext.decodeAudioData can't be a detached buffer", "Incorrect message");

Please also add a check that it is a TypeError.

@@ +35,5 @@
> +      ok(true, "We can't decode the buffer because it's detached");
> +    } else {
> +      ok(false, "We can't decode the buffer because of something else");
> +    }
> +    

Please remove extra whitespace.
Attachment #8830373 - Flags: feedback?(dminor) → feedback+
(Assignee)

Comment 23

a year ago
Created attachment 8830628 [details] [diff] [review]
detach_buffer_testcase.patch

The buffer.data yields undefined. So I use buffer.byteLength and compare with 0 instead. [1] states that if the buffer is detached then byteLength will be 0.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer/byteLength
Attachment #8830373 - Attachment is obsolete: true
Attachment #8830628 - Flags: review?(padenot)
Attachment #8830628 - Flags: review?(dminor)
(Assignee)

Updated

a year ago
Attachment #8829082 - Flags: review?(padenot)
Attachment #8829082 - Flags: review?(dminor)
Comment on attachment 8830628 [details] [diff] [review]
detach_buffer_testcase.patch

Review of attachment 8830628 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me
Attachment #8830628 - Flags: review?(dminor) → review+

Updated

a year ago
Attachment #8829082 - Flags: review?(dminor) → review+
Attachment #8829082 - Flags: review?(padenot) → review+
Attachment #8830628 - Flags: review?(padenot) → review+

Comment 25

a year ago
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1176bc6973a9
Throw TypeError if calling decodeAudioData on a detached buffer. r=padenot,dminor
https://hg.mozilla.org/integration/mozilla-inbound/rev/75ec51fdcd4e
Add testcase for calling DecodeAudioData on detached buffer. r=padenot,dminor
(Assignee)

Comment 26

a year ago
Hi Dan, someone pushed my patches to the inbound server. So do I need to push to try server?
Flags: needinfo?(dminor)
No, they are good patches, I had a look and decided that they could be pushed directly (I tested them locally on my machine). No need for try server, but maybe for your next patch ?
(Assignee)

Comment 28

a year ago
Thank you all :). I'll be working on my next bug right away.
I agree with Paul, it would be a good idea to use the try server for your next patch, even if it's just to get used to it.

Thanks for your contribution!

Updated

a year ago
Flags: needinfo?(dminor)

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1176bc6973a9
https://hg.mozilla.org/mozilla-central/rev/75ec51fdcd4e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.