914030 - AudioContexts leak until their window is done

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

Attachment: offline-audio-leak-1.html

The window holds a reference to AudioContexts, which keeps them alive past their useful life until the window is done.  I'll attach three testcases where the AudioContext is not collected before the document is unloaded.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: offline-audio-leak-2.html

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: offline-audio-leak-3.html

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: update AudioNode ownership comment

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: remove MediaBufferDecoder::Shutdown(), no longer necessary since bug 900711, as part of making AudioContext::Shutdown() idempotent

Subsequent patches will want to use AudioContext::Shutdown() from
AudioDestinationNode.cpp at OfflineAudioContext completion.  Adding another
Shutdown() caller is easiest if both callers can call without causing
problems.  The easiest way to deal with the decoder shutdown code is to rely
on the changes in bug 900711 that mean that nsIThreadPool no longer needs
explicit shutdown, because we can depend on the destructor.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: shut down an AudioDestinationNode's graph on destruction, if not already

When the window is changed to no longer keep the AudioContext alive, the
AudioDestinationNode will be unlinked before AudioContext::Shutdown() is called
if startRendering() is never called on the context.
e.g. offline-audio-leak-3.html

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: shutdown an OfflineAudioContext when it has completed

Because of bug 910171, the patch in bug 914013 is needed to stop test_delayNode from leaking with this patch.
offline-audio-leak-2.html is an example of a leak that needs this patch.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: keep a self reference on destination while offline context is rendering

This will be needed once the window no longer keeps the AudioContext alive.
test_convolverNode_mono_mono.html notices if an OfflineAudioContext is
collected while running.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: don't keep alive used AudioContexts from the window

nsGlobalWindow::FreeInnerObjects() and DisconnectEventTargetObjects() are not
called in the same situations, and I don't know whether there are any
guarantees that one is called before the other, but I think this handles the
possible scenarios anyway.

Comment 9

[Karl Tomlinson (:karlt)]

https://tbpl.mozilla.org/?tree=Try&rev=7f6a868633ae
https://tbpl.mozilla.org/?tree=Try&rev=d91e01c8db0c

Comment 10

[(no longer active)]

Comment on attachment 801389 [details] [diff] [review]
update AudioNode ownership comment

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

::: content/media/webaudio/AudioNode.h
@@ +102,5 @@
>   * real-time processing and output of this AudioNode.
>   *
>   * We track the incoming and outgoing connections to other AudioNodes.
> + * Outgoing connections have strong ownership.  Also, AudioNodes that add
> + * signal to the graph keep self references.

"add signal" is not very clear.  How about "can produce output when their input has already been exhausted" or something?

Comment 11

[(no longer active)]

Comment on attachment 801390 [details] [diff] [review]
remove MediaBufferDecoder::Shutdown(), no longer necessary since bug 900711, as part of making AudioContext::Shutdown() idempotent

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

r=me, but we can't take this patch on Aurora (since bug 900711 has not been fixed there), right?

Comment 12

[(no longer active)]

Comment on attachment 801393 [details] [diff] [review]
shut down an AudioDestinationNode's graph on destruction, if not already

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

::: content/media/MediaStreamGraph.h
@@ +1023,5 @@
>     * in main-thread stream state.
>     */
>    int64_t GetCurrentGraphUpdateIndex() { return mGraphUpdatesSent; }
> +
> +  bool IsNonRealtime();

Nit: const.

Comment 13

[(no longer active)]

Comment on attachment 801397 [details] [diff] [review]
don't keep alive used AudioContexts from the window

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

Hmm, so with this patch, if you construct a graph and have GC collect all of the nodes, it's possible for AudioContext to be destroyed before the playback is "finished".  That is not what we want here.  Can we have nsGlobalWindow hold a strong ref to AudioContext objects, and just break those references when we know that we're done with the context (such as when we receive the rendering callback on an offline context, for example.)

