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.
Created attachment 357046 [details] Patch
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?
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.
(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
Created attachment 357049 [details] Patch
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
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
Comment on attachment 357049 [details] Patch Clearing flags on attachment: 357049 Committed r240142: <https://trac.webkit.org/changeset/240142>
All reviewed patches have been landed. Closing bug.
<rdar://problem/47382926>
(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.
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?
(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.