Bug 192586 – [WebAudio] Call AudioContext::uninitialize() immediately when the AudioContext is stopped

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Yacine Bandou]

When WebProcess is killed, AudioContext::uninitialize() is not called immediately in the stop so the AudioDestinationNode is not destroyed.

In my case, I have a resource device manager, the output audio device is reserved when AudioDestinationNode is instantiated and it is released when AudioDestinationNode is destroyed, thus when the webprocess is killed, the resources leak. 

AudioContext::uninitialize() is not called immediately since commit baa073da82c04c74a501c8c8a513a2d50195dff8 from 2011.
This modification can now be reverted without regression in WebAudio tests.

[Yacine Bandou]

Created attachment 357046 [details]
Patch

[Philippe Normand]

Comment on attachment 357046 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +         AudioContext::uninitialize() is not called immediately since commit baa073da82c04c74a501c8c8a513a2d50195dff8.

This should be a SVN revision, not git commit SHA1

> Source/WebCore/Modules/webaudio/AudioContext.cpp:-323
> -    callOnMainThread([this] {

This was added in https://bugs.webkit.org/show_bug.cgi?id=143816 ... why?

[Xabier Rodríguez Calvar]

Comment on attachment 357046 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +         In my case, I have a resource device manager, the output audio device is reserved when AudioDestinationNode
> +         is instantiated and it is released when AudioDestinationNode is destroyed, thus when the webprocess is killed,
> +         the resources leak.
> +
> +         AudioContext::uninitialize() is not called immediately since commit baa073da82c04c74a501c8c8a513a2d50195dff8.
> +         This modification can now be reverted without regression in WebAudio tests.

And this is wrongly indented.

[Yacine Bandou]

(In reply to Philippe Normand from comment #2)
> Comment on attachment 357046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357046&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +         AudioContext::uninitialize() is not called immediately since commit baa073da82c04c74a501c8c8a513a2d50195dff8.
> 
> This should be a SVN revision, not git commit SHA1
> 
> > Source/WebCore/Modules/webaudio/AudioContext.cpp:-323
> > -    callOnMainThread([this] {
> 
> This was added in https://bugs.webkit.org/show_bug.cgi?id=143816 ... why?

No, It was added in https://trac.webkit.org/changeset/94608/webkit

[Yacine Bandou]

Created attachment 357049 [details]
Patch

[EWS Watchlist]

Comment on attachment 357049 [details]
Patch

Attachment 357049 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10353964

New failing tests:
fast/images/animated-image-mp4-crash.html

[EWS Watchlist]

Created attachment 357059 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[WebKit Commit Bot]

Comment on attachment 357049 [details]
Patch

Clearing flags on attachment: 357049

Committed r240142: <https://trac.webkit.org/changeset/240142>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47382926>

[Darin Adler]

(In reply to Yacine Bandou from comment #0)
> This modification can now be reverted without regression in WebAudio tests.

I’m sorry; I don’t understand why this is the bar for knowing whether the code is needed. Do we have coverage for this teardown scenario in the WebAudio tests? The original patch when this change was made did not add any new tests.

It’s absolutely clear that we want to stop immediately.

What’s not so clear is if the problems with reentrancy and active DOM object teardown that originally motivated this change are covered with tests and have been solved. I am concerned about the test coverage.

[Philippe Normand]

Well indeed it's hard to know for sure if this patch doesn't introduce regressions elsewhere :( However the ChangeLog mentions webaudio/mediaelementaudiosourcenode-gc.html ... can you tell us more Yacine?

[Yacine Bandou]

(In reply to Darin Adler from comment #11)
> (In reply to Yacine Bandou from comment #0)
> > This modification can now be reverted without regression in WebAudio tests.
> 
> I’m sorry; I don’t understand why this is the bar for knowing whether the
> code is needed. Do we have coverage for this teardown scenario in the
> WebAudio tests? The original patch when this change was made did not add any
> new tests.
> 
> It’s absolutely clear that we want to stop immediately.
> 
> What’s not so clear is if the problems with reentrancy and active DOM object
> teardown that originally motivated this change are covered with tests and
> have been solved. I am concerned about the test coverage.

Sorry for this late response.

The original patch has added a new test see https://trac.webkit.org/changeset/94608/webkit

This fix is somehow a revert of this original patch, and for me the non regression is guaranteed by this test.