Summary: | OfflineAudioDestinationNode::startRendering leaks OfflineAudioDestinationNode if offlineRender exists early | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
Component: | Web Audio | Assignee: | Ryosuke Niwa <rniwa> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ddkilzer, dino, eric.carlson, jer.noble, youennf | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Ryosuke Niwa
2019-04-09 19:34:49 PDT
Created attachment 367098 [details]
Fixes the bug
Comment on attachment 367098 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=367098&action=review > Source/WebCore/ChangeLog:9 > + But offlineRender can early exit without ever calling deref() in the main thread, leaking to the leak of s/leaking/leading/ > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.h:69 > + bool startRenderingIfPossible(); Nit: looks like this isn't necessary. Comment on attachment 367098 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=367098&action=review > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:94 > + m_renderThread = Thread::create("offline renderer", [this] { Instead of ref/deref, could the thread lambda take a protectedThis=makeRef(*this) and move protectedThis to the callOnMainThread lambda below? That will make it clear that 'this' is protected and ref/deref count is always ok. (In reply to youenn fablet from comment #4) > Comment on attachment 367098 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=367098&action=review > > > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:94 > > + m_renderThread = Thread::create("offline renderer", [this] { > > Instead of ref/deref, could the thread lambda take a > protectedThis=makeRef(*this) and move protectedThis to the callOnMainThread > lambda below? > That will make it clear that 'this' is protected and ref/deref count is > always ok. That's a bit dangerous since OfflineAudioDestinationNode is not thread-safe ref counted. We need to very carefully copy & not copy things in each lambdas. (In reply to Eric Carlson from comment #3) > Comment on attachment 367098 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=367098&action=review > > > Source/WebCore/ChangeLog:9 > > + But offlineRender can early exit without ever calling deref() in the main thread, leaking to the leak of > > s/leaking/leading/ Fixed. > > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.h:69 > > + bool startRenderingIfPossible(); > > Nit: looks like this isn't necessary. Indeed. Removed. Committed r244145: <https://trac.webkit.org/changeset/244145> |