Closed
Bug 1308434
Opened 8 years ago
Closed 7 years ago
When calling `decodeAudioData` on a detached `ArrayBuffer`, throw.
Categories
(Core :: Web Audio, defect, P3)
Core
Web Audio
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: padenot, Assigned: beekill, Mentored)
Details
(Keywords: good-first-bug)
Attachments
(2 files, 4 obsolete files)
We kind of do this already, but we could throw much earlier. Spec changes at https://github.com/webaudio/web-audio-api/commit/2ed34b9e2073587d67ecd32ce420b9e6ae23c12d.
Reporter | ||
Updated•8 years ago
|
Rank: 35
Updated•8 years ago
|
Mentor: dminor
Keywords: good-first-bug
Assignee | ||
Comment 1•7 years 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)
Comment 2•7 years ago
|
||
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•7 years 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?
Comment 4•7 years ago
|
||
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•7 years 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.
Comment 6•7 years ago
|
||
(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•7 years 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
Comment 8•7 years ago
|
||
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•7 years ago
|
||
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 10•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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)
Comment 13•7 years ago
|
||
Sorry for the delay, I'm looking at the patches this morning.
Status: NEW → ASSIGNED
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
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+
Comment 16•7 years ago
|
||
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•7 years ago
|
||
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 18•7 years ago
|
||
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•7 years 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•7 years ago
|
||
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)
Comment 21•7 years ago
|
||
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 22•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
Attachment #8829082 -
Flags: review?(padenot)
Attachment #8829082 -
Flags: review?(dminor)
Comment 24•7 years ago
|
||
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•7 years ago
|
Attachment #8829082 -
Flags: review?(dminor) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8829082 -
Flags: review?(padenot) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8830628 -
Flags: review?(padenot) → review+
Comment 25•7 years 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•7 years ago
|
||
Hi Dan, someone pushed my patches to the inbound server. So do I need to push to try server?
Flags: needinfo?(dminor)
Reporter | ||
Comment 27•7 years ago
|
||
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•7 years ago
|
||
Thank you all :). I'll be working on my next bug right away.
Comment 29•7 years ago
|
||
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•7 years ago
|
Flags: needinfo?(dminor)
Comment 30•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1176bc6973a9 https://hg.mozilla.org/mozilla-central/rev/75ec51fdcd4e
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
Description
•