leak: AudioContext objects are leaking.

• Component: Web Audio
• Status: RESOLVED FIXED

[Ilya Tikhonovsky]

According to spec an AudioContext will not be garbage collected. It will live until the document goes away.
It is not true at the moment. It keeps alive even after navigation.

As a result it is leaking 
- HRTFDatabase instance ~36mb;
- if it is an offline audio context then it also leaking offline render thread;
and other relatively small objects.

I found two different reasons for such leak:
1) AudioContext deletes OfflineDestinationNode only if it has been initialized.
2) at uninitialization stage it shuts down audio thread in Destination node but the thread is a part of nodes deletion process.

[Ilya Tikhonovsky]

Created attachment 174379 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #1)
> Created an attachment (id=174379) [details]
> Patch

You should be able to test this by using inspector protocol test harness to capture native memory snapshot and compare audio context footprint before and after navigation to see that the context was actually destroyed.

[Adam Barth]

Comment on attachment 174379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174379&action=review

> Source/WebCore/Modules/webaudio/AudioContext.cpp:199
> +    ref();

There must be a way to solve this problem without calling ref() and deref() manually.

[Ilya Tikhonovsky]

Comment on attachment 174379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174379&action=review

>> Source/WebCore/Modules/webaudio/AudioContext.cpp:199
>> +    ref();
> 
> There must be a way to solve this problem without calling ref() and deref() manually.

I chose this solution because I had found about 20 cases in WebCore where the same technic was used.

The simplest variant is to have RefPtr<AudioContext> m_self member inside AudioContext and release it instead of deref. I'd say that it looks less understandable. I found only two places with the same trick. FileThread and DatabaseThread classes.
The other way is to have a Vector<RefPtr<AudioContext> > somewhere inside ScriptExecutionContext or Document. This variant adds another dependency .

What do you think about this solutions and do you know another class with the same lifetime.

[Adam Barth]

> I chose this solution because I had found about 20 cases in WebCore where the same technic was used.

Really?  I'm curious where those are.  We've been trying to avoid manually calling ref and deref for a while.

> What do you think about this solutions and do you know another class with the same lifetime.

I haven't studied the issue in detail, but one approach is to have hasPendingActivity() return |true| in these cases and then the JavaScript garbage collector will keep this object alive (because it is an ActiveDOMObject).

[Ilya Tikhonovsky]

Created attachment 174704 [details]
Patch

[Ilya Tikhonovsky]

(In reply to comment #5)
> > I chose this solution because I had found about 20 cases in WebCore where the same technic was used.
> 
> Really?  I'm curious where those are.  We've been trying to avoid manually calling ref and deref for a while.
> 
> > What do you think about this solutions and do you know another class with the same lifetime.
> 
> I haven't studied the issue in detail, but one approach is to have hasPendingActivity() return |true| in these cases and then the JavaScript garbage collector will keep this object alive (because it is an ActiveDOMObject).

done.

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=174379) [details] [details]
> > Patch
> 
> You should be able to test this by using inspector protocol test harness to capture native memory snapshot and compare audio context footprint before and after navigation to see that the context was actually destroyed.

done.

[Early Warning System Bot]

Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14872203

[Early Warning System Bot]

Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14860309

[Adam Barth]

Comment on attachment 174704 [details]
Patch

The ActiveDOMObject integration looks good.  I wanted to mark this r+, but I wasn't sure about the audio-specific parts.  Thanks for iterating on the approach.

[EFL EWS Bot]

Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14848887

