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

Review-Url: https://codereview.chromium.org/2247433002
Cr-Commit-Position: refs/heads/master@{#414693}

chrome/browser/memory/tab_manager_delegate_chromeos.h

@@ -20,6 +20,7 @@
 #include "chrome/browser/memory/tab_manager.h"
 #include "chrome/browser/memory/tab_stats.h"
 #include "chrome/browser/ui/browser_list_observer.h"
+#include "chromeos/dbus/debug_daemon_client.h"
 #include "components/arc/arc_bridge_service.h"
 #include "components/arc/common/process.mojom.h"
 #include "components/arc/instance_holder.h"
@@ -46,10 +47,6 @@ enum class ProcessType {
 
 // The Chrome OS TabManagerDelegate is responsible for keeping the
 // renderers' scores up to date in /proc/<pid>/oom_score_adj.
-//
-// Note that AdjustOomPriorities will be called on the UI thread by
-// TabManager, but the actual work will take place on the file thread
-// (see implementation of AdjustOomPriorities).
 class TabManagerDelegate
     : public arc::InstanceHolder<arc::mojom::ProcessInstance>::Observer,
       public aura::client::ActivationChangeObserver,
@@ -89,12 +86,6 @@ class TabManagerDelegate
   void AdjustOomPriorities(const TabStatsList& tab_list);
 
  protected:
-  // Sets oom_score_adj for a list of tabs.
-  // This is a delegator to to SetOomScoreAdjForTabsOnFileThread(),
-  // also as a seam for unit test.
-  virtual void SetOomScoreAdjForTabs(
-      const std::vector<std::pair<base::ProcessHandle, int>>& entries);
-
   // Kills an Arc process. Returns true if the kill request is successfully sent
   // to Android. Virtual for unit testing.
   virtual bool KillArcProcess(const int nspid);
@@ -103,6 +94,9 @@ class TabManagerDelegate
   // Virtual for unit testing.
   virtual bool KillTab(int64_t tab_id);
 
+  // Get debugd client instance.
+  virtual chromeos::DebugDaemonClient* GetDebugDaemonClient();
+
  private:
   FRIEND_TEST_ALL_PREFIXES(TabManagerDelegateTest, CandidatesSorted);
   FRIEND_TEST_ALL_PREFIXES(TabManagerDelegateTest, KillMultipleProcesses);
@@ -131,7 +125,7 @@ class TabManagerDelegate
       const TabStatsList& tab_list,
       const std::vector<arc::ArcProcess>& arc_processes);
 
-  // Posts AdjustFocusedTabScore task to the file thread.
+  // Sets OOM score for the focused tab.
   void OnFocusTabScoreAdjustmentTimeout();
 
   // Kills a process after getting all info of tabs and apps.
@@ -142,9 +136,6 @@ class TabManagerDelegate
   void AdjustOomPriorities(const TabStatsList& tab_list,
                            const std::vector<arc::ArcProcess>& arc_processes);
 
-  // Sets the score of the focused tab to the least value.
-  void AdjustFocusedTabScoreOnFileThread();
-
   // Sets a newly focused tab the highest priority process if it wasn't.
   void AdjustFocusedTabScore(base::ProcessHandle pid);
 
@@ -153,13 +144,6 @@ class TabManagerDelegate
       const TabStatsList& tab_list,
       const std::vector<arc::ArcProcess>& arc_processes);
 
-  // Sets oom_score_adj of an ARC app.
-  void SetOomScoreAdjForApp(int nspid, int score);
-
-  // Sets oom_score_adj for a list of tabs on the file thread.
-  void SetOomScoreAdjForTabsOnFileThread(
-      const std::vector<std::pair<base::ProcessHandle, int>>& entries);
-
   // Sets OOM score for processes in the range [|rbegin|, |rend|) to integers
   // distributed evenly in [|range_begin|, |range_end|).
   // The new score is set in |new_map|.
@@ -186,13 +170,10 @@ class TabManagerDelegate
   // adjusted when |focus_process_score_adjust_timer_| is expired.
   std::unique_ptr<FocusedProcess> focused_process_;
 
