Show all system process in the task_manager.

chrome/browser/chromeos/arc/arc_process_service.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // The main point of this class is to cache ARC proc nspid<->pid mapping
 // globally. Since the calculation is costly, a dedicated worker thread is
 // used. All read/write of its internal data structure (i.e., the mapping)
 // should be on this thread.
 
 #include "chrome/browser/chromeos/arc/arc_process_service.h"
 
 #include <queue>
 #include <set>
 #include <string>
 
+#include "base/callback_forward.h"

[Yusuke Sato, 2016/06/30 19:46:52]

including _forward.h in .cc seems a bit weird, and this file actually use
.Run(). Did you mean "base/callback.h" probably?

 #include "base/process/process.h"
 #include "base/process/process_iterator.h"
 #include "base/task_runner_util.h"
 #include "base/trace_event/trace_event.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace arc {
 
 namespace {
 
-const char kSequenceToken[] = "arc_process_service";
+constexpr char kSequenceToken[] = "arc_process_service";
 
 // Weak pointer.  This class is owned by ArcServiceManager.
 ArcProcessService* g_arc_process_service = nullptr;
 
 }  // namespace
 
 using base::kNullProcessId;
 using base::Process;
 using base::ProcessId;
 using base::SequencedWorkerPool;
@@ skipping 33 matching lines @@
       FROM_HERE,
       base::Bind(&ArcProcessService::Reset,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ArcProcessService::Reset() {
   DCHECK(thread_checker_.CalledOnValidThread());
   nspid_to_pid_.clear();
 }
 
-bool ArcProcessService::RequestProcessList(
+void ArcProcessService::PostTaskToOwnThreadAndReply(
+    const base::Closure& task,
+    const base::Closure& reply) {
+  auto runner = worker_pool_->GetSequencedTaskRunner(
+      worker_pool_->GetNamedSequenceToken(kSequenceToken));
+  runner->PostTaskAndReply(FROM_HERE, task, reply);
+}
+
+void ArcProcessService::RequestSystemProcessList(
     RequestProcessListCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  auto ret_processes = new vector<ArcProcess>();

[Luis Héctor Chávez, 2016/06/30 16:46:03]

Raw pointers scare me a bit.

This can potentially leak if this object goes away before CallbackRelay can be
called. Can you instead make GetArcSystemProcessList return a vector<ArcProcess>
instead? PostTaskAndReplyWithResult should be able to take care of that
scenario.

[Yusuke Sato, 2016/06/30 19:46:52]

+1, although you probably have to make ArcProcessService a base::RefCounted<>
class.

[cylee1, 2016/06/30 21:02:39]

When I wrote the code I tried to use PostTaskAndReplyWithResult as you said, but
WeakPtr can only bind to functions with no return type. See
https://cs.chromium.org/chromium/src/base/bind_internal.h?rcl=0&l=317
So I made it so.

That said, I found later that I shouldn't use WeakPtr here at all, because from
comment of WeakPtr:
// Weak pointers may be passed safely between threads, but must always be
// dereferenced and invalidated on the same SequencedTaskRunner otherwise
// checking the pointer would be racey.
I shouldn't use WeakPtr in another thread, because it may be invalidated in the
calling thread if ArcProcessService object dies (so the WeakPtrFactor
invalidates all WeakPtr it has issued).  
I know the problem but haven't find a time to fix yet. Sorry for it.

I think using Unretained here might be fine, or we need to use RefCounted as
Yusuke said.
I've thought of using RefCounted, but since ArcProcessService is managed by
ArcServiceManager
(https://cs.chromium.org/chromium/src/components/arc/arc_service_manager.h?rcl...)
and it uses unique_ptr there, we may need more change to make it happen...

Also Line 169 needs to be changed as well.

BTW, I found it not rare in our code base to new a raw pointer and use a
Unretained then Owned pair in PostTaskAndReply.
E.g.
https://cs.chromium.org/chromium/src/components/drive/chromeos/change_list_lo...
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/avat...
https://cs.chromium.org/chromium/src/components/drive/chromeos/file_system/co...
... and many
IMHO I think it may not be that risky.

Also the implementation of PostTaskAndReplyWithResult
https://cs.chromium.org/chromium/src/base/task_runner_util.h?rcl=1467300303&l=54
just new the ReturnType and use it like what I did here.
I don't seem to see any special handling to prevent from memory leak ??

[Yusuke Sato, 2016/06/30 22:23:53]

hmm... Luis, any idea?
I was more concerned about code quality. Using raw pointers makes the code more
difficult to modify (and review) IMO.
Typically the object passed from the first callback to the second one is
std::unique_ptr, which automatically prevents memory leaks.

[Luis Héctor Chávez, 2016/06/30 23:21:51]

CallbackRelay doesn't need to be a member function at all. It can be a
standalone function, obviating the need for the weak pointer for the second
call.

That being said, CallbackRelay might even be itself unnecessary, since IIUC you
can just pass the callback directly to PostTaskAndReplyWithResult.

[Yusuke Sato, 2016/06/30 23:40:50]

I think I misspoke. My understanding is that PostTaskAndReplyWithResult ensures
no memory leaks by forcing us to use RefCounted types.

[cylee1, 2016/07/01 23:15:14]

Yes.. I remember I introduced CallbackRelay because I thought I can't use
PostTaskAndReplyWithResult at that time.  If using PostTaskAndReplyWithResult
it's not needed.

[cylee1, 2016/07/01 23:15:14]

Sorry I don't see where it forcing us to use RefCounted type? But I remember
Bind forcing us to use either refcounted type of WeakPtrs, That's why it needs
UnRetained for raw pointers.
Anyway, I agree switching to PostTaskAndReplyWithResult() is the right way to
go.

+  PostTaskToOwnThreadAndReply(
+      base::Bind(&ArcProcessService::GetArcSystemProcessList,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Unretained(ret_processes)),

[Yusuke Sato, 2016/06/30 19:46:52]

remove

+      base::Bind(&ArcProcessService::CallbackRelay,
+                 weak_ptr_factory_.GetWeakPtr(), callback,
+                 base::Owned(ret_processes)));

[Yusuke Sato, 2016/06/30 19:46:52]

remove

+}
+
+void ArcProcessService::GetArcSystemProcessList(
+    vector<ArcProcess>* ret_processes) {

[Yusuke Sato, 2016/06/30 19:46:51]

remove once you switch to PostTaskAndReplyWithResult.

+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  const base::ProcessIterator::ProcessEntries& entry_list =
+      base::ProcessIterator(nullptr).Snapshot();
+  ProcessId arc_init_pid = GetArcInitProcessId(entry_list);
+
+  if (arc_init_pid == kNullProcessId) {
+    return;
+  }
+
+  // Enumerate the child processes of ARC init for gathering ARC System
+  // Processes.
+  for (const base::ProcessEntry& entry : entry_list) {
+    // TODO(hctsai): For now, we only gather direct child process of init, need
+    //               to get the processes below.

[Yusuke Sato, 2016/06/30 19:46:52]

Can you mention the intalled/dex2oat issue as an example?

+    if (entry.parent_pid() == arc_init_pid) {
+      const ProcessId child_pid = entry.pid();
+      const ProcessId child_nspid =
+          base::Process(child_pid).GetPidInNamespace();
+      const std::string process_name =
+          !entry.cmd_line_args().empty() ? entry.cmd_line_args()[0] : "";

[Yusuke Sato, 2016/06/30 19:46:52]

nit: can you factor this out as a function in anonymous namespace? line 298 is
doing the same.

+      if (child_nspid != kNullProcessId) {
+        ret_processes->emplace_back(child_nspid, child_pid, process_name,
+                                    mojom::ProcessState::PERSISTENT);
+      }
+    }
+  }
+}
+
+bool ArcProcessService::RequestAppProcessList(
+    RequestProcessListCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   arc::mojom::ProcessInstance* process_instance =
       arc_bridge_service()->process_instance();
   if (!process_instance) {
     return false;
   }
   process_instance->RequestProcessList(
       base::Bind(&ArcProcessService::OnReceiveProcessList,
                  weak_ptr_factory_.GetWeakPtr(),
                  callback));
   return true;
 }
 
 void ArcProcessService::OnReceiveProcessList(
     const RequestProcessListCallback& callback,
     mojo::Array<arc::mojom::RunningAppProcessInfoPtr> mojo_processes) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   auto raw_processes = new vector<mojom::RunningAppProcessInfoPtr>();

[Yusuke Sato, 2016/06/30 19:46:51]

Any reason to convert it to vector at this point? (see also my comment at line
185)

[cylee1, 2016/06/30 21:02:39]

I don't remember the exact reason, but I remember it's hard to bind
vector<mojom::RunningAppProcessInfoPtr> to a callback, even if I used move(),
Pass(), or anything alike. Compiler alway prohibit me from moving it . Sorry
that I didn't find a solution then.

   mojo_processes.Swap(raw_processes);
 
   auto ret_processes = new vector<ArcProcess>();

[Yusuke Sato, 2016/06/30 19:46:51]

This has the same, potential memory leak issue as line 93. Let's switch to
...WithResult().

   // Post to its dedicated worker thread to avoid race condition.
   // Since no two tasks with the same token should be run at the same.
   // Note: GetSequencedTaskRunner's shutdown behavior defaults to
   // SKIP_ON_SHUTDOWN (ongoing task blocks shutdown).
   // So in theory using Unretained(this) should be fine since the life cycle
   // of |this| is the same as the main browser.
   // To be safe I still use weak pointers, but weak_ptrs can only bind to
   // methods without return values. That's why I can't use
   // PostTaskAndReplyWithResult but handle the return object by myself.
-  auto runner = worker_pool_->GetSequencedTaskRunner(
-      worker_pool_->GetNamedSequenceToken(kSequenceToken));
-  runner->PostTaskAndReply(
-      FROM_HERE,
+  PostTaskToOwnThreadAndReply(
       base::Bind(&ArcProcessService::UpdateAndReturnProcessList,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 base::Owned(raw_processes),
+                 weak_ptr_factory_.GetWeakPtr(), base::Owned(raw_processes),

[Yusuke Sato, 2016/06/30 19:46:51]

base::Passed(&mojo_processes) ?

[Luis Héctor Chávez, 2016/06/30 19:50:37]

If you have to add a prefix to variables, I prefer to steer off "mojo_" and
prefer using something like "instance_" or "arc_".

                  base::Unretained(ret_processes)),

[Yusuke Sato, 2016/06/30 19:46:52]

remove

       base::Bind(&ArcProcessService::CallbackRelay,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 callback,
+                 weak_ptr_factory_.GetWeakPtr(), callback,
                  base::Owned(ret_processes)));

[Yusuke Sato, 2016/06/30 19:46:52]

remove

 }
 
 void ArcProcessService::CallbackRelay(
     const RequestProcessListCallback& callback,
     const vector<ArcProcess>* ret_processes) {

[Yusuke Sato, 2016/06/30 19:46:51]

unique_ptr instead of a raw pointer

[cylee1, 2016/06/30 21:02:39]

If use PostTaskAndReplyWithResult, maybe the vector is passed as a const
reference?
The same below

   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   callback.Run(*ret_processes);
 }
 
 void ArcProcessService::UpdateAndReturnProcessList(
     const vector<arc::mojom::RunningAppProcessInfoPtr>* raw_processes,

[Yusuke Sato, 2016/06/30 19:46:51]

Line 152 receives a smiliar type of object as value (with move), and this line
does with a raw pointer (with Owned). This is a bit difficult to read, and IIUC,
using a raw pointer like this is kind of discouraged in Chromium these days. Can
you change this line to

 mojo::Array<arc::mojom::RunningAppProcessInfoPtr> raw_processes

(or mojo_processes) and use base::Passed(&mojo_processes) at line 170?

     vector<ArcProcess>* ret_processes) {

[Yusuke Sato, 2016/06/30 19:46:52]

remove once you switch to ...WithResult().

   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Cleanup dead pids in the cache |nspid_to_pid_|.
   set<ProcessId> nspid_to_remove;
   for (const auto& entry : nspid_to_pid_) {
     nspid_to_remove.insert(entry.first);
   }
   bool unmapped_nspid = false;
   for (const auto& entry : *raw_processes) {
     // erase() returns 0 if coudln't find the key. It means a new process.
     if (nspid_to_remove.erase(entry->pid) == 0) {
       nspid_to_pid_[entry->pid] = kNullProcessId;
       unmapped_nspid = true;
     }
   }
   for (const auto& entry : nspid_to_remove) {
     nspid_to_pid_.erase(entry);
   }
 
   // The operation is costly so avoid calling it when possible.
   if (unmapped_nspid) {
     UpdateNspidToPidMap();
   }
 
   PopulateProcessList(raw_processes, ret_processes);
 }
 
 void ArcProcessService::PopulateProcessList(
     const vector<arc::mojom::RunningAppProcessInfoPtr>* raw_processes,

[Yusuke Sato, 2016/06/30 19:46:51]

mojo::Array<arc::mojom::RunningAppProcessInfoPtr> raw_processes

(or mojo_processes)

     vector<ArcProcess>* ret_processes) {

[Yusuke Sato, 2016/06/30 19:46:51]

Can you use unique_ptr instead of a raw pointer?

   DCHECK(thread_checker_.CalledOnValidThread());
 
   for (const auto& entry : *raw_processes) {
     const auto it = nspid_to_pid_.find(entry->pid);
     // In case the process already dies so couldn't find corresponding pid.

[cylee1, 2016/07/15 23:17:02]

Could you keep this comments or re-write it ?
I meant to explain why it's possible that it is not in nspid_to_pid_ (nspid is
returned but there's a race so the process had died when we update the map).

[Hsu-Cheng, 2016/07/27 08:07:59]

Done.

     if (it != nspid_to_pid_.end() && it->second != kNullProcessId) {
       ArcProcess arc_process(entry->pid, it->second, entry->process_name,
                              entry->process_state);
       // |entry->packages| is provided only when process.mojom's verion is >=4.
       if (entry->packages) {
         for (const auto& package : entry->packages) {
           arc_process.packages().push_back(package.get());
         }
       }
       ret_processes->push_back(std::move(arc_process));
@@ skipping 18 matching lines @@
   // processes may have the same nspid. We need to get the proper subset of
   // processes to create correct nspid -> pid map.
 
   // Construct the process tree.
   // NB: This can contain a loop in case of race conditions.
   map<ProcessId, vector<ProcessId> > process_tree;
   for (const base::ProcessEntry& entry : entry_list)
     process_tree[entry.parent_pid()].push_back(entry.pid());
 
   // Find the ARC init process.
-  ProcessId arc_init_pid = kNullProcessId;
-  for (const base::ProcessEntry& entry : entry_list) {
-    // TODO(nya): Add more constraints to avoid mismatches.
-    std::string process_name =
-        !entry.cmd_line_args().empty() ? entry.cmd_line_args()[0] : "";
-    if (process_name == "/init") {
-      arc_init_pid = entry.pid();
-      break;
-    }
-  }
+  ProcessId arc_init_pid = GetArcInitProcessId(entry_list);
 
   // Enumerate all processes under ARC init and create nspid -> pid map.
   if (arc_init_pid != kNullProcessId) {
     std::queue<ProcessId> queue;
     std::set<ProcessId> visited;

[Yusuke Sato, 2016/06/30 19:46:51]

nit: (not your fault but..) remove std:: to be consistent with line 190. Or
remove line 38-40 instead. I kind of prefer the latter as 'using std::xxx;' is
uncommon in chrome/.

     queue.push(arc_init_pid);
     while (!queue.empty()) {
       ProcessId pid = queue.front();
       queue.pop();
       // Do not visit the same process twice. Otherwise we may enter an infinite
       // loop if |process_tree| contains a loop.
       if (!visited.insert(pid).second)
         continue;
 
       ProcessId nspid = base::Process(pid).GetPidInNamespace();
 
       // All ARC processes should be in namespace so nspid is usually non-null,
       // but this can happen if the process has already gone.
       // Only add processes we're interested in (those appear as keys in
       // |nspid_to_pid_|).
       if (nspid != kNullProcessId &&
           nspid_to_pid_.find(nspid) != nspid_to_pid_.end())
         nspid_to_pid_[nspid] = pid;
 
       for (ProcessId child_pid : process_tree[pid])
         queue.push(child_pid);
     }
   }
 }
 
+// static
+ProcessId ArcProcessService::GetArcInitProcessId(
+    const base::ProcessIterator::ProcessEntries& entry_list) {
+  // Find the ARC init process.
+  for (const base::ProcessEntry& entry : entry_list) {
+    // TODO(nya): Add more constraints to avoid mismatches.
+    std::string process_name =
+        !entry.cmd_line_args().empty() ? entry.cmd_line_args()[0] : "";
+    if (process_name == "/init") {
+      return entry.pid();
+    }
+  }
+  return kNullProcessId;
+}
+
 }  // namespace arc