WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
196759
OfflineAudioDestinationNode::startRendering leaks OfflineAudioDestinationNode if offlineRender exists early
https://bugs.webkit.org/show_bug.cgi?id=196759
Summary
OfflineAudioDestinationNode::startRendering leaks OfflineAudioDestinationNode...
Ryosuke Niwa
Reported
2019-04-09 19:34:49 PDT
OfflineAudioDestinationNode::startRendering calls ref() unconditionally but offlineRender() calls deref() conditionally.
Attachments
Fixes the bug
(4.92 KB, patch)
2019-04-09 19:40 PDT
,
Ryosuke Niwa
eric.carlson
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1
2019-04-09 19:34:58 PDT
<
rdar://problem/47900062
>
Ryosuke Niwa
Comment 2
2019-04-09 19:40:23 PDT
Created
attachment 367098
[details]
Fixes the bug
Eric Carlson
Comment 3
2019-04-10 12:02:09 PDT
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.
youenn fablet
Comment 4
2019-04-10 12:06:38 PDT
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.
Ryosuke Niwa
Comment 5
2019-04-10 13:23:57 PDT
(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.
Ryosuke Niwa
Comment 6
2019-04-10 13:25:15 PDT
(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.
Ryosuke Niwa
Comment 7
2019-04-10 13:31:06 PDT
Committed
r244145
: <
https://trac.webkit.org/changeset/244145
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug