In the past (prior to r583378) the hang renderer dialog would only be shown if an input event (mouse/keyboard/etc event) wouldn't be acked by a renderer within 30s timeout.  I believe the input events would only be delivered to foreground tabs.

After r583378 (which landed in 70.0.3524.0) the hang renderer dialog can be also shown for navigations that take longer than 30s to commit (note r586053 which increased the commit timeout from 10s to 30s in 70.0.3533.0).  Commits can happen in background tabs and I agree that we should treat them differently (either increase the commit timeout or avoid showing the dialog altogether).

Adding Internals>Sandbox>SiteIsolation label since the navigation commit timeout was introduced to surface hangs related to navigating an OOPIF hosted in a hang renderer.

This bug is specifically about hang reports (i.e. the thread watchdog), but thanks for chiming in as what you described is something I've also been digging into @  issue 880997  (all yours!)

I assume that the right way to detect whether a process has been backgrounded is RenderProcessHost::IsProcessBackgrounded.

I can try to figure out how to tweak hang-renderer-notifications based on RenderProcessHost::IsProcessBackgrounded - I think for background processes we can return early after running the |hang_monitor_restarter| (running this callback will make us recheck the background status after the next timeout period).

I am not yet sure if want to consult RenderProcessHost::IsProcessBackgrounded somewhere in //content layer (e.g. in WebContentsImpl::RendererUnresponsive) or somewhere in //chrome layer (e.g. in Browser::RendererUnresponsive).  I am leaning toward the latter, since Browser::RendererUnresponsive already covers IsTabBlocked.  FWIW, I see that there already is WebContentsImpl::ShouldIgnoreUnresponsiveRenderer, but I am not sure if we can/should extend it to cover background processes, because ShouldIgnoreUnresponsiveRenderer is also used in RenderFrameHostImpl::BeforeUnloadTimeout and RenderViewHostImpl::ClosePageTimeout and I don't think we want to tweak *these* timeouts based on the background/foreground status (do we?).

I am not sure if I know how to add a regression test (since I don't see an easy, robust way to control renderer priority from tests).  Any hints/suggestions for writing a test are welcomed.

Actually, we should ideally "foreground" background tabs when on unload/close (otherwise we have a shutdown priority inversion). So then we could use WebContentsImpl::ShouldIgnoreUnresponsiveRenderer?

See ChromeRenderProcessHostBackgroundingTest.MultipleTabs for an example backgrounding test.

Thanks for picking this up!

> Actually, we should ideally "foreground" background tabs when on unload/close
> (otherwise we have a shutdown priority inversion). So then we could use
> WebContentsImpl::ShouldIgnoreUnresponsiveRenderer?

My ad-hoc tests indicate that we don't "foreground" background tabs on unload/close - I've opened issue 883468 to investigate this further.  Therefore I'll probably move forward with a tweak in Browser::RendererUnresponsive.

Also - is it really best to use RenderProcessHost::IsProcessBackgrounded as input for deciding whether to suppress the hung-renderer-dialog?  The relationship between "user is interested in slowness of this WebContents" and "this WebContents's main frame is hosted in a foreground-priority process" doesn't seem very well established to me - for example:
- Would extension background pages run with background or foreground priority?  Even if today they run with background priority, we should be able to tweak this without influencing when the hung-renderer-dialog is shown.
- Would we ever consider running invisible OOPIFs in a background process?  Would the user still want to know about slow commits if such OOPIFs are part of a foreground tab?

I am wondering whether we should consider using WebContents::GetVisibility instead.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/54d75db2a274d03cc90584ba6b5afaa7fc1ef959

commit 54d75db2a274d03cc90584ba6b5afaa7fc1ef959
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Sep 13 23:43:05 2018

Split navigation-timing UMAs based on whether renderer is backgrounded.

This CL adds extra suffixes for navigation-timing UMAs, based on whether
the renderer handling the navigation is backgrounded or not.

Since we plan to restrict the hang-renderer-dialog to only show up for
foreground tabs, the navigation commit timeout should be based on
numbers that exclude backgrounded renderers.  The new UMAs should help
with getting such numbers.

Bug:  881812 
Change-Id: I68fd4f83dbb09ee036d1a4fd23591c8255c66398
Reviewed-on: https://chromium-review.googlesource.com/1220470
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591213}
[modify] https://crrev.com/54d75db2a274d03cc90584ba6b5afaa7fc1ef959/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/54d75db2a274d03cc90584ba6b5afaa7fc1ef959/content/browser/frame_host/navigation_handle_impl_browsertest.cc
[modify] https://crrev.com/54d75db2a274d03cc90584ba6b5afaa7fc1ef959/tools/metrics/histograms/histograms.xml

