The default bug view has changed. See this FAQ.

Implement AudioBuffer

RESOLVED FIXED in mozilla18

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla18
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 663589 [details] [diff] [review]
Patch (v1)

Boris, please take extra care in reviewing the JSObject* related parts.  I had a lengthy discussion with folks on IRC to figure out how to do this, and I think I got it right, but I would like you to treat this patch as though it was written by a monkey.  Or a parrot.  Or something along those levels of intelligence.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #663589 - Flags: review?(bzbarsky)
Comment on attachment 663589 [details] [diff] [review]
Patch (v1)

I actually have no idea what this HOLD_JS_OBJECT/DROP_JS_OBJECTS stuff is about (and e.g. why XHR doesn't need it!).

Olli, can you look over that part?

Looking at the rest now.
Attachment #663589 - Flags: review?(bugs)
Comment on attachment 663589 [details] [diff] [review]
Patch (v1)

>diff --git a/content/media/webaudio/AudioBuffer.cpp b/content/media/webaudio/AudioBuffer.cpp
>+AudioBuffer::InitializeBuffers(uint32_t aNumberOfChannels, JSContext* aJSContext)
>+    memset(JS_GetFloat32ArrayData(array, aJSContext), 0, mLength * sizeof(float));

That's been done already: creating a new typed array with no data pointer pre-initializes the data to 0.  Wish they'd document that in the API, of course....

>+AudioBuffer::GetChannelData(JSContext* aJSContext, uint32_t aChannel,
>+    aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);

NS_ERROR_DOM_INDEX_SIZE_ERR might be better here.

>+++ b/content/media/webaudio/AudioBuffer.h
>+  nsTArray<JSObject*> mChannels;

That's infallible.  You want a FallibleTAray<JSObject*>, I think.

>+++ b/dom/bindings/Bindings.conf
> 'mozAudioContext': {
....
>+'AudioBuffer' : {

AudioBuffer comes before AudioContext in alphabetical order.

>+++ b/dom/webidl/AudioContext.webidl
>+    AudioBuffer createBuffer(unsigned long numberOfChannels, unsigned long length, float sampleRate);

May want to annotate this with [Creator].  Maybe.  At some point we might actually optimize those a bit more (e.g. not bother looking for an existing wrapper when wrapping to JS).

r=me with that, modulo the HOLD/DROP thing.
Attachment #663589 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #2)
> I actually have no idea what this HOLD_JS_OBJECT/DROP_JS_OBJECTS stuff is
> about (and e.g. why XHR doesn't need it!).

HOLD_JS_OBJECT tells the JS GC that this is a C++ object that holds pointers to JS, so the GC should trace it, using the NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN method.  If you fail to do this when you should, then the GC may collect the JS object the C++ object is holding onto, leading to use-after-free when the C++ object tries to touch its JS field.

DROP_JS_OBJECT indicates that this C++ object no longer holds JS pointers. This is mostly important when the C++ object is about to go away, because otherwise the GC will end up trying to examine a dead object when it runs, again causing a use-after-free problem.

This whole setup is a bit delicate considering how badly things can go if it is messed up. Fortunately, a lot of the time (but not all of the time!) these errors will show up quickly in debug builds. We really need to come up with a better way to declare these classes, as we get these problems far too often.

XHR does nsContentUtils::PreserveWrapper() in RootJSResultObjects(), which does a HoldJSObjects. I think it does its drop in the UNLINK_PRESERVED_WRAPPER_BLAH_BLAH gunk.
Generally speaking, every object should have a HOLD by the time it is starting to hold JS objects, and a DROP by the time it is going away, so they should match up. In your case, NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER actually does a DROP, so you don't need an explicit one.

I don't think I see anywhere you explicitly do a nsContentUtils::PreserveWrapper(), so I'm not sure that a hold is ever happening. I may be confused, though.
OK.  So for XHR it's depending on the preserve call meaning that later when it unlinks the preserved wrapper it will drop.  That works, I guess.  It also looks like XHR assumes that Unlink is always called before the destructor?  Otherwise it looks like it can go away without doing the drop...

Given that, I think this patch is wrong in that it does the NS_DROP_JS_OBJECTS in unlink, but I don't think it ever does the HOLD part.  It should presumably do that up front in InitializeBuffers before it starts allocating objects.  Also, we should probably drop in the destructor too, because I'm pretty sure AudioBuffers can be destroyed without unlink.  Andrew, is DROP_JS_OBJECT idempotent?

> NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER actually does a DROP

Only if you actually have a preserved wrapper.  Which this class may or may not.

I suppose one option is to call PreserveWrapper in InitializeBuffers and then nix the DROP from unlink.  But my question about unlink vs destructor ordering still stands.
And boy howdy should we get some of this into code comments in nsContentUtils... ;)
Yeah, I really need to document this somewhere. I'll put that on my to-do list.

> But my question about unlink vs destructor ordering still stands.

Once something has a preserved wrapper, only the cycle collector can kill it, so it will always be unlinked before it is destroyed.

Comment 9

5 years ago
If that's truly the case, we should file a bug against all of the IndexedDB code that conditionally does DROP_JS_OBJECTS in the destructor if unlinking hasn't happened.
> Once something has a preserved wrapper, only the cycle collector can kill it

OK, right, so that's why XHR works.  Places that directly HOLD instead of calling PreserveWrapper() would still need to do manual DROP in destructor...

In any case, it sounds like the easy way forward for this bug is to PreserveWrapper in InitializeBuffers, assuming it's safe to call PreserveWrapper before you even have a wrapper (and in situations in which you might never get one at all, in fact!).
Comment on attachment 663589 [details] [diff] [review]
Patch (v1)

I don't see what keeps Float32Array objects alive.
Shouldn't you call NS_HOLD_JS_OBJECTS in AudioBuffer::InitializeBuffers
and _DROP in dtor if unlink mChannels still has non-null values.
Attachment #663589 - Flags: review?(bugs) → review-
Might be good to have a test which create AudioBuffer, keeps js ref to it, 
calls gc and then uses channels.
(Assignee)

Comment 13

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #3)
> Comment on attachment 663589 [details] [diff] [review]
> Patch (v1)
> 
> >diff --git a/content/media/webaudio/AudioBuffer.cpp b/content/media/webaudio/AudioBuffer.cpp
> >+AudioBuffer::InitializeBuffers(uint32_t aNumberOfChannels, JSContext* aJSContext)
> >+    memset(JS_GetFloat32ArrayData(array, aJSContext), 0, mLength * sizeof(float));
> 
> That's been done already: creating a new typed array with no data pointer
> pre-initializes the data to 0.  Wish they'd document that in the API, of
> course....

OK, I'll take this out, and will add a comment to jsfriendapi.h instead.  :-)

> >+AudioBuffer::GetChannelData(JSContext* aJSContext, uint32_t aChannel,
> >+    aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> 
> NS_ERROR_DOM_INDEX_SIZE_ERR might be better here.

Well, I used DOM_SYNTAX_ERR because that's what WebKit does.  We need to spec the exact error codes at some point, but for now I just want to throw what WebKit does to be able to make some progress...  :/

> >+++ b/content/media/webaudio/AudioBuffer.h
> >+  nsTArray<JSObject*> mChannels;
> 
> That's infallible.  You want a FallibleTAray<JSObject*>, I think.

Ouch! :(

> >+++ b/dom/bindings/Bindings.conf
> > 'mozAudioContext': {
> ....
> >+'AudioBuffer' : {
> 
> AudioBuffer comes before AudioContext in alphabetical order.
> 
> >+++ b/dom/webidl/AudioContext.webidl
> >+    AudioBuffer createBuffer(unsigned long numberOfChannels, unsigned long length, float sampleRate);
> 
> May want to annotate this with [Creator].  Maybe.  At some point we might
> actually optimize those a bit more (e.g. not bother looking for an existing
> wrapper when wrapping to JS).

Will do.  I've also fixed the HOLD_JS_OBJECTS problem.
(Assignee)

Comment 14

5 years ago
Created attachment 664151 [details] [diff] [review]
Patch (v2)
Attachment #663589 - Attachment is obsolete: true
Attachment #664151 - Flags: review?(bugs)
Comment on attachment 664151 [details] [diff] [review]
Patch (v2)

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(AudioBuffer)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mContext)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
>+  NS_DROP_JS_OBJECTS(tmp, AudioBuffer);
>+  for (uint32_t i = 0; i < tmp->mChannels.Length(); ++i) {
>+    tmp->mChannels[i] = nullptr;
>+  }
>+  tmp->mChannels.Clear();
No need to set values to null if you're going to Clear the whole array anyway.

>+AudioBuffer::InitializeBuffers(uint32_t aNumberOfChannels, JSContext* aJSContext)
>+{
>+  NS_HOLD_JS_OBJECTS(this, AudioBuffer);
Call this after SetCapacity has succeeded.

So we may end up putting the object to JSObjectholders twice and removed twice too.
But that should be ok, since it is a hashtable, and if there are two removals they happen in
a row in unlink.
Attachment #664151 - Flags: review?(bugs) → review+
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a255d821707
Flags: in-testsuite+
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/4a255d821707
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(In reply to Mounir Lamouri (:mounir) from comment #17)
> https://hg.mozilla.org/mozilla-central/rev/4a255d821707

    1.72 +  for (uint32_t i = 0; i < aNumberOfChannels; ++i) {
    1.73 +    JSObject* array = JS_NewFloat32Array(aJSContext, mLength);
    1.74 +    if (!array) {
    1.75 +      return false;
    1.76 +    }
    1.77 +    mChannels.AppendElement(array);
    1.78 +  }
    1.79 +
    1.80 +  NS_HOLD_JS_OBJECTS(this, AudioBuffer);

Is this safe if we GC under JS_NewFloat32Array?
(Assignee)

Comment 19

5 years ago
(In reply to comment #18)
> (In reply to Mounir Lamouri (:mounir) from comment #17)
> > https://hg.mozilla.org/mozilla-central/rev/4a255d821707
> 
>     1.72 +  for (uint32_t i = 0; i < aNumberOfChannels; ++i) {
>     1.73 +    JSObject* array = JS_NewFloat32Array(aJSContext, mLength);
>     1.74 +    if (!array) {
>     1.75 +      return false;
>     1.76 +    }
>     1.77 +    mChannels.AppendElement(array);
>     1.78 +  }
>     1.79 +
>     1.80 +  NS_HOLD_JS_OBJECTS(this, AudioBuffer);
> 
> Is this safe if we GC under JS_NewFloat32Array?

Can that happen?  Who should I ask?
Argh, I was wrong. That is not safe. Sorry.
Put NS_HOLD_JS_OBJECTS before the array stuff.

(I wish JS API would be sane.)
(Assignee)

Comment 21

5 years ago
(In reply to comment #20)
> Argh, I was wrong. That is not safe. Sorry.
> Put NS_HOLD_JS_OBJECTS before the array stuff.

Fixed in https://hg.mozilla.org/integration/mozilla-inbound/rev/29fe3da0ea11.

> (I wish JS API would be sane.)

Don't we all? ;-)
https://hg.mozilla.org/mozilla-central/rev/29fe3da0ea11
> Can that happen? 

In general, GC can happen on any JSAPI allocation.  Since you never know when the engine will want to allocate, best to assume it happens on any JSAPI call unless you've verified otherwise.
You need to log in before you can comment on or make changes to this bug.