::: content/media/webaudio/AudioContext.cpp
@@ +66,5 @@
>  }
>  
>  AudioContext::~AudioContext()
>  {
> +  if (nsPIDOMWindow* window = GetOwner()) {

Please do not assign in conditional statements.  Also, GetOwner should never return null, so perhaps it's best to MOZ_ASSERT it here.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 801397 [details] [diff] [review]
don't keep alive used AudioContexts from the window

What ehsan said...

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to Boris Zbarsky [:bz] from comment #14)
> What ehsan said...

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #13)
> Please do not assign in conditional statements.

Can you help me understand, please, by telling me the reason for your request?
This is initialization, not assignment.

> GetOwner should never
> return null, so perhaps it's best to MOZ_ASSERT it here.

It can be null, when opening offline-audio-leak-3.html, for example, and closing before the context is collected.

AudioContext is an nsDOMEventTargetHelper.
nsDOMEventTargetHelper::DisconnectFromOwner() must be called to set the
mOwnerWindow to null before the nsDOMEventTargetHelper destructor, if it is
ever called.  Is something meant to ensure that this won't be called before
the derived class destructor, or why should DisconnectFromOwner() never be
called for an AudioContext?

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 801397 [details] [diff] [review]
don't keep alive used AudioContexts from the window

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #13)
> Hmm, so with this patch, if you construct a graph and have GC collect all of
> the nodes, it's possible for AudioContext to be destroyed before the
> playback is "finished".  That is not what we want here.

That is not what we want, but that won't happen.
GC/CC won't collect the context if it is playing because the "node producing sound on its output even when it has no input" will keep a self reference and the node keeps a reference to the context.

> Can we have
> nsGlobalWindow hold a strong ref to AudioContext objects, and just break
> those references when we know that we're done with the context (such as when
> we receive the rendering callback on an offline context, for example.)

That is not necessary, and that defeats the cycle collection on which we are depending to detect when an online AudioContext is done.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I meant the part about contexts that are still playing.

Comment 18

[Karl Tomlinson (:karlt)]

Attachment: offline-audio-leak-5.html

This is a better example for comment 5 than 3.html, because this ensures there is a destination and a cycle.  (4.html is bug 914033.)

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 801397 [details] [diff] [review]
don't keep alive used AudioContexts from the window

Is comment 16 clear?

Comment 20

[Boris Zbarsky [:bzbarsky]]

I don't know enough aboutthe AudioNode ownership model to tell.  You really want Ehsan here.

Comment 21

[(no longer active)]

(In reply to Karl Tomlinson (:karlt) from comment #15)
> (In reply to Boris Zbarsky [:bz] from comment #14)
> > What ehsan said...
> 
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #13)
> > Please do not assign in conditional statements.
> 
> Can you help me understand, please, by telling me the reason for your
> request?
> This is initialization, not assignment.

I think this code pattern is very confusing.  Instead of:

if (type name = value) { }

I usually prefer:

type name = value;
if (name) { }

When reading code, it's easy to confuse the first pattern with an equality check.

> > GetOwner should never
> > return null, so perhaps it's best to MOZ_ASSERT it here.
> 
> It can be null, when opening offline-audio-leak-3.html, for example, and
> closing before the context is collected.
> 
> AudioContext is an nsDOMEventTargetHelper.
> nsDOMEventTargetHelper::DisconnectFromOwner() must be called to set the
> mOwnerWindow to null before the nsDOMEventTargetHelper destructor, if it is
> ever called.  Is something meant to ensure that this won't be called before
> the derived class destructor, or why should DisconnectFromOwner() never be
> called for an AudioContext?

Nevermind, my mistake.  You're right!

Comment 22

[Karl Tomlinson (:karlt)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #21)
> When reading code, it's easy to confuse the first pattern with an equality
> check.

OK, thanks.  I can change that.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #11)
> r=me, but we can't take this patch on Aurora (since bug 900711 has not been
> fixed there), right?

Yes, not without the patch in bug 900711.  If we don't get approval there, then we'll have to do something else.

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 801397 [details] [diff] [review]
don't keep alive used AudioContexts from the window

(In reply to Karl Tomlinson (:karlt) from comment #8)
> nsGlobalWindow::FreeInnerObjects() and DisconnectEventTargetObjects() are not
> called in the same situations, and I don't know whether there are any
> guarantees that one is called before the other, but I think this handles the
> possible scenarios anyway.

Not really.  This patch is depending on FreeInnerObjects() being called before DisconnectEventTargetObjects(), and I'm failing to prove that is always what happens.

Perhaps AudioContext needs to override DisconnectFromOwner().
Please let me know if you have better ideas.

Comment 24

[(no longer active)]

Comment on attachment 801397 [details] [diff] [review]
don't keep alive used AudioContexts from the window

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

Hmm, I think you're right here, Karl.  It's very sad that the AudioNode lifetime has gotten to the point that I'm seemingly unable to understand it properly myself!

Comment 25

[(no longer active)]

(In reply to Karl Tomlinson (:karlt) from comment #23)
> Comment on attachment 801397 [details] [diff] [review]
> don't keep alive used AudioContexts from the window
> 
> (In reply to Karl Tomlinson (:karlt) from comment #8)
> > nsGlobalWindow::FreeInnerObjects() and DisconnectEventTargetObjects() are not
> > called in the same situations, and I don't know whether there are any
> > guarantees that one is called before the other, but I think this handles the
> > possible scenarios anyway.
> 
> Not really.  This patch is depending on FreeInnerObjects() being called
> before DisconnectEventTargetObjects(), and I'm failing to prove that is
> always what happens.
> 
> Perhaps AudioContext needs to override DisconnectFromOwner().
> Please let me know if you have better ideas.

Hmm, that's a question for Boris, I think.

Comment 26

[(no longer active)]

Also, Christoph, can you please take this patch for a fun ride inside the fuzzer, and let us know if it doesn't behave?  :-)

Comment 27

[Christoph Diehl [:posidron]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #26)
> Also, Christoph, can you please take this patch for a fun ride inside the
> fuzzer, and let us know if it doesn't behave?  :-)

Of course.

Comment 28

[Christoph Diehl [:posidron]]

Ehsan, nothing is popping up here.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Bobby, do you happen to know whether the ordering guarantee comment 25 mentions holds?  I can't prove or disprove it so far.  :(

Comment 30

[Karl Tomlinson (:karlt)]

Thanks for looking, Boris.

FWIW, I tried calling AudioContext::Shutdown() *only* from AudioContext::DisconnectFromOwner(), but DisconnectFromOwner() did not get called for a context in an old document when loading a new document in the same tab, resulting in leaks.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> but DisconnectFromOwner() did not get called for a context in an old document when
> loading a new document in the same tab

Is that because the old window went into bfcache?

Comment 32

[Karl Tomlinson (:karlt)]

I assume so.  I'd expect it to be in bfcache at that stage.
~WindowStateHolder only calls FreeInnerObjects().

DetachFromDocShell() also calls FreeInnerObjects() for the list of inner windows, but CleanUp() only on the current inner window.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Hmm.  That sounds like a bigger problem with event targets...

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Window doesn't keep any EventTargetHelper object alive (mEventTargetObjects contains raw pointers).
DisconnectFromOwner is there just to inform
EventTargetHelper objects that the owner window is gone, and that can be used as an optimization
to release stuff sooner. Also, that lets EventTargetHelper objects to keep just a raw pointer to 
the window.

(IIRC the original reason for DisconnectFromOwner was that Necko which keeps stuff alive too long, and that
ended up keeping XHR too long and then the whole window object.)

So I don't see any problem with EventTargetHelper objects. You need to design your classes so that objects can go away
even if the window is still there.

Comment 35

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #29)
> Bobby, do you happen to know whether the ordering guarantee comment 25
> mentions holds?  I can't prove or disprove it so far.  :(

I can't prove it easily either. The basic problem is that CleanUp is supposed to operate on outers, and FreeInnerObjects is supposed to operate on inners. But we do both for both, which makes it insanely difficult to wrap one's head around what's supposed to happen conceptually. :-(

Comment 36

[Karl Tomlinson (:karlt)]

(In reply to Olli Pettay [:smaug] from comment #34)
> (IIRC the original reason for DisconnectFromOwner was that Necko which keeps
> stuff alive too long, and that
> ended up keeping XHR too long and then the whole window object.)

When I first read this, I thought you were implying that DisconnectFromOwner
was a notification that stuff should cleanup early.  That is what AudioContext
is looking for here, but is not what DisconnectFromOwner currently provides.
I assume what you are referring to here is that DisconnectFromOwner means that
stuff doesn't need to keep a strong reference to the window.

Comment 37

[Karl Tomlinson (:karlt)]

Attachment: part 6: don't keep alive used AudioContexts from the window v3

As it is nsGlobalWindow code that doesn't know which cleanup/free code will
run first, and DisconnectFromOwner() is not intended as a cleanup
notification, this makes an ugly defensive in nsGlobalWindow code against the
unknowns by cleaning up AudioContexts in both situations.

There is already a precedence for this in calling CleanupCachedXBLHandlers().
The precise reason for this is not provided in bug 413447 comment 23, but
here's guessing it was similar uncertainties.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

right, and when DisconnectFromOwner is called, the EventTarget object may perhaps release also
other stuff earlier if it knows they can't be used without the window/owner.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Oh, you're making window to keep just raw references to AudioContext. Why can't you then rely on
DisconnectFromOwner() ? nsGlobalWindow::CleanUp does call DisconnectFromOwner on each known
EventTargetHelper.

Comment 40

[Karl Tomlinson (:karlt)]

(In reply to Olli Pettay [:smaug] from comment #39)
> Oh, you're making window to keep just raw references to AudioContext. Why
> can't you then rely on
> DisconnectFromOwner() ? nsGlobalWindow::CleanUp does call
> DisconnectFromOwner on each known
> EventTargetHelper.

Cleanup is called from ~nsGlobalWindow but not other close/DetachFromDocShell() methods if the window is in the bfcache.  I guess something in webaudio is keeping a reference to the window until the AudioContext gets a cleanup notification.  I haven't worked out what that is.

Comment 41

[Karl Tomlinson (:karlt)]

Attachment: don't keep alive used AudioContexts from the window v2 using DisconnectFromOwner

This was the approach used in comment 30.
It works fine in the testcases attached here, but leaked when reloading content/media/webaudio/test/test_audioBufferSourceNodeNeutered.html.
I'll narrow that down.

Comment 42

[Karl Tomlinson (:karlt)]

Attachment: rollup using v2 with DisconnectFromOwner

Includes patch in bug 914013, and some AudioContext constructor/destructor printfs.

Comment 43

[Karl Tomlinson (:karlt)]

Attachment: online-audio-bf-leak-2.html

When using rollup 2, this does not leak when loading in a tab and then closing the tab, but it does leak with these steps:
1. Load testcase.
2. unload, by loading about:blank in the same tab.
3. GC
4. CC

Desired results: window and AudioContext are deleted.
Actual: neither are deleted.

Comment 44

[Karl Tomlinson (:karlt)]

There is also a shutdown crash in MediaStreamGraph after the leak:

#6  0x0000000000000000 in ?? ()
#7  0x00007f9d59cfca47 in cubeb_stream_stop (stream=0x7f9bcc078520)
    at /home/karl/moz/dev/media/libcubeb/src/cubeb.c:215
#8  0x00007f9d57dda004 in mozilla::BufferedAudioStream::Pause (
    this=0x7f9bcc00e7e0)
    at /home/karl/moz/dev/content/media/AudioStream.cpp:701
#9  0x00007f9d57dd9a32 in mozilla::BufferedAudioStream::Shutdown (
    this=0x7f9bcc00e7e0)
    at /home/karl/moz/dev/content/media/AudioStream.cpp:587
#10 0x00007f9d57e23753 in mozilla::MediaStream::DestroyImpl (this=0x24f3b60)
    at /home/karl/moz/dev/content/media/MediaStreamGraph.cpp:1673
#11 0x00007f9d57e25509 in mozilla::ProcessedMediaStream::DestroyImpl (
    this=0x24f3b60)
    at /home/karl/moz/dev/content/media/MediaStreamGraph.cpp:2269
#12 0x00007f9d57e237e8 in mozilla::MediaStream::Message::Run (this=0x1b7b3b0)
    at /home/karl/moz/dev/content/media/MediaStreamGraph.cpp:1689
#13 0x00007f9d57e23835 in mozilla::MediaStream::Message::RunDuringShutdown (
    this=0x1b7b3b0)
    at /home/karl/moz/dev/content/media/MediaStreamGraph.cpp:1693
#14 0x00007f9d57e22f48 in mozilla::MediaStreamGraphImpl::AppendMessage (
    this=0x11e08a0, aMessage=0x1b7b3b0)
    at /home/karl/moz/dev/content/media/MediaStreamGraph.cpp:1523
#15 0x00007f9d57e238a4 in mozilla::MediaStream::Destroy (this=0x24f3b60)
    at /home/karl/moz/dev/content/media/MediaStreamGraph.cpp:1696
#16 0x00007f9d57e64e95 in mozilla::dom::AudioNode::DestroyMediaStream (
    this=0x2f4b840)
    at /home/karl/moz/dev/content/media/webaudio/AudioNode.cpp:322
#17 0x00007f9d57e5e5b8 in mozilla::dom::AudioDestinationNode::DestroyMediaStream (this=0x2f4b840)
    at /home/karl/moz/dev/content/media/webaudio/AudioDestinationNode.cpp:251
#18 0x00007f9d57e643ff in mozilla::dom::AudioNode::DisconnectFromGraph (
    this=0x2f4b840)
    at /home/karl/moz/dev/content/media/webaudio/AudioNode.cpp:136
#19 0x00007f9d57e63c5b in mozilla::dom::AudioNode::cycleCollection::Unlink (
    this=0x7f9d5d3194b0 <mozilla::dom::AudioNode::_cycleCollectorGlobal>, 
    p=0x2f4b840) at /home/karl/moz/dev/content/media/webaudio/AudioNode.cpp:21
#20 0x00007f9d5981770d in nsCycleCollector::CollectWhite (this=0xb33dc0, 
    aListener=0x0) at /home/karl/moz/dev/xpcom/base/nsCycleCollector.cpp:2363
