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?

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.

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...

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?

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.

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.

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.

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.

Landed with the UNLINK and TRAVERSE macros for the callbacks. https://hg.mozilla.org/integration/mozilla-inbound/rev/8352522283e4

Assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.