Crash: RenderProcessHost media stream counts wrong |
||||||||
Issue descriptionFrom miu@ in https://chromium-review.googlesource.com/c/chromium/src/+/1232475: If the renderer crashes, this RenderProcessGone() method will be called, which calls RPHImpl::OnMediaStreamRemoved(). However, it does not set is_audible_ to false, and so when the AudioStreamMonitor later calls into RFHImpl::OnAudibleStateChanged(), the call to OnMediaStreamRemoved() will be duplicated. RPHImpl will be decrementing its media_stream_count_ twice! Relevant logs (what led me to investigate this in the first place): [23096:23096:0918/122322.118583:FATAL:render_process_host_impl.cc(2668)] Check failed: media_stream_count_ > 0 (0 vs. 0) #0 0x7fe453b7da2c base::debug::StackTrace::StackTrace() #1 0x7fe453aa7f1b logging::LogMessage::~LogMessage() #2 0x7fe4511ee6f2 content::RenderProcessHostImpl::OnMediaStreamRemoved() #3 0x7fe450fcae58 content::RenderFrameHostImpl::OnAudibleStateChanged() #4 0x7fe4510db3c0 content::AudioStreamMonitor::UpdateStreams() #5 0x7fe4510db9e9 content::AudioStreamMonitor::StopMonitoringStreamOnUIThread() #6 0x7fe453a88aed base::debug::TaskAnnotator::RunTask() #7 0x7fe453ab5a3e base::MessageLoop::RunTask() ...
,
Sep 22
Requesting merge, but definitely some things for the Release PM to consider before approval: 1. I only discovered this because of a DCHECK() failure. This bug would not cause a browser crash in a release build. 2a. First performance concern: Without this fix, there are some cases where a render process will be permanently lowered in priority (after any media playback or video capture has occurred). To the user, they may occasionally find web browsing performance to be sluggish; with lower-end hardware much more-often experiencing the sluggishness. 2b. Second performance concern: Without this fix, there are some cases where a render process will never be lowered in priority. To the user, this will look like high CPU and power usage, even on their non-active browser tabs. 3. History: This bug has existed since M69 (1 Chrome release). 4. Safety of fix: Very safe. The new code both fixes the crash and, structurally, guarantees the desired behavior will happen in all scenarios. 5. Other: This fix may turn out to fix open performance bugs that other developer's are having a hard time reproducing and/or root-causing.
,
Sep 22
The bug is marked as P3 or Feature. It should not be merged as M70 is in beta. Please contact the approriate milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 24
The auto-reject was arbitrary because no one adjusted the priority label. Given miu@'s description in comment 2 and how this bug can affect process priority, I'll bump up the priority and restart the merge discussion. gab@ recently found massive performance improvements are possible when we fix incorrect process priority issues (e.g., issue 560446 ), so it may be worth considering this for merge to M70. gab@: Would you be able to check whether there have been any observable performance improvements since r593354 landed in 71.0.3559.0? I imagine it's too soon to check today, but maybe tomorrow? Or perhaps it's worth considering for merge regardless, given that we know it will cause unexpected performance issues?
,
Sep 24
,
Sep 24
Thank you so much for more details. It's great to see so many performance benefits, so I'm approving the merge for M70. However, gab@ can you please check creis's request in #4 and can we monitor this week's beta closely for performance?
,
Sep 24
Re. #4 : I suggest looking at https://uma.googleplex.com/p/chrome/timeline_v2 for metrics you think may have budged and looking at the 99th or even 99.9th percentile to see if you fixed a priority inversion making things really bad sometimes. For potentially relevant metrics you may want to refer to : https://groups.google.com/a/google.com/d/msg/chrome-catan/i1HhL0Sf4wA/jwNo4kXDEAAJ
,
Sep 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b7785314254cb5affe04f548fadc3965b74f8c6 commit 3b7785314254cb5affe04f548fadc3965b74f8c6 Author: Yuri Wiitala <miu@chromium.org> Date: Tue Sep 25 01:54:49 2018 Crash fix: RenderProcessHostImpl media stream counts. Multiple fixes around RenderFrameHostImpl not always notifying its RenderProcessHostImpl that audio streams have been added/removed. Before these fixes, RenderProcessHostImpl was sometimes leaving a render process backgrounded indefinitely, or sometimes leaving it non-backgrounded indefinitely. (cherry picked from commit 04bf53f77d39ae62e107941c9a850cbca565efaa) Bug: 887208 Change-Id: I1713887c96382b8bdf032c82bb49723fc0219f71 Reviewed-on: https://chromium-review.googlesource.com/1232475 Commit-Queue: Yuri Wiitala <miu@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#593354} Reviewed-on: https://chromium-review.googlesource.com/1242399 Reviewed-by: Yuri Wiitala <miu@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#639} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/3b7785314254cb5affe04f548fadc3965b74f8c6/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/3b7785314254cb5affe04f548fadc3965b74f8c6/content/browser/frame_host/render_frame_host_impl_browsertest.cc
,
Sep 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b7785314254cb5affe04f548fadc3965b74f8c6 Commit: 3b7785314254cb5affe04f548fadc3965b74f8c6 Author: miu@chromium.org Commiter: miu@chromium.org Date: 2018-09-25 01:54:49 +0000 UTC Crash fix: RenderProcessHostImpl media stream counts. Multiple fixes around RenderFrameHostImpl not always notifying its RenderProcessHostImpl that audio streams have been added/removed. Before these fixes, RenderProcessHostImpl was sometimes leaving a render process backgrounded indefinitely, or sometimes leaving it non-backgrounded indefinitely. (cherry picked from commit 04bf53f77d39ae62e107941c9a850cbca565efaa) Bug: 887208 Change-Id: I1713887c96382b8bdf032c82bb49723fc0219f71 Reviewed-on: https://chromium-review.googlesource.com/1232475 Commit-Queue: Yuri Wiitala <miu@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#593354} Reviewed-on: https://chromium-review.googlesource.com/1242399 Reviewed-by: Yuri Wiitala <miu@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#639} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Sep 25
The NextAction date has arrived: 2018-09-25 |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bugdroid1@chromium.org
, Sep 21