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

Ilya Tikhonovsky
Reported 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.
Attachments
Patch (9.75 KB, patch)
2012-11-15 02:25 PST, Ilya Tikhonovsky
no flags
Patch (19.18 KB, patch)
2012-11-16 09:11 PST, Ilya Tikhonovsky
no flags
with guard #if ENABLE(WEB_AUDIO) (24.35 KB, patch)
2012-11-18 06:18 PST, Ilya Tikhonovsky
abarth: review+
abarth: commit-queue-
relanded with RefPtr<AudioContext> guard in deleteMarkedNodes (26.59 KB, patch)
2012-11-19 06:58 PST, Ilya Tikhonovsky
no flags
Ilya Tikhonovsky
Comment 1 2012-11-15 02:25:24 PST
Yury Semikhatsky
Comment 2 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.
Adam Barth
Comment 3 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.
Ilya Tikhonovsky
Comment 4 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.
Adam Barth
Comment 5 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).
Ilya Tikhonovsky
Comment 6 2012-11-16 09:11:29 PST
Ilya Tikhonovsky
Comment 7 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.
Early Warning System Bot
Comment 8 2012-11-16 09:22:46 PST
Early Warning System Bot
Comment 9 2012-11-16 09:27:34 PST
Adam Barth
Comment 10 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.
EFL EWS Bot
Comment 11 2012-11-16 10:10:49 PST
kov's GTK+ EWS bot
Comment 12 2012-11-16 10:55:18 PST
Chris Rogers
Comment 13 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
Build Bot
Comment 14 2012-11-16 19:34:56 PST
WebKit Review Bot
Comment 15 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
Peter Beverloo (cr-android ews)
Comment 16 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
Build Bot
Comment 17 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
Build Bot
Comment 18 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
Ilya Tikhonovsky
Comment 19 2012-11-18 06:18:23 PST
Created attachment 174846 [details] with guard #if ENABLE(WEB_AUDIO)
Adam Barth
Comment 20 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
WebKit Review Bot
Comment 21 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
Ilya Tikhonovsky
Comment 22 2012-11-18 22:11:20 PST
Ilya Tikhonovsky
Comment 23 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
Ilya Tikhonovsky
Comment 24 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
Kentaro Hara
Comment 25 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>
Ilya Tikhonovsky
Comment 27 2012-11-19 06:54:23 PST
Ilya Tikhonovsky
Comment 28 2012-11-19 06:58:09 PST
Reopening to attach new patch.
Ilya Tikhonovsky
Comment 29 2012-11-19 06:58:17 PST
Created attachment 174971 [details] relanded with RefPtr<AudioContext> guard in deleteMarkedNodes
Philippe Normand
Comment 30 2012-12-29 08:21:01 PST
*** Bug 86910 has been marked as a duplicate of this bug. ***
Philippe Normand
Comment 31 2012-12-29 08:21:10 PST
*** Bug 80681 has been marked as a duplicate of this bug. ***
Philippe Normand
Comment 32 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.
Kenneth Russell
Comment 33 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.
Note You need to log in before you can comment on or make changes to this bug.