#21 0x00007f9d5981876d in nsCycleCollector::FinishCollection (this=0xb33dc0, 
    aListener=0x0) at /home/karl/moz/dev/xpcom/base/nsCycleCollector.cpp:2799
#22 0x00007f9d598182e7 in nsCycleCollector::Collect (this=0xb33dc0, 
    aCCType=ShutdownCC, aWhiteNodes=0x7fff77925c10, aResults=0x0, 
    aListener=0x0) at /home/karl/moz/dev/xpcom/base/nsCycleCollector.cpp:2701
#23 0x00007f9d5981819c in nsCycleCollector::ShutdownCollect (this=0xb33dc0, 
    aListener=0x0) at /home/karl/moz/dev/xpcom/base/nsCycleCollector.cpp:2668
#24 0x00007f9d5981889e in nsCycleCollector::Shutdown (this=0xb33dc0)
    at /home/karl/moz/dev/xpcom/base/nsCycleCollector.cpp:2835
#25 0x00007f9d59819984 in nsCycleCollector_shutdown ()
    at /home/karl/moz/dev/xpcom/base/nsCycleCollector.cpp:3189
#26 0x00007f9d5978fdb0 in mozilla::ShutdownXPCOM (servMgr=0x0)
    at /home/karl/moz/dev/xpcom/build/nsXPComInit.cpp:756
