Issue metadata
Sign in to add a comment
|
Chrome OS: Chrome's OOM killer writes unnecessary error logs |
||||||||||||||||||||||
Issue descriptionChrome Version: M60 tot, Chrome OS only. What steps will reproduce the problem? (1) Follow e.g. https://bugs.chromium.org/p/chromium/issues/detail?id=706048#c76 to trigger Chrome's OOM kill with lots of new tab pages. (2) Check /var/log/ui/ui.LATEST file. What is the expected result? No erroneous logs there. What happens instead? There are lots of "Failed to kill ..." lines: [26417:26417:0511/160951.962294:ERROR:device_event_log_impl.cc(156)] [16:09:51.962] Memory: tab_manager_delegate_chromeos.cc:640 Failed to kill tab 0, process_type BACKGROUND_TAB [26417:26417:0511/160951.962455:ERROR:device_event_log_impl.cc(156)] [16:09:51.962] Memory: tab_manager_delegate_chromeos.cc:640 Failed to kill tab 0, process_type BACKGROUND_TAB [26417:26417:0511/160951.965759:ERROR:device_event_log_impl.cc(156)] [16:09:51.965] Memory: tab_manager_delegate_chromeos.cc:640 Failed to kill tab 29525, process_type BACKGROUND_TAB Please use labels and text to provide additional information. This is because TabManagerDelegate::LowMemoryKillImpl() in chrome/browser/memory/tab_manager_delegate_chromeos.cc tries to kill Chrome tabs even when some of the tabs share the same renderer process. We should modify the function so that it never tries to kill the same renderer process twice (or more.)
,
May 12 2017
It looks like TabStats has a is_discarded field that we could use to filter out discarded tabs inside TabManagerDelegate::LowMemoryKillImpl().
,
May 12 2017
#4 Thanks for the tip. It looks like a shortcut for this issue. But checking it again, KillTab() https://cs.chromium.org/chromium/src/chrome/browser/memory/tab_manager_delegate_chromeos.cc?rcl=38f94f533d809b4202db39ac8c17be4442eb4256&l=570 returns false if 1) CanDiscardTab() returns false 2) DiscardTabById() returns false. Examining the two functions, they return false for valid cases (e.g., active tab, already discarded tab, tab playing audio, etc). They basically don't report real error we're interested in. It means maybe I should just remove the log line ? MEMORY_LOG(ERROR) << "Failed to kill " << *it; Any objection? Another extreme is to print the reason why a tab couldn't be discarded. But I think it's too informative for us.
,
May 12 2017
I think we should just remove the "failed to kill" log lines for now.
,
May 12 2017
I think we should merge this back to M59?
,
May 12 2017
oops, I should request mergeback after https://codereview.chromium.org/2879673004/ has landed.
,
May 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9c5c339f64135eedc7af11876f52a23fda402b15 commit 9c5c339f64135eedc7af11876f52a23fda402b15 Author: cylee <cylee@chromium.org> Date: Fri May 12 22:36:51 2017 TabManager: Remove a redundant log line which produces excessive log. KillTab() returns false on valid cases, so useless to report it. BUG= chromium:721632 TEST=manual Review-Url: https://codereview.chromium.org/2879673004 Cr-Commit-Position: refs/heads/master@{#471470} [modify] https://crrev.com/9c5c339f64135eedc7af11876f52a23fda402b15/chrome/browser/memory/tab_manager_delegate_chromeos.cc
,
May 15 2017
,
May 15 2017
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1dbb725b409a99fb67d9250618c7ccdc92aafc07 commit 1dbb725b409a99fb67d9250618c7ccdc92aafc07 Author: Cheng-Yu Lee <cylee@chromium.org> Date: Tue May 16 17:58:36 2017 TabManager: Remove a redundant log line which produces excessive log. KillTab() returns false on valid cases, so useless to report it. BUG= chromium:721632 TEST=manual Review-Url: https://codereview.chromium.org/2879673004 Cr-Original-Commit-Position: refs/heads/master@{#471470} Review-Url: https://codereview.chromium.org/2884363002 . Cr-Commit-Position: refs/branch-heads/3071@{#586} Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641} [modify] https://crrev.com/1dbb725b409a99fb67d9250618c7ccdc92aafc07/chrome/browser/memory/tab_manager_delegate_chromeos.cc
,
May 16 2017
Merged to M59
,
Jan 22 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 Deleted