One more argument for special-casing background tabs *above* WebContentsImpl:
*) some //chrome-layer handlers (like hung-renderer-dialog) want to ignore unresponsive background tabs
*) some //chrome-layer handlers (like task manager) want to continue processing unresponsive background tabs (e.g. annotate their tasks).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bbc55f660b075b69a081a7715eeac440ac3025b5

commit bbc55f660b075b69a081a7715eeac440ac3025b5
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Sep 14 16:14:43 2018

Enable ChromeRenderProcessHostBackgroundingTest.MultipleTabs on Linux.

While |base::Process::CanBackgroundProcesses()| returns false on Linux,
we are still able to verify whether processes are backgrounded by
looking at RenderProcessHost::IsProcessBackgrounded().  This
verification is not as thorough/end-to-end as what we can do on other
platforms, but it is sufficient to enable
ChromeRenderProcessHostBackgroundingTest.MultipleTabs test on Linux.

Bug:  881812 , 883468
Change-Id: I536ea10a279d7138d632b8846dc2a0d8e507c001
Reviewed-on: https://chromium-review.googlesource.com/1222249
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591363}
[modify] https://crrev.com/bbc55f660b075b69a081a7715eeac440ac3025b5/chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc

Re. #10 : that's a fair point. Ideally priority should match the user's interest already.
 - Extensions currently (unfortunately) always run foreground.
 - A process responsible for OOPIFs will only ever be backgrounded if all its IFs are invisible. So I don't think this is a problem.

Overall the heuristics for determining importance to the user / priority / etc. are in the wrong place. We're looking to move that logic to a service driven by TabManager (with full knowledge of the tab strip / frame associations).

WebContents::GetVisibility() misses use cases like audio and casting which the current model is trying to take into account. I think rederiving yet another heuristic for what "matters" would bring us further from our goal to have a single source of truth for such logic.

 Issue 880997  has been merged into this issue.

Copying labels from  issue 880997 

I've talked with gab@ yesterday and:

1. We agreed that WebContents::GetVisibility is the right signal for surfacing hangs to the user.

    1.a. In particular, we agreed that we should not surface slowness in some  foreground-priority WebContents like extension-background-page or background-tab-from-the-same-browsing-instance-as-foreground-tab.  In both cases the hung renderer dialog would be disruptive (it disrupts user's interactions with the visible tab) and we expect that users would be confused by presence/information about extension background page.

    1.b. We also agreed that we should block reporting of hang renderers through both 1) hung renderer dialog and 2) task manager.  Therefore, it seems to make sense to block the reporting in //content layer (e.g. in WebContentsImpl) rather than in the //chrome layer.

2. In theory it should be possible to write a regression test (similar to CommitTimeoutForHungRenderer), but testing for *absence* of notification is tricky and sensitive to time (i.e. how long should the test wait to conclude that the notification did not happen).  I am still wondering how to write the test (or whether to write it at all).

This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label.

All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This issue applies to platforms where Site Isolation is turned on by default.

Re. #17.1. : I guess "visibility" as well as "hearability" (audio) matter in practice. "casting" also matters. That's why being at "foregrounded" priority is the ideal proxy. Unfortunately there are some things that run "foregrounded" that the user shouldn't have to worry about (e.g. extension background pages).

So the priority should be :
 1) Definitely don't report hangs for things that we intentionally "background".
 2) Attempt not to report hangs for things that the user doesn't care about.

(1) is the release blocker IMO. We can use "visibility" for (2) initially but IMO that will result in under reporting audible/casting but invisible web contents... We should at least ensure that those do report as hung when foregrounded at least (e.g. if user focuses to check "what's going on").

Currently there are only 2 mechanisms for detecting that a renderer is hung:
1. timeout for acking an input event
2. timeout for committing a navigation

If we want to detect hangouts in an audio-playing (or casting) background tab (that doesn't get input events because it is in the background + that doesn't navigate), then we'd need a separate work item / bug for this.


I think we've agreed yesterday that we want to be conservative for M70 (i.e. avoid over-reporting / make reporting as close to pre-M70 behavior as possible) and that using WebContents::GetVisibility as a signal will result in the desired conservative behavior.  Post-M70 we can revisit both 1) the heuristics used to decide whether to report a hang and 2) the signals used to detect a hung.