-  // This lock is for |oom_score_map_|.
-  base::Lock oom_score_lock_;
-  // Map maintaining the process handle - oom_score mapping. Behind
-  // |oom_score_lock_|.
+  // Map maintaining the process handle - oom_score mapping.
   ProcessScoreMap oom_score_map_;
 
-  // Util for getting system memory satatus.
+  // Util for getting system memory status.
   std::unique_ptr<TabManagerDelegate::MemoryStat> mem_stat_;
 
   // Holds a weak pointer to arc::mojom::ProcessInstance.

chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc

@@ -13,11 +13,13 @@
 #include "ash/test/ash_test_base.h"
 #include "base/logging.h"
 #include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop.h"
 #include "base/process/process_handle.h"
 #include "chrome/browser/chromeos/arc/arc_process.h"
 #include "chrome/browser/memory/tab_manager.h"
 #include "chrome/browser/memory/tab_stats.h"
 #include "chrome/common/url_constants.h"
+#include "chromeos/dbus/fake_debug_daemon_client.h"
 #include "components/arc/common/process.mojom.h"
 #include "components/arc/test/fake_arc_bridge_service.h"
 #include "components/exo/shell_surface.h"
@@ -118,11 +120,6 @@ class MockTabManagerDelegate : public TabManagerDelegate {
   }
 
  protected:
-  // Nullify the operation for unit test.
-  void SetOomScoreAdjForTabs(
-      const std::vector<std::pair<base::ProcessHandle, int>>& entries)
-      override {}
-
   bool KillArcProcess(const int nspid) override {
     killed_arc_processes_.push_back(nspid);
     return true;
@@ -133,7 +130,12 @@ class MockTabManagerDelegate : public TabManagerDelegate {
     return true;
   }
 
+  chromeos::DebugDaemonClient* GetDebugDaemonClient() override {
+    return &debugd_client_;
+  }
+
  private:
+  chromeos::FakeDebugDaemonClient debugd_client_;
   std::vector<int> killed_arc_processes_;
   std::vector<int64_t> killed_tabs_;
 };
@@ -167,6 +169,7 @@ class MockMemoryStat : public TabManagerDelegate::MemoryStat {
 };
 
 TEST_F(TabManagerDelegateTest, SetOomScoreAdj) {
+  base::MessageLoop message_loop;
   arc::FakeArcBridgeService fake_arc_bridge_service;
   MockTabManagerDelegate tab_manager_delegate;
 

chromeos/dbus/debug_daemon_client.cc

@@ -8,6 +8,8 @@
 #include <stddef.h>
 #include <stdint.h>
 #include <unistd.h>
+
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -292,7 +294,7 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
         debugd::kDebugdInterface,
         debugd::kSystraceStart);
     dbus::MessageWriter writer(&method_call);
-    writer.AppendString("all"); // TODO(sleffler) parameterize category list
+    writer.AppendString("all");  // TODO(sleffler) parameterize category list
 
     DVLOG(1) << "Requesting a systrace start";
     debugdaemon_proxy_->CallMethod(
@@ -442,6 +444,30 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
     debugdaemon_proxy_->WaitForServiceToBeAvailable(callback);
   }
 
+  void SetOomScoreAdj(const std::map<pid_t, int32_t>& pid_to_oom_score_adj,
+                      const SetOomScoreAdjCallback& callback) override {
+    dbus::MethodCall method_call(debugd::kDebugdInterface,
+                                 debugd::kSetOomScoreAdj);
+    dbus::MessageWriter writer(&method_call);
+
+    dbus::MessageWriter sub_writer(nullptr);
+    writer.OpenArray("{ii}", &sub_writer);
+
+    dbus::MessageWriter elem_writer(nullptr);
+    for (const auto& entry : pid_to_oom_score_adj) {
+      sub_writer.OpenDictEntry(&elem_writer);
+      elem_writer.AppendInt32(entry.first);
+      elem_writer.AppendInt32(entry.second);
+      sub_writer.CloseContainer(&elem_writer);
+    }
+    writer.CloseContainer(&sub_writer);
+
+    debugdaemon_proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&DebugDaemonClientImpl::OnSetOomScoreAdj,
+                   weak_ptr_factory_.GetWeakPtr(), callback));
+  }
+
  protected:
   void Init(dbus::Bus* bus) override {
     debugdaemon_proxy_ =
@@ -527,7 +553,7 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
   void OnGetAllLogs(const GetLogsCallback& callback,
                     dbus::Response* response) {
     std::map<std::string, std::string> logs;
-    bool broken = false; // did we see a broken (k,v) pair?
+    bool broken = false;  // did we see a broken (k,v) pair?
     dbus::MessageReader sub_reader(NULL);
     if (!response || !dbus::MessageReader(response).PopArray(&sub_reader)) {
       callback.Run(false, logs);
@@ -623,7 +649,7 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
       // If debugd crashes or completes I/O before this message is processed
       // then pipe_reader_ can be NULL, see OnIOComplete().
       if (pipe_reader_.get())
-        pipe_reader_->OnDataReady(-1); // terminate data stream
+        pipe_reader_->OnDataReady(-1);  // terminate data stream
     }
     // NB: requester is signaled when i/o completes
   }
@@ -645,6 +671,15 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
     pipe_reader_.reset();
   }
 
