Bug 192586

Summary: [WebAudio] Call AudioContext::uninitialize() immediately when the AudioContext is stopped
Product: WebKit Reporter: Yacine Bandou <bandou.yacine>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, commit-queue, darin, ews-watchlist, jer.noble, olivier.blin, pnormand, tsaunier, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews116 for mac-sierra none

Yacine Bandou
Reported 2018-12-11 06:16:56 PST
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.
Attachments
Patch (2.54 KB, patch)
2018-12-11 06:32 PST, Yacine Bandou 		no flags
DetailsFormatted DiffDiff
Patch (2.50 KB, patch)
2018-12-11 07:46 PST, Yacine Bandou 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews116 for mac-sierra (2.03 MB, application/zip)
2018-12-11 09:40 PST, EWS Watchlist 		no flags
Details
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Yacine Bandou
Comment 1 2018-12-11 06:32:10 PST
Created attachment 357046 [details] Patch
Philippe Normand
Comment 2 2018-12-11 06:52:27 PST
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 3 2018-12-11 06:53:35 PST
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
Comment 4 2018-12-11 07:45:44 PST
(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
Comment 5 2018-12-11 07:46:32 PST
Created attachment 357049 [details] Patch
EWS Watchlist
Comment 6 2018-12-11 09:40:03 PST
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
Comment 7 2018-12-11 09:40:05 PST
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 8 2019-01-18 05:46:34 PST
Comment on attachment 357049 [details] Patch Clearing flags on attachment: 357049 Committed r240142: <https://trac.webkit.org/changeset/240142>
WebKit Commit Bot
Comment 9 2019-01-18 05:46:36 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 10 2019-01-18 05:47:28 PST
<rdar://problem/47382926>
Darin Adler
Comment 11 2019-01-18 12:38:44 PST
(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
Comment 12 2019-01-21 01:44:57 PST
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
Comment 13 2019-01-22 05:10:12 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.