#27 0x00007f9d5978f7d5 in NS_ShutdownXPCOM (servMgr=0xb13508)
    at /home/karl/moz/dev/xpcom/build/nsXPComInit.cpp:621

Not sure whether I can blame that entirely on the leak or whether that is another issue to resolve.

Comment 45

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #43)
> Created attachment 803371 [details]
> online-audio-bf-leak-2.html
> 
> When using rollup 2, this does not leak when loading in a tab and then
> closing the tab, but it does leak with these steps:
> 1. Load testcase.
> 2. unload, by loading about:blank in the same tab.
> 3. GC
> 4. CC
> 
> Desired results: window and AudioContext are deleted.
> Actual: neither are deleted.

Hmm.  Actually we don't want those to be deleted while they are in the bf cache.
And they do get deleted after the tab is closed.
Perhaps this is just an audio shutdown issue.

Comment 46

[Karl Tomlinson (:karlt)]

Attachment: online-audio-bf-leak-6.html

This is a better testcase for the v2 DisconnectFromOwner problem.

When using rollup v2, this does not leak when loading in a tab and then
immediately closing the tab, but it does leak with these steps:
1. Load testcase.
2. unload, by loading about:blank in the same tab.
3. close tab
4. GC
5. CC

Shutting down the browser reports a JSRuntime leak.