[kov's GTK+ EWS bot]

Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14872233

[Chris Rogers]

Comment on attachment 174704 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174704&action=review

Thanks for doing this.  The audio parts seem ok.  For what it's worth, the AudioContext used to not leak and is something I definitely verified, but obviously code changes...  One change which may have been an issue is:
http://trac.webkit.org/changeset/133239

I have some minor style nits, and looks like the bots are unhappy, but looks good from the audio standpoint.  I'll leave it up to Adam as far as the details of the test.

> LayoutTests/inspector-protocol/nmi-webaudio-leak-test.html:25
> +	for (var index = 0; index < typeNames.length; ++index) {

nit: indentation

> LayoutTests/inspector-protocol/nmi-webaudio-leak-test.html:56
> +	else

nit: indentation

> LayoutTests/inspector-protocol/nmi-webaudio-leak-test.html:61
> +    function iFrameNavigated(messageObject)  {

nit: extra space

[Build Bot]

Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14861419

[WebKit Review Bot]

Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14872366

New failing tests:
inspector-protocol/nmi-webaudio-leak-test.html

[Peter Beverloo (cr-android ews)]

Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14876174

[Build Bot]

Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14868505

New failing tests:
inspector-protocol/nmi-webaudio-leak-test.html

[Build Bot]

Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14873449

New failing tests:
inspector-protocol/nmi-webaudio-leak-test.html

[Ilya Tikhonovsky]

Created attachment 174846 [details]
with guard #if ENABLE(WEB_AUDIO)

[Adam Barth]

Comment on attachment 174846 [details]
with guard #if ENABLE(WEB_AUDIO)

View in context: https://bugs.webkit.org/attachment.cgi?id=174846&action=review

> Source/WebCore/Modules/webaudio/AudioContext.cpp:317
>      m_document = 0; // document is going away

There's a bug on file about m_document not being needed in this class because ActiveDOMObject already provides one.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:319
> +    // Usually ScriptExecutionController calls stop twice.

What is a ScriptExecutionController?  Grepping in WebCore doesn't find anything by that name.

> Source/WebCore/Modules/webaudio/AudioContext.h:260
> +    // ScriptExecutionContext calls stop twice.
> +    // We'd like to schedule only one stop action for them.

Is that a bug with ScriptExecutionContext?  Should we file a but and investigate?

> LayoutTests/inspector-protocol/nmi-webaudio-leak-test.html:3
> +<script type="text/javascript" src="../http/tests/inspector-protocol/resources/protocol-test.js"></script>

type="text/javascript"  <--- this isn't needed

[WebKit Review Bot]

Comment on attachment 174846 [details]
with guard #if ENABLE(WEB_AUDIO)

Attachment 174846 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14879521

New failing tests:
inspector-protocol/nmi-webaudio-leak-test.html

[Ilya Tikhonovsky]

Committed r135111: <http://trac.webkit.org/changeset/135111>

[Ilya Tikhonovsky]

(In reply to comment #20)
> (From update of attachment 174846 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174846&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioContext.cpp:317
> >      m_document = 0; // document is going away
> 
> There's a bug on file about m_document not being needed in this class because ActiveDOMObject already provides one.

I'll do that in a separate patch.

> 
> > Source/WebCore/Modules/webaudio/AudioContext.cpp:319
> > +    // Usually ScriptExecutionController calls stop twice.
> 
> What is a ScriptExecutionController?  Grepping in WebCore doesn't find anything by that name.

typo was fixed.

> 
> > Source/WebCore/Modules/webaudio/AudioContext.h:260
> > +    // ScriptExecutionContext calls stop twice.
> > +    // We'd like to schedule only one stop action for them.
> 
> Is that a bug with ScriptExecutionContext?  Should we file a but and investigate?

will do

> 
> > LayoutTests/inspector-protocol/nmi-webaudio-leak-test.html:3
> > +<script type="text/javascript" src="../http/tests/inspector-protocol/resources/protocol-test.js"></script>
> 
> type="text/javascript"  <--- this isn't needed

done

[Ilya Tikhonovsky]

> > Source/WebCore/Modules/webaudio/AudioContext.h:260
> > +    // ScriptExecutionContext calls stop twice.
> > +    // We'd like to schedule only one stop action for them.
> 
> Is that a bug with ScriptExecutionContext?  Should we file a but and investigate?

It is calling twice from DocumentLoader::commitLoad with these call stacks.

#0  WebCore::AudioContext::stop () at ../../third_party/WebKit/Source/WebCore/Modules/webaudio/AudioContext.cpp:316
#1  WebCore::ScriptExecutionContext::stopActiveDOMObjects () at ../../third_party/WebKit/Source/WebCore/dom/ScriptExecutionContext.cpp:234
#2  WebCore::Document::detach () at ../../third_party/WebKit/Source/WebCore/dom/Document.cpp:2093
#3  WebCore::Document::prepareForDestruction () at ../../third_party/WebKit/Source/WebCore/dom/Document.cpp:2155
#4  WebCore::Frame::setView (view=...) at ../../third_party/WebKit/Source/WebCore/page/Frame.cpp:266
#5  WebCore::Frame::createView (viewportSize=..., backgroundColor=..., transparent=false, fixedLayoutSize=..., fixedVisibleContentRect=..., useFixedLayout=false, horizontalScrollbarMode=WebCore::ScrollbarAuto, horizontalLock=false, 
    verticalScrollbarMode=WebCore::ScrollbarAuto, verticalLock=false) at ../../third_party/WebKit/Source/WebCore/page/Frame.cpp:781
#6  WebKit::WebFrameImpl::createFrameView () at ../../third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:2287
#7  WebKit::FrameLoaderClientImpl::makeDocumentView () at ../../third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:282
#8  WebKit::FrameLoaderClientImpl::transitionToCommittedForNewPage () at ../../third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1420
#9  WebCore::FrameLoader::transitionToCommitted (cachedPage=...) at ../../third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:1850
#10 WebCore::FrameLoader::commitProvisionalLoad () at ../../third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:1692
#11 WebCore::DocumentLoader::commitIfReady () at ../../third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:281
#12 WebCore::DocumentLoader::commitLoad (data=0x7fffec2e2140 "<html>\n</html>\n\377") at ../../third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:313
#13 WebCore::DocumentLoader::receivedData (data=0x7fffec2e2140 "<html>\n</html>\n\377") at ../../third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:387


#0  WebCore::AudioContext::stop () at ../../third_party/WebKit/Source/WebCore/Modules/webaudio/AudioContext.cpp:316
#1  WebCore::ScriptExecutionContext::stopActiveDOMObjects () at ../../third_party/WebKit/Source/WebCore/dom/ScriptExecutionContext.cpp:234
#2  WebCore::FrameLoader::clear (newDocument=0x7fffec30d000, clearWindowProperties=true, clearScriptObjects=true, clearFrameView=true) at ../../third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:560
#3  WebCore::DocumentWriter::begin (urlReference=..., dispatch=false, ownerDocument=0x0) at ../../third_party/WebKit/Source/WebCore/loader/DocumentWriter.cpp:134
#4  WebCore::DocumentLoader::commitData (bytes=0x7fffec2e2140 "<html>\n</html>\n\377") at ../../third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:328
#5  WebKit::WebFrameImpl::commitDocumentData (data=0x7fffec2e2140 "<html>\n</html>\n\377") at ../../third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:1125
#6  WebKit::FrameLoaderClientImpl::committedLoad (loader=0x7fffeb864000, data=0x7fffec2e2140 "<html>\n</html>\n\377") at ../../third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1136
#7  WebCore::DocumentLoader::commitLoad (data=0x7fffec2e2140 "<html>\n</html>\n\377") at ../../third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:321
#8  WebCore::DocumentLoader::receivedData (data=0x7fffec2e2140 "<html>\n</html>\n\377") at ../../third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:387

[Kentaro Hara]

Reverted r135111 for reason:

The patch caused crashes in several layout tests

Committed r135145: <http://trac.webkit.org/changeset/135145>

[Kentaro Hara]

Several tests hit an assertion in a Linux Debug build.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=webaudio%2Fbiquad-allpass.html

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=webaudio%2Fdistance-inverse.html

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=webaudio%2Faudionode-connect-order.html

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=webaudio%2Faudioparam-setTargetAtTime.html

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=webaudio%2Fdelaynode-max-nondefault-delay.html

...etc

[Ilya Tikhonovsky]

Committed r135152: <http://trac.webkit.org/changeset/135152>

[Ilya Tikhonovsky]

Reopening to attach new patch.

[Ilya Tikhonovsky]

Created attachment 174971 [details]
relanded with RefPtr<AudioContext> guard in deleteMarkedNodes

[Philippe Normand]

*** Bug 86910 has been marked as a duplicate of this bug. ***

[Philippe Normand]

*** Bug 80681 has been marked as a duplicate of this bug. ***

[Philippe Normand]

Is this test specific to Chromium?
In a GTK build with WebAudio enabled the test times out. Memory.getProcessMemoryDistribution() returns an amount much lower than 15000000:  996. So it loops forever calling:

InspectorTest.sendCommand('Memory.getProcessMemoryDistribution', {}, waitForSharedAudioData);

again and again.

[Kenneth Russell]

(In reply to comment #32)
> Is this test specific to Chromium?
> In a GTK build with WebAudio enabled the test times out. Memory.getProcessMemoryDistribution() returns an amount much lower than 15000000:  996. So it loops forever calling:
> 
> InspectorTest.sendCommand('Memory.getProcessMemoryDistribution', {}, waitForSharedAudioData);
> 
> again and again.

Ilya: in order to make this test more robust and portable I suggest adding a new Inspector counter tracking the number of live AudioContexts. See Source/WebCore/testing/Internals.idl, numberOfLiveNodes/Documents, and how they're implemented.