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

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Paul Adenot (:padenot)]

Attachment: WIP, crashes somewhere, plus I have no idea what I'm doing

Comment 1

[(no longer active)]

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?

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Paul Adenot (:padenot)]

Attachment: backtrace

Comment 4

[Paul Adenot (:padenot)]

Attachment: Keep the source ArrayBuffer to a decodeAudioData call alive until the decoding is complete. r=

Comment 5

[Paul Adenot (:padenot)]

Attachment: Compile error

If I uncomment:

> NS_IMPL_CYCLE_COLLECTION_UNLINK(mArrayBuffer)

I get this, which I don't understand.

Comment 6

[Andrew McCreight [:mccr8]]

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.

Comment 7

[Andrew McCreight [:mccr8]]

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

Comment 8

[Paul Adenot (:padenot)]

Attachment: Compile error 2

If I uncomment:

>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mArrayBuffer)

I get this compile error.

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Paul Adenot (:padenot)]

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?

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[Paul Adenot (:padenot)]

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

Comment 13

[Andrew McCreight [:mccr8]]

For some reason, the WebAudioDecodeJob is reporting a refcount of 0 to the cycle collector.  Is it being addrefed anywhere?

Comment 14

[(no longer active)]

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.

Comment 15

[Andrew McCreight [:mccr8]]

Yup, that sounds right to me.

Comment 16

[Paul Adenot (:padenot)]

Attachment: Keep the source ArrayBuffer to a decodeAudioData call alive until the decoding is complete. r=

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 17

[Andrew McCreight [:mccr8]]

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 18

[(no longer active)]

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.

Comment 20

[Paul Adenot (:padenot)]

Landed with the UNLINK and TRAVERSE macros for the callbacks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8352522283e4

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8352522283e4

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e18d6a0ab17c

Comment 23

[u279076]

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