+  void OnSetOomScoreAdj(const SetOomScoreAdjCallback& callback,
+                        dbus::Response* response) {
+    std::string output;
+    if (response && dbus::MessageReader(response).PopString(&output))
+      callback.Run(true, output);
+    else
+      callback.Run(false, "");
+  }
+
   dbus::ObjectProxy* debugdaemon_proxy_;
   std::unique_ptr<PipeReaderForString> pipe_reader_;
   StopAgentTracingCallback callback_;

chromeos/dbus/debug_daemon_client.h

@@ -6,8 +6,11 @@
 #define CHROMEOS_DBUS_DEBUG_DAEMON_CLIENT_H_
 
 #include <stdint.h>
+#include <sys/types.h>
 
 #include <map>
+#include <string>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/files/file.h"
@@ -194,6 +197,18 @@ class CHROMEOS_EXPORT DebugDaemonClient
   virtual void WaitForServiceToBeAvailable(
       const WaitForServiceToBeAvailableCallback& callback) = 0;
 
+  // A callback for SetOomScoreAdj().
+  typedef base::Callback<void(bool success, const std::string& output)>
+      SetOomScoreAdjCallback;
+
+  // Set OOM score oom_score_adj for some process.
+  // Note that the corresponding DBus configuration of the debugd method
+  // "SetOomScoreAdj" only permits setting OOM score for processes running by
+  // user chronos or Android apps.
+  virtual void SetOomScoreAdj(
+      const std::map<pid_t, int32_t>& pid_to_oom_score_adj,
+      const SetOomScoreAdjCallback& callback) = 0;
+
   // Factory function, creates a new instance and returns ownership.
   // For normal usage, access the singleton via DBusThreadManager::Get().
   static DebugDaemonClient* Create();

chromeos/dbus/fake_debug_daemon_client.cc

@@ -195,6 +195,13 @@ void FakeDebugDaemonClient::WaitForServiceToBeAvailable(
   }
 }
 
+void FakeDebugDaemonClient::SetOomScoreAdj(
+    const std::map<pid_t, int32_t>& pid_to_oom_score_adj,
+    const SetOomScoreAdjCallback& callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(callback, false, ""));
+}
+
 void FakeDebugDaemonClient::SetDebuggingFeaturesStatus(int featues_mask) {
   featues_mask_ = featues_mask;
 }

chromeos/dbus/fake_debug_daemon_client.h

@@ -6,7 +6,10 @@
 #define CHROMEOS_DBUS_FAKE_DEBUG_DAEMON_CLIENT_H_
 
 #include <stdint.h>
+#include <sys/types.h>
 
+#include <map>
+#include <string>
 #include <vector>
 
 #include "base/compiler_specific.h"
@@ -66,6 +69,8 @@ class CHROMEOS_EXPORT FakeDebugDaemonClient : public DebugDaemonClient {
       const EnableDebuggingCallback& callback) override;
   void WaitForServiceToBeAvailable(
       const WaitForServiceToBeAvailableCallback& callback) override;
+  void SetOomScoreAdj(const std::map<pid_t, int32_t>& pid_to_oom_score_adj,
+                      const SetOomScoreAdjCallback& callback) override;
 
   // Sets debugging features mask for testing.
   virtual void SetDebuggingFeaturesStatus(int featues_mask);