SGTM (hadn't realized the new heuristics already didn't apply to audio/casting tabs so visibility is good proxy for M70 and a strict improvement over M69 which didn't take navigation into account at all)

WIP CL @ https://crrev.com/c/1222684

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f0593dd0c181f83ebb1a2faed61997b1250e180

commit 4f0593dd0c181f83ebb1a2faed61997b1250e180
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Sep 21 18:33:54 2018

Don't show hung-renderer-dialog for background tabs.

Bug:  881812 
Change-Id: I7e9eddb4386baac05e9292463f9f597132c5c14e
Reviewed-on: https://chromium-review.googlesource.com/1222684
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593270}
[modify] https://crrev.com/4f0593dd0c181f83ebb1a2faed61997b1250e180/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/4f0593dd0c181f83ebb1a2faed61997b1250e180/content/browser/web_contents/web_contents_impl.cc

adding a merge TBD label - if it looks good in canary, we should consider merging to M70 for next week. 

How does this look in today's canary?

abdulsyed@, I think we should consider merging r593270 to M70:

*) The current experience in M70 seems worth fixing: 1) the hung-renderer-dialog is relatively likely to  appear for a background tab during session restore (when many background tabs are present and are actively navigating their frames) and 2) the user experience (of having foreground tab browsing interrupted by the hung-renderer-dialog for an unrelated background tab) is pretty jarring.

*) The CL is pretty simple (touches implementation of one method - WebContentsImpl::RendererUnresponsive).

*) I've looked at Crash reports from 71.0.3559.0 (where r593270 landed) and beyond [1] and I don't see any crashes that would be obviously related to the CL from #c25.

*) The only potential negative impact from the CL would be getting less reports about unresponsive renderers (visible both in Crash and via hung-renderer-dialog shown to end users).  AFAICT the CL hides only reports where things are working as expected (i.e. for background tabs which expectedly run with lower priority), but even if the CL hid other reports, I think the impact would be pretty low.


[1] https://crash.corp.google.com/browse?q=product_name%3D%27Chrome%27+AND+product.Version%3E%3D%2771.0.3559.0%27+AND+expanded_custom_data.ChromeCrashProto.ptype%3D%27browser%27

This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review
Please contact the 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

Thank you lukasza@ - Regarding negative impact, I'm wondering what the right approach should be for measuring unresponsive renderers. Is this something that should be measured through a different metric, and not renderer crashes?

Re. #28/30 :
 * Strong +1 to merging this to M70 (I don't think we should ship without and that's why I originally marked RB-S, in particular since this same CL is also a fix for  issue 880997 ).
 * I don't think last point is "negative", it will be inline with M69 and more correct (and yes we want hangs to be reported through the crash machinery on top of just metrics as minidumps have more information -- this has long been the case)

Sounds good - thanks, approving for M70. Branch:3538

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/23dc578db938461ec846a0815aa7fa64cf5033b9

commit 23dc578db938461ec846a0815aa7fa64cf5033b9
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Mon Sep 24 18:36:56 2018

Don't show hung-renderer-dialog for background tabs.

TBR=lukasza@chromium.org

(cherry picked from commit 4f0593dd0c181f83ebb1a2faed61997b1250e180)

Bug:  881812 
Change-Id: I7e9eddb4386baac05e9292463f9f597132c5c14e
Reviewed-on: https://chromium-review.googlesource.com/1222684
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#593270}
Reviewed-on: https://chromium-review.googlesource.com/1240734
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#593}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/23dc578db938461ec846a0815aa7fa64cf5033b9/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/23dc578db938461ec846a0815aa7fa64cf5033b9/content/browser/web_contents/web_contents_impl.cc

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/23dc578db938461ec846a0815aa7fa64cf5033b9

Commit: 23dc578db938461ec846a0815aa7fa64cf5033b9
Author: lukasza@chromium.org
Commiter: lukasza@chromium.org
Date: 2018-09-24 18:36:56 +0000 UTC

Don't show hung-renderer-dialog for background tabs.

TBR=lukasza@chromium.org

(cherry picked from commit 4f0593dd0c181f83ebb1a2faed61997b1250e180)

Bug:  881812 
Change-Id: I7e9eddb4386baac05e9292463f9f597132c5c14e
Reviewed-on: https://chromium-review.googlesource.com/1222684
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#593270}
Reviewed-on: https://chromium-review.googlesource.com/1240734
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#593}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}