I assume AudioBuffer::mJSChannels is keeping the script global alive, the
window is never destroyed and so AudioContext never gets a cleanup
notification.

I don't get this leak with "don't keep alive used AudioContexts from the window v3"

The other problem smaug noticed with v2, as it is now, is that when a page doesn't go into bfcache (view-source: or reload) the AudioContexts are not muted, noticed by smaug).
That doesn't seem to be a problem using the FreeInnerObjects notification of v3.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 803299 [details] [diff] [review]
part 6: don't keep alive used AudioContexts from the window v3

r=me

Comment 48

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e3442b758e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8edc48525bc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/86c85d702556
https://hg.mozilla.org/integration/mozilla-inbound/rev/976ea2dba918
https://hg.mozilla.org/integration/mozilla-inbound/rev/18a21a1b330b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c13601b7924

The offline destination node self reference code is tested by test_convolverNode_mono_mono.html.
Our test suite doesn't test leaks that are cleaned up with the document.

Comment 49

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c5e3442b758e
https://hg.mozilla.org/mozilla-central/rev/8edc48525bc6
https://hg.mozilla.org/mozilla-central/rev/86c85d702556
https://hg.mozilla.org/mozilla-central/rev/976ea2dba918
https://hg.mozilla.org/mozilla-central/rev/18a21a1b330b
https://hg.mozilla.org/mozilla-central/rev/3c13601b7924

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Karl Tomlinson (:karlt) from comment #46)
> I assume AudioBuffer::mJSChannels is keeping the script global alive, the
> window is never destroyed and so AudioContext never gets a cleanup
> notification.
AudioBuffer certainly is a root in the leaked cycle.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

