AudioContexts leak until their window is done

RESOLVED FIXED in Firefox 25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

(Depends on 1 bug)

Trunk
mozilla26
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox24 unaffected, firefox25+ fixed, firefox26+ fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(13 attachments, 2 obsolete attachments)

418 bytes, text/html
Details
398 bytes, text/html
Details
172 bytes, text/html
Details
1.27 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.27 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
6.28 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
1.04 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
3.31 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
254 bytes, text/html
Details
6.52 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.23 KB, patch
Details | Diff | Splinter Review
20.80 KB, patch
Details | Diff | Splinter Review
345 bytes, text/html
Details
Assignee

Description

6 years ago
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.
Assignee

Comment 1

6 years ago
Assignee

Comment 2

6 years ago
Assignee

Comment 3

6 years ago
Attachment #801389 - Flags: review?(roc)
Assignee

Comment 4

6 years ago
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.
Attachment #801390 - Flags: review?(ehsan)
Assignee

Updated

6 years ago
Depends on: 900711
Assignee

Updated

6 years ago
Attachment #801390 - Attachment description: remove.mediabufferdecoder.shutdown → remove MediaBufferDecoder::Shutdown(), no longer necessary since bug 900711, as part of making AudioContext::Shutdown() idempotent
Whiteboard: [MemShrink]
Assignee

Comment 5

6 years ago
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
Assignee

Updated

6 years ago
Attachment #801393 - Flags: review?(ehsan)
Assignee

Comment 6

6 years ago
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.
Attachment #801395 - Flags: review?(ehsan)
Assignee

Comment 7

6 years ago
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.
Attachment #801396 - Flags: review?(ehsan)
Assignee

Updated

6 years ago
Depends on: 914013
Assignee

Comment 8

6 years ago
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.
Attachment #801397 - Flags: review?(bzbarsky)
Attachment #801397 - Flags: feedback?(ehsan)
Assignee

Updated

6 years ago
Blocks: 914033
Assignee

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink][needs bug 900711 for Aurora]
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 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?
Attachment #801390 - Flags: review?(ehsan) → review+
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.
Attachment #801393 - Flags: review?(ehsan) → review+
Attachment #801395 - Flags: review?(ehsan) → review+
Attachment #801396 - Flags: review?(ehsan) → review+
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.
Attachment #801397 - Flags: feedback?(ehsan) → feedback-
Comment on attachment 801397 [details] [diff] [review]
don't keep alive used AudioContexts from the window

What ehsan said...
Attachment #801397 - Flags: review?(bzbarsky) → review-
Assignee

Comment 15

6 years ago
(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?
Flags: needinfo?(ehsan)
Flags: needinfo?(bzbarsky)
Assignee

Comment 16

6 years ago
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.
Attachment #801397 - Flags: feedback- → feedback?(ehsan)
I meant the part about contexts that are still playing.
Flags: needinfo?(bzbarsky)
Assignee

Comment 18

6 years ago
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.)
Assignee

Comment 19

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

Is comment 16 clear?
Attachment #801397 - Flags: review- → review?(bzbarsky)
I don't know enough aboutthe AudioNode ownership model to tell.  You really want Ehsan here.
(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!
Flags: needinfo?(ehsan)
Assignee

Comment 22

6 years ago
(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.
Assignee

Comment 23

6 years ago
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 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!
Attachment #801397 - Flags: review?(bzbarsky)
Attachment #801397 - Flags: review+
Attachment #801397 - Flags: feedback?(ehsan)
(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.
Flags: needinfo?(bzbarsky)
Also, Christoph, can you please take this patch for a fun ride inside the fuzzer, and let us know if it doesn't behave?  :-)
Flags: needinfo?(cdiehl)
(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.
Flags: needinfo?(cdiehl)
Ehsan, nothing is popping up here.
Bobby, do you happen to know whether the ordering guarantee comment 25 mentions holds?  I can't prove or disprove it so far.  :(
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley+bmo)
Assignee

Comment 30

6 years ago
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.
> 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?
Assignee

Comment 32

6 years ago
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.
Hmm.  That sounds like a bigger problem with event targets...
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(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. :-(
Flags: needinfo?(bobbyholley+bmo)
Assignee

Comment 36

6 years ago
(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.
Assignee

Comment 37

6 years ago
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.
Attachment #801397 - Attachment is obsolete: true
Attachment #803299 - Flags: review?(bzbarsky)
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.
Assignee

Updated

6 years ago
Blocks: webaudio
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.
Assignee

Comment 40

6 years ago
(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.
Assignee

Comment 41

6 years ago
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.
Assignee

Comment 42

6 years ago
Includes patch in bug 914013, and some AudioContext constructor/destructor printfs.
Assignee

Comment 43

6 years ago
Posted file online-audio-bf-leak-2.html (obsolete) —
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.
Assignee

Comment 44

6 years ago
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.
Assignee

Comment 45

6 years ago
(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.
Assignee

Comment 46

6 years ago
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.
Attachment #803371 - Attachment is obsolete: true
Comment on attachment 803299 [details] [diff] [review]
part 6: don't keep alive used AudioContexts from the window v3

r=me
Attachment #803299 - Flags: review?(bzbarsky) → review+
(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.
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 :)
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
Assignee

Updated

6 years ago
Blocks: 910171
Assignee

Updated

6 years ago
Depends on: 917260
Assignee

Comment 53

6 years ago
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.
Attachment #803299 - Attachment description: don't keep alive used AudioContexts from the window v3 → part 6: don't keep alive used AudioContexts from the window v3
Attachment #803299 - Flags: approval-mozilla-beta?
Assignee

Updated

6 years ago
Whiteboard: [MemShrink][needs bug 900711 for Aurora] → [MemShrink]
(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.
Attachment #803299 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

6 years ago
Depends on: 926522
Assignee

Updated

6 years ago
Depends on: 936317
You need to log in before you can comment on or make changes to this bug.