TabManager: Set OOM scores via a new debugd interface on ChromeOS.

TabManager: Set OOM scores via a new debugd interface on ChromeOS.

Setting OOM scores generally requires root permission. In the past it's done
1. For Chrome processes: via a SUID binary chrome-sandbox.
2. For Andorid processes: Send scores to Android via IPC. Android
handles the rest.

Now a new method is added to debugd dbus interface on ChromeOS to
handle OOM score settings.
https://chromium-review.googlesource.com/#/c/366154/

So we can set OOM score settings in the unified way on ChromeOS.

BUG=b:30193036
TEST=minnie

Committed: https://crrev.com/2af9aaceb40213973d0c989de413d0505afefbce
Cr-Commit-Position: refs/heads/master@{#414693}

[cylee1, 2016-08-12 17:28:40]

The CQ bit was checked by cylee@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 17:29:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cylee1, 2016-08-12 17:34:38]

cylee@chromium.org changed reviewers:
+ chrisha@chromium.org, georgesak@chromium.org, hashimoto@chromium.org,
stevenjb@chromium.org

[cylee1, 2016-08-12 17:34:39]

Hi Chris,
  PTAL since George seems to be OOO?

[commit-bot: I haz the power, 2016-08-12 17:53:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 17:53:28]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[stevenjb, 2016-08-12 23:30:18]

chromeos/dbus/ lgtm

[hashimoto, 2016-08-15 03:54:23]

https://codereview.chromium.org/2247433002/diff/1/chrome/browser/memory/tab_m...
File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right):

