Closed Bug 914033 Opened 7 years ago Closed 7 years ago

OfflineAudioContexts that don't startRendering() leak until their window is done

Categories

(Core :: Web Audio, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: karlt, Assigned: karlt)

References

Details

Attachments

(6 files)

If a source is started, it adds a self reference, but these are not removed on offline contexts that don't run before their external references are removed, until the window is done.

AudioDestinationNode or AudioContext needs to take ownership of (what are
currently) source self references and report them to the cycle collector until
startRendering() is called on the context.
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Depends on: 916422
Depends on: 910171
This is proof of context using OscillatorNode and the testcase here.
I'll do the same for other playing references.

Depends on attachment 805095 [details] [diff] [review].
Attachment #805098 - Flags: review?(ehsan)
The hashtable use case here is more a set than a hashtable, but
other clients can add ImplCycleCollectionTraverse for their EntryTypes.
Attachment #805101 - Flags: review?(khuey)
https://tbpl.mozilla.org/?tree=Try&rev=f2720a1add3b
https://tbpl.mozilla.org/?tree=Try&rev=f48b8d4e053c

(In reply to Karl Tomlinson (:karlt) from comment #1)
> This is proof of ...

concept ...
Blocks: 916680
Comment on attachment 805098 [details] [diff] [review]
use AudioContext::RegisterActiveNode() in OscillatorNode

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

::: content/media/webaudio/AudioContext.cpp
@@ -558,5 @@
> -  nsTArray<OscillatorNode*> oscNodes;
> -  GetHashtableElements(mOscillatorNodes, oscNodes);
> -  for (uint32_t i = 0; i < oscNodes.Length(); ++i) {
> -    ErrorResult rv;
> -    oscNodes[i]->Stop(0.0, rv);

So, with this patch, I think we won't call Stop() on oscillator nodes when the context shuts down.  That seems wrong.
Attachment #805098 - Flags: review?(ehsan) → review-
Comment on attachment 805103 [details] [diff] [review]
add active nodes to CC traversal of AudioContext when the context is not rendering

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

::: content/media/webaudio/AudioContext.cpp
@@ +50,5 @@
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDestination)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mListener)
> +  if (!tmp->mIsStarted) {
> +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mActiveNodes)
> +  }

This won't work for non-offline contexts, right?
Attachment #805103 - Flags: review?(ehsan) → review-
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> Comment on attachment 805098 [details] [diff] [review]
> use AudioContext::RegisterActiveNode() in OscillatorNode

> ::: content/media/webaudio/AudioContext.cpp
> @@ -558,5 @@
> > -  nsTArray<OscillatorNode*> oscNodes;
> > -  GetHashtableElements(mOscillatorNodes, oscNodes);
> > -  for (uint32_t i = 0; i < oscNodes.Length(); ++i) {
> > -    ErrorResult rv;
> > -    oscNodes[i]->Stop(0.0, rv);
> 
> So, with this patch, I think we won't call Stop() on oscillator nodes when
> the context shuts down.  That seems wrong.

Yes, we don't call OscillatorNode::Stop().

OscillatorNode::Stop() only tells OscillatorNodeEngine to set mStop.  mStop is used only in ProduceAudioBlock().

AudioContext::Shutdown() calls Suspend().

Suspend() was added to "Pause Web Audio streams when we go into the bfcache"
(bug 815492).

If the streams are suspended, then I expect that ProduceAudioBlock() will not
be called.

The streams are already stopped so there is no need to tell the engine to stop.

Am I missing something?
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> @@ +50,5 @@
> > +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDestination)
> > +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mListener)
> > +  if (!tmp->mIsStarted) {
> > +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mActiveNodes)
> > +  }
> 
> This won't work for non-offline contexts, right?

This is fixing a bug that applies only to offline contexts.  See comment 0.

Online contexts are started when they are created; their sources will play and should not be collected until they have stopped (or the window goes away).
mActiveNodes won't be traversed for online contexts because mIsStarted is always true.