AudioBufferSourceNode::mPlayingRef, how do we end up releasing that if window is just deleted?
I don't see anything calling AudioContext::Shutdown()...

But apparently https://hg.mozilla.org/mozilla-central/rev/3c13601b7924 added such Shutdown call, so
I guess I should update my build :)

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

So the latest m-c doesn't show the problem, obviously because Shutdown() is called, but the rollop2
patch had too, in DisconnectFromOwner.
I think we have a bug in nsGlobalWindow handling :/ CleanUp is called only on the current inner window.
We should call it on all, even on those in bfcache.
And/Or 
  mEventTargetObjects.EnumerateEntries(DisconnectEventTargetObjects, nullptr);
  mEventTargetObjects.Clear();
should happen in FreeInnerObjects

Comment 53

[Karl Tomlinson (:karlt)]

Comment on attachment 803299 [details] [diff] [review]
part 6: don't keep alive used AudioContexts from the window v3

[Approval Request Comment]
Please consider this a request for all reviewed patches in this bug, which have landed on 26.

Bug caused by (feature/regressing bug #): webaudio
User impact if declined: Web pages that create and release webaudio contexts instead of reusing a single context will use unbounded memory resources.
Resources are released on page close.
Testing completed (on m-c, etc.): on m-c, aurora
Risk to taking this patch (and alternatives if risky): some risk, but limited to webaudio.
String or IDL/UUID changes made by this patch: none.

Comment 54

[Alex Keybl [:akeybl]]

(In reply to Karl Tomlinson (:karlt) from comment #53)
> Risk to taking this patch (and alternatives if risky): some risk, but
> limited to webaudio.

Thanks for this.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3ff1d6426073
https://hg.mozilla.org/releases/mozilla-beta/rev/0cd6256aacfe
https://hg.mozilla.org/releases/mozilla-beta/rev/dde022e03351
https://hg.mozilla.org/releases/mozilla-beta/rev/2f1d4aea750e
https://hg.mozilla.org/releases/mozilla-beta/rev/439302ba8435
https://hg.mozilla.org/releases/mozilla-beta/rev/7caeec1e5f7b