https://codereview.chromium.org/2247433002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:779:
chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->SetOomScoreAdj(
D-Bus clients are not thread-safe.
They must be accessed only on the UI thread.

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
File chromeos/dbus/debug_daemon_client.cc (right):

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.cc:468: dbus::MessageWriter sub_writer(NULL);
Please declare this variable in as local a scope as possible, and as close to
the first use as possible.
(https://google.github.io/styleguide/cppguide.html#Local_Variables)

Also, please use nullptr.

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.cc:469: dbus::MessageWriter elem_writer(NULL);
ditto.

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.cc:750: if (callback.is_null())
Is there any strong reason to allow null callbacks?
If not, please remove this code.

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
File chromeos/dbus/debug_daemon_client.h (right):

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.h:195: // A callback for SetOOmScoreAdj().
nit: s/OOm/Oom/

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.h:200: // Note that the API only permits
setting OOM score for processes running by
Please make it clear what "the API" here means. (is it debugd's API, or this
class's?)

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.h:202: virtual void SetOomScoreAdj(const
std::map<int, int>& pairs,
From the name "pairs", it's not clear what the values in this map mean.
Please give this argument a more descriptive name (e.g. pid_to_oom_score_adj)

[Georges Khalil, 2016-08-15 18:47:54]

Just a couple of nits.

https://codereview.chromium.org/2247433002/diff/1/chrome/browser/memory/tab_m...
File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right):

https://codereview.chromium.org/2247433002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:87: if (!success ||
output != "") {
nit: !output.empty()
nit: no braces

[cylee1, 2016-08-16 19:47:11]

https://codereview.chromium.org/2247433002/diff/1/chrome/browser/memory/tab_m...
File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right):

https://codereview.chromium.org/2247433002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:87: if (!success ||
output != "") {
On 2016/08/15 18:47:54, Georges Khalil (OOO Sep 7) wrote:
> nit: !output.empty()
> nit: no braces

Done.

https://codereview.chromium.org/2247433002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:779:
chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->SetOomScoreAdj(
On 2016/08/15 03:54:23, hashimoto wrote:
> D-Bus clients are not thread-safe.
> They must be accessed only on the UI thread.

Done.

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
File chromeos/dbus/debug_daemon_client.cc (right):

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.cc:468: dbus::MessageWriter sub_writer(NULL);
On 2016/08/15 03:54:23, hashimoto wrote:
> Please declare this variable in as local a scope as possible, and as close to
> the first use as possible.
> (https://google.github.io/styleguide/cppguide.html#Local_Variables)
> 
> Also, please use nullptr.

Done.

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.cc:469: dbus::MessageWriter elem_writer(NULL);
On 2016/08/15 03:54:23, hashimoto wrote:
> ditto.

Done.

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.cc:750: if (callback.is_null())
On 2016/08/15 03:54:23, hashimoto wrote:
> Is there any strong reason to allow null callbacks?
> If not, please remove this code.

Done.

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
File chromeos/dbus/debug_daemon_client.h (right):

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.h:195: // A callback for SetOOmScoreAdj().
On 2016/08/15 03:54:23, hashimoto wrote:
> nit: s/OOm/Oom/

Done.

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.h:200: // Note that the API only permits
setting OOM score for processes running by
On 2016/08/15 03:54:23, hashimoto wrote:
> Please make it clear what "the API" here means. (is it debugd's API, or this
> class's?)

Done.

https://codereview.chromium.org/2247433002/diff/1/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.h:202: virtual void SetOomScoreAdj(const
std::map<int, int>& pairs,
On 2016/08/15 03:54:23, hashimoto wrote:
> From the name "pairs", it's not clear what the values in this map mean.
> Please give this argument a more descriptive name (e.g. pid_to_oom_score_adj)

Done.

[hashimoto, 2016-08-17 02:48:47]

chromeos/dbus lgtm

https://codereview.chromium.org/2247433002/diff/40001/chromeos/dbus/fake_debu...
File chromeos/dbus/fake_debug_daemon_client.cc (right):

https://codereview.chromium.org/2247433002/diff/40001/chromeos/dbus/fake_debu...
chromeos/dbus/fake_debug_daemon_client.cc:203: const std::map<int, int>& scores,
Please use the same argument name as used in debug_daemon_client.h.

https://codereview.chromium.org/2247433002/diff/40001/chromeos/dbus/fake_debu...
File chromeos/dbus/fake_debug_daemon_client.h (right):

https://codereview.chromium.org/2247433002/diff/40001/chromeos/dbus/fake_debu...
chromeos/dbus/fake_debug_daemon_client.h:72: void SetOomScoreAdj(const
std::map<int, int>& scores,
Please use the same argument name as used in debug_daemon_client.h.

[Georges Khalil, 2016-08-18 16:20:46]

lgtm % nits.

https://codereview.chromium.org/2247433002/diff/40001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right):

https://codereview.chromium.org/2247433002/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:189: // Checks whether
the containing instance is an ARC  app. If so it resets the
nit: triple space between ARC & app.

https://codereview.chromium.org/2247433002/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:465: // just switched to
an ARC app before the timeout. We can not
nit: formatting.

https://codereview.chromium.org/2247433002/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:608: app.process_name()
== kArcInBackgroundDummyprocess)
nit: braces.

[cylee1, 2016-08-18 22:16:26]

Thanks to Georges for the quick review when in vacation !

https://codereview.chromium.org/2247433002/diff/40001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_delegate_chromeos.cc (right):

https://codereview.chromium.org/2247433002/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:189: // Checks whether
the containing instance is an ARC  app. If so it resets the
On 2016/08/18 16:20:45, Georges Khalil (OOO Sep 7) wrote:
> nit: triple space between ARC & app.

Done.

https://codereview.chromium.org/2247433002/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:465: // just switched to
an ARC app before the timeout. We can not
On 2016/08/18 16:20:45, Georges Khalil (OOO Sep 7) wrote:
> nit: formatting.

Done.

https://codereview.chromium.org/2247433002/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_delegate_chromeos.cc:608: app.process_name()
== kArcInBackgroundDummyprocess)
On 2016/08/18 16:20:45, Georges Khalil (OOO Sep 7) wrote:
> nit: braces.

Done.

https://codereview.chromium.org/2247433002/diff/40001/chromeos/dbus/fake_debu...
File chromeos/dbus/fake_debug_daemon_client.cc (right):

https://codereview.chromium.org/2247433002/diff/40001/chromeos/dbus/fake_debu...
chromeos/dbus/fake_debug_daemon_client.cc:203: const std::map<int, int>& scores,
On 2016/08/17 02:48:46, hashimoto wrote:
> Please use the same argument name as used in debug_daemon_client.h.

Done.

https://codereview.chromium.org/2247433002/diff/40001/chromeos/dbus/fake_debu...
File chromeos/dbus/fake_debug_daemon_client.h (right):

https://codereview.chromium.org/2247433002/diff/40001/chromeos/dbus/fake_debu...
chromeos/dbus/fake_debug_daemon_client.h:72: void SetOomScoreAdj(const
std::map<int, int>& scores,
On 2016/08/17 02:48:47, hashimoto wrote:
> Please use the same argument name as used in debug_daemon_client.h.

Done.

[cylee1, 2016-08-24 22:21:00]

The CQ bit was checked by cylee@chromium.org

[cylee1, 2016-08-24 22:21:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, hashimoto@chromium.org, georgesak@chromium.org
Link to the patchset: https://codereview.chromium.org/2247433002/#ps80001
(title: "minor change: pass score as int32_t to match the receiver side")

[commit-bot: I haz the power, 2016-08-24 22:22:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 22:39:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 22:40:00]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cylee1, 2016-08-25 19:12:07]

The CQ bit was checked by cylee@chromium.org

[commit-bot: I haz the power, 2016-08-25 19:13:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 20:11:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 20:11:02]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cylee1, 2016-08-26 12:20:08]

The CQ bit was checked by cylee@chromium.org

[cylee1, 2016-08-26 12:20:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
hashimoto@chromium.org, stevenjb@chromium.org, georgesak@chromium.org
Link to the patchset: https://codereview.chromium.org/2247433002/#ps100001
(title: "fix unittest")

[commit-bot: I haz the power, 2016-08-26 12:20:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 13:04:48]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-26 13:06:19]

Description was changed from

==========
TabManager: Set OOM scores via a new debugd interface on ChromeOS.

Setting OOM scores generally requires root permission. In the past it's done
1. For Chrome processes: via a SUID binary chrome-sandbox.
2. For Andorid processes: Send scores to Android via IPC. Android
handles the rest.

Now a new method is added to debugd dbus interface on ChromeOS to
handle OOM score settings.
https://chromium-review.googlesource.com/#/c/366154/

So we can set OOM score settings in the unified way on ChromeOS.

BUG=b:30193036
TEST=minnie
==========

to

==========
TabManager: Set OOM scores via a new debugd interface on ChromeOS.

Setting OOM scores generally requires root permission. In the past it's done
1. For Chrome processes: via a SUID binary chrome-sandbox.
2. For Andorid processes: Send scores to Android via IPC. Android
handles the rest.

Now a new method is added to debugd dbus interface on ChromeOS to
handle OOM score settings.
https://chromium-review.googlesource.com/#/c/366154/

So we can set OOM score settings in the unified way on ChromeOS.

BUG=b:30193036
TEST=minnie

Committed: https://crrev.com/2af9aaceb40213973d0c989de413d0505afefbce
Cr-Commit-Position: refs/heads/master@{#414693}
==========

[commit-bot: I haz the power, 2016-08-26 13:06:20]

Patchset 6 (id:??) landed as
https://crrev.com/2af9aaceb40213973d0c989de413d0505afefbce
Cr-Commit-Position: refs/heads/master@{#414693}