Bug 102356

Summary: leak: AudioContext objects are leaking.
Product: WebKit Reporter: Ilya Tikhonovsky <loislo>
Component: Web AudioAssignee: Ilya Tikhonovsky <loislo>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, crogers, dglazkov, eric.carlson, feature-media-reviews, gtk-ews, gyuyoung.kim, haraken, japhet, jer.noble, kbr, peter+ews, philn, pnormand, rakuco, webkit-ews, webkit.review.bot, xan.lopez, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
with guard #if ENABLE(WEB_AUDIO)
abarth: review+, abarth: commit-queue-
relanded with RefPtr<AudioContext> guard in deleteMarkedNodes none

Description Ilya Tikhonovsky 2012-11-15 02:05:03 PST
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.
Comment 1 Ilya Tikhonovsky 2012-11-15 02:25:24 PST
Created attachment 174379 [details]
Patch
Comment 2 Yury Semikhatsky 2012-11-15 07:18:54 PST
(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.
Comment 3 Adam Barth 2012-11-15 10:36:39 PST
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.
Comment 4 Ilya Tikhonovsky 2012-11-15 12:25:00 PST
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.
Comment 5 Adam Barth 2012-11-15 12:43:20 PST
> 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).
Comment 6 Ilya Tikhonovsky 2012-11-16 09:11:29 PST
Created attachment 174704 [details]
Patch
Comment 7 Ilya Tikhonovsky 2012-11-16 09:14:27 PST
(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.
Comment 8 Early Warning System Bot 2012-11-16 09:22:46 PST
Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14872203
Comment 9 Early Warning System Bot 2012-11-16 09:27:34 PST
Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14860309
Comment 10 Adam Barth 2012-11-16 09:50:28 PST
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.
Comment 11 EFL EWS Bot 2012-11-16 10:10:49 PST
Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14848887
Comment 12 kov's GTK+ EWS bot 2012-11-16 10:55:18 PST
Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14872233
Comment 13 Chris Rogers 2012-11-16 15:02:48 PST
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
Comment 14 Build Bot 2012-11-16 19:34:56 PST
Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14861419
Comment 15 WebKit Review Bot 2012-11-16 19:48:53 PST
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
Comment 16 Peter Beverloo (cr-android ews) 2012-11-16 22:09:51 PST
Comment on attachment 174704 [details]
Patch

Attachment 174704 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14876174
Comment 17 Build Bot 2012-11-17 03:07:23 PST
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
Comment 18 Build Bot 2012-11-17 04:08:49 PST
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
Comment 19 Ilya Tikhonovsky 2012-11-18 06:18:23 PST
Created attachment 174846 [details]
with guard #if ENABLE(WEB_AUDIO)
Comment 20 Adam Barth 2012-11-18 09:07:48 PST
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
Comment 21 WebKit Review Bot 2012-11-18 12:55:13 PST
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
Comment 22 Ilya Tikhonovsky 2012-11-18 22:11:20 PST
Committed r135111: <http://trac.webkit.org/changeset/135111>
Comment 23 Ilya Tikhonovsky 2012-11-18 22:17:26 PST
(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
Comment 24 Ilya Tikhonovsky 2012-11-19 04:01:38 PST
> > 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
Comment 25 Kentaro Hara 2012-11-19 05:36:50 PST
Reverted r135111 for reason:

The patch caused crashes in several layout tests

Committed r135145: <http://trac.webkit.org/changeset/135145>
Comment 27 Ilya Tikhonovsky 2012-11-19 06:54:23 PST
Committed r135152: <http://trac.webkit.org/changeset/135152>
Comment 28 Ilya Tikhonovsky 2012-11-19 06:58:09 PST
Reopening to attach new patch.
Comment 29 Ilya Tikhonovsky 2012-11-19 06:58:17 PST
Created attachment 174971 [details]
relanded with RefPtr<AudioContext> guard in deleteMarkedNodes
Comment 30 Philippe Normand 2012-12-29 08:21:01 PST
*** Bug 86910 has been marked as a duplicate of this bug. ***
Comment 31 Philippe Normand 2012-12-29 08:21:10 PST
*** Bug 80681 has been marked as a duplicate of this bug. ***
Comment 32 Philippe Normand 2012-12-30 09:47:21 PST
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.
Comment 33 Kenneth Russell 2012-12-30 21:28:54 PST
(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.