Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698813007:
Fix teardown of stale AudioStreamMonitor poll callbacks. (Closed)
|
Created:
3 years, 10 months ago by DaleCurtis Modified:
3 years, 10 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, nasko+codewatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix teardown of stale AudioStreamMonitor poll callbacks.
It's possible for the RenderProcessHost and WebContents (and thus AudioStreamMonitor) to
survive the death of the render process and subsequently be reused. During this
period StartStopMonitoringHelper() will be unable to lookup the WebContents
using the now-dead |render_frame_id|. We must thus have a secondary mechanism for clearing stale callbacks.
This change introduces AudioStreamMonitor:RenderProcessGone() which is called by
the WebContents whenever WCO::RenderProcessGone() is. This allows the audio stream
monitor to clear stale callbacks that the AudioOutputDelegateImpl will no longer
be able to route to it.
Subsequently this also fixes a bug on Android (and anywhere else power monitoring
is disabled), where process termination would not correctly clear the audible
state for a process.
On all platforms this fixes a minor leak of AudioOutputController objects, since
they are bound by ref-ptr into the callback map containing stale entries. I say
minor since they would immediately be overwritten if the process is reusued or
tossed completely when the WebContents object goes away.
BUG=692857
TEST=new unittest, manual test:
1. load http://mounirlamouri.github.io/sandbox/autoplay/test.html
2. kill process from task manager.
3. click the reload button == boom (boom no longer after patch!).
Review-Url: https://codereview.chromium.org/2698813007
Cr-Commit-Position: refs/heads/master@{#451664}
Committed: https://chromium.googlesource.com/chromium/src/+/3d1e1301ee8f7c1cab24e2d161218336ea0e7b7c
Patch Set 1 #
Total comments: 6
Patch Set 2 : Relocate RenderProcessGone() call. #Patch Set 3 : Always store callbacks, fix tests. #
Messages
Total messages: 23 (11 generated)
dalecurtis@chromium.org changed reviewers: + creis@chromium.org, maxmorin@chromium.org
Found the bug :)
Description was changed from ========== Fix teardown of stale AudioStreamMonitor poll callbacks. It's possible for the RenderProcessHost and WebContents (and thus this class) to survive the death of the render process and subsequently be reused. During this period StartStopMonitoringHelper() will be unable to lookup the WebContents using the now-dead |render_frame_id|. We must thus have this secondary mechanism for clearing stale callbacks. This change introduces AudioStreamMonitor:RenderProcessGone() which is called by the WebContents whenever WC::RenderProcessGone() is. This allows the audio stream monitor to clear stale callbacks that the AudioOutputDelegateImpl will no longer be able to route to it. Subsequently this also fixes a bug on Android (and anywhere else power monitoring is disabled), where process termination would not correctly clear the audible state for a process. BUG=692857 TEST=new unittest, manual test: 1. load http://mounirlamouri.github.io/sandbox/autoplay/test.html 2. kill process from task manager. 3. click the reload button == boom (boom no longer after patch!). ========== to ========== Fix teardown of stale AudioStreamMonitor poll callbacks. It's possible for the RenderProcessHost and WebContents (and thus this class) to survive the death of the render process and subsequently be reused. During this period StartStopMonitoringHelper() will be unable to lookup the WebContents using the now-dead |render_frame_id|. We must thus have this secondary mechanism for clearing stale callbacks. This change introduces AudioStreamMonitor:RenderProcessGone() which is called by the WebContents whenever WC::RenderProcessGone() is. This allows the audio stream monitor to clear stale callbacks that the AudioOutputDelegateImpl will no longer be able to route to it. Subsequently this also fixes a bug on Android (and anywhere else power monitoring is disabled), where process termination would not correctly clear the audible state for a process. On all platforms this fixes a minor leak of AudioOutputController objects, since they are bound by ref-ptr into the callback map containing stale entries. BUG=692857 TEST=new unittest, manual test: 1. load http://mounirlamouri.github.io/sandbox/autoplay/test.html 2. kill process from task manager. 3. click the reload button == boom (boom no longer after patch!). ==========
Description was changed from ========== Fix teardown of stale AudioStreamMonitor poll callbacks. It's possible for the RenderProcessHost and WebContents (and thus this class) to survive the death of the render process and subsequently be reused. During this period StartStopMonitoringHelper() will be unable to lookup the WebContents using the now-dead |render_frame_id|. We must thus have this secondary mechanism for clearing stale callbacks. This change introduces AudioStreamMonitor:RenderProcessGone() which is called by the WebContents whenever WC::RenderProcessGone() is. This allows the audio stream monitor to clear stale callbacks that the AudioOutputDelegateImpl will no longer be able to route to it. Subsequently this also fixes a bug on Android (and anywhere else power monitoring is disabled), where process termination would not correctly clear the audible state for a process. On all platforms this fixes a minor leak of AudioOutputController objects, since they are bound by ref-ptr into the callback map containing stale entries. BUG=692857 TEST=new unittest, manual test: 1. load http://mounirlamouri.github.io/sandbox/autoplay/test.html 2. kill process from task manager. 3. click the reload button == boom (boom no longer after patch!). ========== to ========== Fix teardown of stale AudioStreamMonitor poll callbacks. It's possible for the RenderProcessHost and WebContents (and thus this class) to survive the death of the render process and subsequently be reused. During this period StartStopMonitoringHelper() will be unable to lookup the WebContents using the now-dead |render_frame_id|. We must thus have this secondary mechanism for clearing stale callbacks. This change introduces AudioStreamMonitor:RenderProcessGone() which is called by the WebContents whenever WC::RenderProcessGone() is. This allows the audio stream monitor to clear stale callbacks that the AudioOutputDelegateImpl will no longer be able to route to it. Subsequently this also fixes a bug on Android (and anywhere else power monitoring is disabled), where process termination would not correctly clear the audible state for a process. On all platforms this fixes a minor leak of AudioOutputController objects, since they are bound by ref-ptr into the callback map containing stale entries. I say minor since they would immediately be overwritten if the process is reusued or tossed completely when the WebContents object goes away. BUG=692857 TEST=new unittest, manual test: 1. load http://mounirlamouri.github.io/sandbox/autoplay/test.html 2. kill process from task manager. 3. click the reload button == boom (boom no longer after patch!). ==========
Description was changed from ========== Fix teardown of stale AudioStreamMonitor poll callbacks. It's possible for the RenderProcessHost and WebContents (and thus this class) to survive the death of the render process and subsequently be reused. During this period StartStopMonitoringHelper() will be unable to lookup the WebContents using the now-dead |render_frame_id|. We must thus have this secondary mechanism for clearing stale callbacks. This change introduces AudioStreamMonitor:RenderProcessGone() which is called by the WebContents whenever WC::RenderProcessGone() is. This allows the audio stream monitor to clear stale callbacks that the AudioOutputDelegateImpl will no longer be able to route to it. Subsequently this also fixes a bug on Android (and anywhere else power monitoring is disabled), where process termination would not correctly clear the audible state for a process. On all platforms this fixes a minor leak of AudioOutputController objects, since they are bound by ref-ptr into the callback map containing stale entries. I say minor since they would immediately be overwritten if the process is reusued or tossed completely when the WebContents object goes away. BUG=692857 TEST=new unittest, manual test: 1. load http://mounirlamouri.github.io/sandbox/autoplay/test.html 2. kill process from task manager. 3. click the reload button == boom (boom no longer after patch!). ========== to ========== Fix teardown of stale AudioStreamMonitor poll callbacks. It's possible for the RenderProcessHost and WebContents (and thus AudioStreamMonitor) to survive the death of the render process and subsequently be reused. During this period StartStopMonitoringHelper() will be unable to lookup the WebContents using the now-dead |render_frame_id|. We must thus have a secondary mechanism for clearing stale callbacks. This change introduces AudioStreamMonitor:RenderProcessGone() which is called by the WebContents whenever WCO::RenderProcessGone() is. This allows the audio stream monitor to clear stale callbacks that the AudioOutputDelegateImpl will no longer be able to route to it. Subsequently this also fixes a bug on Android (and anywhere else power monitoring is disabled), where process termination would not correctly clear the audible state for a process. On all platforms this fixes a minor leak of AudioOutputController objects, since they are bound by ref-ptr into the callback map containing stale entries. I say minor since they would immediately be overwritten if the process is reusued or tossed completely when the WebContents object goes away. BUG=692857 TEST=new unittest, manual test: 1. load http://mounirlamouri.github.io/sandbox/autoplay/test.html 2. kill process from task manager. 3. click the reload button == boom (boom no longer after patch!). ==========
Cool! Nice job figuring this out, I wish there was clear documentation on the relationship between rph, wc and rfh so we could avoid these bugs. lgtm.
https://codereview.chromium.org/2698813007/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2698813007/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:4401: audio_stream_monitor_.RenderProcessGone(rvh->GetProcess()->GetID()); Actually, this code confuses me. Shouldn't audio_stream_monitor_ be a RenderProcessHostObserver instead?
https://codereview.chromium.org/2698813007/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2698813007/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:4401: audio_stream_monitor_.RenderProcessGone(rvh->GetProcess()->GetID()); On 2017/02/17 at 16:39:53, Max Morin wrote: > Actually, this code confuses me. Shouldn't audio_stream_monitor_ be a RenderProcessHostObserver instead? Could be, but since WC is the only owner of this class it seemed clearer to have a single method versus inheriting everything from RPO. Defer to content/ folk on preference though.
Nice find! LGTM. https://codereview.chromium.org/2698813007/diff/1/content/browser/media/audio... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2698813007/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:26: // at time of call; e.g., in the event of a crash. Looks like this was already partly-noted here? :) https://codereview.chromium.org/2698813007/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2698813007/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:4401: audio_stream_monitor_.RenderProcessGone(rvh->GetProcess()->GetID()); On 2017/02/17 17:23:03, DaleCurtis wrote: > On 2017/02/17 at 16:39:53, Max Morin wrote: > > Actually, this code confuses me. Shouldn't audio_stream_monitor_ be a > RenderProcessHostObserver instead? > > Could be, but since WC is the only owner of this class it seemed clearer to have > a single method versus inheriting everything from RPO. Defer to content/ folk on > preference though. Yeah, I can see both sides, but I see the argument for handling it here. I'll suggest moving it up near the other special cases above, like below the HideValidationMessage call.
The CQ bit was checked by dalecurtis@chromium.org
Thanks for review! https://codereview.chromium.org/2698813007/diff/1/content/browser/media/audio... File content/browser/media/audio_stream_monitor.cc (right): https://codereview.chromium.org/2698813007/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.cc:26: // at time of call; e.g., in the event of a crash. On 2017/02/17 at 21:27:55, Charlie Reis wrote: > Looks like this was already partly-noted here? :) Yes, but when written I had assumed only the RenderProcessHost was reused, not the WebContents too :) https://codereview.chromium.org/2698813007/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2698813007/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:4401: audio_stream_monitor_.RenderProcessGone(rvh->GetProcess()->GetID()); On 2017/02/17 at 21:27:55, Charlie Reis wrote: > On 2017/02/17 17:23:03, DaleCurtis wrote: > > On 2017/02/17 at 16:39:53, Max Morin wrote: > > > Actually, this code confuses me. Shouldn't audio_stream_monitor_ be a > > RenderProcessHostObserver instead? > > > > Could be, but since WC is the only owner of this class it seemed clearer to have > > a single method versus inheriting everything from RPO. Defer to content/ folk on > > preference though. > > Yeah, I can see both sides, but I see the argument for handling it here. I'll suggest moving it up near the other special cases above, like below the HideValidationMessage call. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, maxmorin@chromium.org Link to the patchset: https://codereview.chromium.org/2698813007/#ps20001 (title: "Relocate RenderProcessGone() call.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Turns out I hadn't really fixed the Android issue, maxmorin@ PTAL. We now always store poll callbacks and use them to compute the number of active streams. I've updated the tests so we get coverage for when polling is and isn't supported.
On 2017/02/18 01:03:30, DaleCurtis wrote: > Turns out I hadn't really fixed the Android issue, maxmorin@ PTAL. We now always > store poll callbacks and use them to compute the number of active streams. I've > updated the tests so we get coverage for when polling is and isn't supported. Oh, I should've caught this during review :). lgtm.
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2698813007/#ps40001 (title: "Always store callbacks, fix tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487625874669630,
"parent_rev": "1bdc716d3d2537161e1b0b5f86e8f207bda3c727", "commit_rev":
"3d1e1301ee8f7c1cab24e2d161218336ea0e7b7c"}
Message was sent while issue was closed.
Description was changed from ========== Fix teardown of stale AudioStreamMonitor poll callbacks. It's possible for the RenderProcessHost and WebContents (and thus AudioStreamMonitor) to survive the death of the render process and subsequently be reused. During this period StartStopMonitoringHelper() will be unable to lookup the WebContents using the now-dead |render_frame_id|. We must thus have a secondary mechanism for clearing stale callbacks. This change introduces AudioStreamMonitor:RenderProcessGone() which is called by the WebContents whenever WCO::RenderProcessGone() is. This allows the audio stream monitor to clear stale callbacks that the AudioOutputDelegateImpl will no longer be able to route to it. Subsequently this also fixes a bug on Android (and anywhere else power monitoring is disabled), where process termination would not correctly clear the audible state for a process. On all platforms this fixes a minor leak of AudioOutputController objects, since they are bound by ref-ptr into the callback map containing stale entries. I say minor since they would immediately be overwritten if the process is reusued or tossed completely when the WebContents object goes away. BUG=692857 TEST=new unittest, manual test: 1. load http://mounirlamouri.github.io/sandbox/autoplay/test.html 2. kill process from task manager. 3. click the reload button == boom (boom no longer after patch!). ========== to ========== Fix teardown of stale AudioStreamMonitor poll callbacks. It's possible for the RenderProcessHost and WebContents (and thus AudioStreamMonitor) to survive the death of the render process and subsequently be reused. During this period StartStopMonitoringHelper() will be unable to lookup the WebContents using the now-dead |render_frame_id|. We must thus have a secondary mechanism for clearing stale callbacks. This change introduces AudioStreamMonitor:RenderProcessGone() which is called by the WebContents whenever WCO::RenderProcessGone() is. This allows the audio stream monitor to clear stale callbacks that the AudioOutputDelegateImpl will no longer be able to route to it. Subsequently this also fixes a bug on Android (and anywhere else power monitoring is disabled), where process termination would not correctly clear the audible state for a process. On all platforms this fixes a minor leak of AudioOutputController objects, since they are bound by ref-ptr into the callback map containing stale entries. I say minor since they would immediately be overwritten if the process is reusued or tossed completely when the WebContents object goes away. BUG=692857 TEST=new unittest, manual test: 1. load http://mounirlamouri.github.io/sandbox/autoplay/test.html 2. kill process from task manager. 3. click the reload button == boom (boom no longer after patch!). Review-Url: https://codereview.chromium.org/2698813007 Cr-Commit-Position: refs/heads/master@{#451664} Committed: https://chromium.googlesource.com/chromium/src/+/3d1e1301ee8f7c1cab24e2d16121... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3d1e1301ee8f7c1cab24e2d16121... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
