Closed Bug 891986 Opened 6 years ago Closed 6 years ago

Keep the source ArrayBuffer to a decodeAudioData call alive until the decoding is complete

Categories

(Core :: Web Audio, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox24 + fixed
firefox25 + fixed

People

(Reporter: padenot, Assigned: padenot)

References

Details

(Whiteboard: [blocking-webaudio+][qa-])

Attachments

(5 files, 2 obsolete files)

No description provided.
Attachment #773408 - Flags: feedback?(ehsan)
Comment on attachment 773408 [details] [diff] [review]
WIP, crashes somewhere, plus I have no idea what I'm doing

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

This looks good to me, I can't imagine what you're doing wrong.  Andrew, do you mind taking a look please?
Attachment #773408 - Flags: feedback?(ehsan) → feedback?(continuation)
Can you include the crash stacks?

TRAVERSE and UNLINK need to visit CCed fields, like AudioContext, but that should only cause leaks, not crashes.

Clear mArrayBuffer in unlink and the destructor, call drop in the destructor.  You aren't calling drop right now, so XPConnect will end up with a dangling pointer to the job object after it dies.  Maybe that is the crash.
Attachment #773408 - Flags: feedback?(continuation)
Attached file backtrace
Attachment #773408 - Attachment is obsolete: true
Attached file Compile error
If I uncomment:

> NS_IMPL_CYCLE_COLLECTION_UNLINK(mArrayBuffer)

I get this, which I don't understand.
That crash is because you need to clear the JS pointer before you do DROP.  That's really only to make the thing that checks if the clear has happened happy.
(In reply to Paul Adenot (:padenot) from comment #5)
> I get this, which I don't understand.
The UNLINK is templated, and the Heap<> stuff is pretty new, so there's no case for it.  We may not actually have any cases for JS stuff, come to think of it...
Attached file Compile error 2
If I uncomment:

>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mArrayBuffer)

I get this compile error.
JS pointers, like mArrayBuffer, are in TRACE, so the GC can use them, too.  Then, in TRAVERSE, you need to add the line  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS to invoke the TRACE from the TRAVERSE.  Sorry, I forgot about that part.
I don't have the crash anymore, and my testcase passes, but since I can't visit the field because it is not implemented, does that mean I leak?
Flags: needinfo?(continuation)
It should be Traversing mArrayBuffer via the Trace method, and unlinking it manually.

The leak problem I'd worry about is the other cycle-collected things that this class points to, like mContext.  All of those need to be added to TRAVERSE and UNLINK, with the macros.
Flags: needinfo?(continuation)
Attached file valgrind stack trace
So, I wanted to know if I leaked, so I fired up valgrind, and now it crashes during the collect, with this stack (only in valgrind, though, it is fine when running normally).

About the need to cycle collect all the other stuff going from the WebAudioDecodeJob, I have no idea.
For some reason, the WebAudioDecodeJob is reporting a refcount of 0 to the cycle collector.  Is it being addrefed anywhere?
Hmm, so currently WebAudioDecodeJob's lifetime is managed explicitly.  For hooking it up to CC, this object needs to be refcounted, which means that you cannot allocate it on the stack, put it in nsAutoPtr's, etc.  This means that you need to change the consumers (AudioContext::CreateBuffer/DecodeAudioData, mDecodeJobs, etc.) and then you probably need to traverse and unlink the stuff that are cycle collected (such as mContext) as well.
Yup, that sounds right to me.
I don't crash anymore, valgrind does not complain (not sure if this is a
reliable tool for CC/GC work), and my testcase passes.
Attachment #775743 - Flags: review?(continuation)
Attachment #774039 - Attachment is obsolete: true
Comment on attachment 775743 [details] [diff] [review]
Keep the source ArrayBuffer to a decodeAudioData call alive until the decoding is complete. r=

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

The CC stuff looks okay to me, aside from the callback stuff I mention below.  Ehsan should review it, too, as I don't know anything about WebAudio. :)

::: content/media/webaudio/MediaBufferDecoder.cpp
@@ +24,5 @@
>  #include "nsCxPusher.h"
>  
>  namespace mozilla {
>  
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(WebAudioDecodeJob)

I think the two callbacks also need to be traversed and unlinked, because they are some kind of WebIDL-y thing.
Attachment #775743 - Flags: review?(ehsan)
Attachment #775743 - Flags: review?(continuation)
Attachment #775743 - Flags: review+
Comment on attachment 775743 [details] [diff] [review]
Keep the source ArrayBuffer to a decodeAudioData call alive until the decoding is complete. r=

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

::: content/media/webaudio/MediaBufferDecoder.cpp
@@ +24,5 @@
>  #include "nsCxPusher.h"
>  
>  namespace mozilla {
>  
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(WebAudioDecodeJob)

Yes.
Attachment #775743 - Flags: review?(ehsan) → review+
Blocks: 886787
Whiteboard: [blocking-webaudio+]
Blocks: 889270
Duplicate of this bug: 886787
Landed with the UNLINK and TRAVERSE macros for the callbacks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8352522283e4
https://hg.mozilla.org/mozilla-central/rev/8352522283e4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: 882589
Keywords: checkin-needed
Assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.
Whiteboard: [blocking-webaudio+] → [blocking-webaudio+][qa-]
You need to log in before you can comment on or make changes to this bug.