Sources in offline contexts won't play unless startRendering() is called on the context.  If the last reference to the context is removed before startRendering() is called, then startRendering() can't be called; the context can be collected.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> > -  nsTArray<OscillatorNode*> oscNodes;
> > -  GetHashtableElements(mOscillatorNodes, oscNodes);
> > -  for (uint32_t i = 0; i < oscNodes.Length(); ++i) {
> > -    ErrorResult rv;
> > -    oscNodes[i]->Stop(0.0, rv);
> 
> So, with this patch, I think we won't call Stop() on oscillator nodes when
> the context shuts down.  That seems wrong.

You omitted the associated comment from the quote:

>-  // Stop all Oscillator nodes to make sure they release their
>-  // playing reference.

Stop() was called only to release the reference.
Comment on attachment 805103 [details] [diff] [review]
add active nodes to CC traversal of AudioContext when the context is not rendering

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

(In reply to Karl Tomlinson (:karlt) from comment #8)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> > @@ +50,5 @@
> > > +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDestination)
> > > +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mListener)
> > > +  if (!tmp->mIsStarted) {
> > > +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mActiveNodes)
> > > +  }
> > 
> > This won't work for non-offline contexts, right?
> 
> This is fixing a bug that applies only to offline contexts.  See comment 0.
> 
> Online contexts are started when they are created; their sources will play
> and should not be collected until they have stopped (or the window goes
> away).
> mActiveNodes won't be traversed for online contexts because mIsStarted is
> always true.

Sorry, this last bit was what I was missing, can you please add a comment mentioning that?  Or even better a MOZ_ASSERT asserting that?  r=me with that.
Attachment #805103 - Flags: review- → review+
(In reply to Karl Tomlinson (:karlt) from comment #7)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> > Comment on attachment 805098 [details] [diff] [review]
> > use AudioContext::RegisterActiveNode() in OscillatorNode
> 
> > ::: content/media/webaudio/AudioContext.cpp
> > @@ -558,5 @@
> > > -  nsTArray<OscillatorNode*> oscNodes;
> > > -  GetHashtableElements(mOscillatorNodes, oscNodes);
> > > -  for (uint32_t i = 0; i < oscNodes.Length(); ++i) {
> > > -    ErrorResult rv;
> > > -    oscNodes[i]->Stop(0.0, rv);
> > 
> > So, with this patch, I think we won't call Stop() on oscillator nodes when
> > the context shuts down.  That seems wrong.
> 
> Yes, we don't call OscillatorNode::Stop().
> 
> OscillatorNode::Stop() only tells OscillatorNodeEngine to set mStop.  mStop
> is used only in ProduceAudioBlock().
> 
> AudioContext::Shutdown() calls Suspend().
> 
> Suspend() was added to "Pause Web Audio streams when we go into the bfcache"
> (bug 815492).
> 
> If the streams are suspended, then I expect that ProduceAudioBlock() will not
> be called.
> 
> The streams are already stopped so there is no need to tell the engine to
> stop.
> 
> Am I missing something?

No, you're right.  My mistake.  Sorry!
Flags: needinfo?(ehsan)
Attachment #805098 - Flags: review- → review+
Comment on attachment 805101 [details] [diff] [review]
hashtable.cc.helpers

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

::: xpcom/glue/nsTHashtable.h
@@ +505,5 @@
> +
> +  nsCycleCollectionTraversalCallback& mCallback;
> +  const char* mName;
> +  uint32_t mFlags;
> +

nit: extra newline

@@ +513,5 @@
> +PLDHashOperator
> +ImplCycleCollectionTraverse_EnumFunc(EntryType *aEntry,
> +                                     void* aUserData)
> +{
> +  nsTHashtableCCTraversalData* userData =

auto userData = ...
Attachment #805101 - Flags: review?(khuey) → review+
Blocks: 918221
Attachment #807592 - Attachment description: buffersource.active → use AudioContext::RegisterActiveNode() in AudioBufferSourceNode
Attachment #807591 - Flags: review?(ehsan) → review+
Attachment #807592 - Flags: review?(ehsan) → review+
You need to log in before you can comment on or make changes to this bug.